Is this a correct Interlocked synchronization design?

Question

I have a system that takes Samples. I have multiple client threads in the application that are interested in these Samples, but the actual process of taking a Sample can only occur in one context. It's fast enough that it's okay for it to block the calling process until Sampling is done, but slow enough that I don't want multiple threads piling up requests. I came up with this design (stripped down to minimal details):

public class Sample
{
    private static Sample _lastSample;
    private static int _isSampling;

    public static Sample TakeSample(AutomationManager automation)
    {
        //Only start sampling if not already sampling in some other context
        if (Interlocked.CompareExchange(ref _isSampling, 0, 1) == 0)
        {
            try
            {
                Sample sample = new Sample();
                sample.PerformSampling(automation);
                _lastSample = sample;
            }
            finally
            {
                //We're done sampling
                _isSampling = 0;
            }
        }

        return _lastSample;
    }

    private void PerformSampling(AutomationManager automation)
    {
        //Lots of stuff going on that shouldn't be run in more than one context at the same time
    }
}

Is this safe for use in the scenario I described?

H H · Accepted Answer · 2010-04-08T07:06:10.833

5

Yes, it looks safe because int is an atomic type here. But I would still advice replacing

private static int _isSampling;

with

private static object _samplingLock = new object();

and use :

lock(_samplingLock)
{
    Sample sample = new Sample();
    sample.PerformSampling(automation);
   _lastSample = sample;
}

Simply because it is the recommended pattern and also makes sure all access to _lastSample is treated correctly.

NB: I would expect comparable speed, lock uses the managed Monitor class that uses Interlocked internally.

Edit:

I missed the back-off aspect, here is another version :

   if (System.Threading.Monitor.TryEnter(_samplingLock))
   {
     try
     {
         .... // sample stuff
     }
     finally
     {
          System.Threading.Monitor.Exit(_samplingLock);
     }
   }

edited Apr 08 '10 at 07:06

answered Apr 07 '10 at 21:27

H H

263,252
30
330
514

1

The TryEnter is probably better from a maintenance perspective, as most likely more people will be familiar with Monitor.TryEnter than with Interlocked.CompareExchange. I'm not too worried about overhead in this case, as I'm only taking samples a few times per second. When it does actually perform sampling, the overhead of the sampling process itself will be quite a bit more than the locking primitive, I'm sure. – Dan Bryant Apr 07 '10 at 21:56
I like the Monitor.TryEnter, but shouldnt we be using the ReaderWriterLockSlim these days?? http://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim_members.aspx – Mike_G Apr 07 '10 at 21:57
@Mike_g : No, only when the situation calls for it. I don't see that here. – H H Apr 07 '10 at 22:09
Okay, it's using TryEnter now; so much for the 'clever' solution. :) On a side note, I can't edit the post, but the correct method is Monitor.Exit(_samplingLock). – Dan Bryant Apr 07 '10 at 22:17
1

Interlocked.CompareExchange() is very efficient than the given solution. You just changed what he asked. – Simple Fellow Oct 19 '15 at 11:04
@SimpleFellow - not clear what you mean. "efficient" has been dealt with here, it's about the general practice of locking a resource like _lastSample . – H H Oct 19 '15 at 11:58
isn't Interlocked.CompareExchange() is times faster than Monitor.TryEnter()/Exit()? (Reference: http://blog.teamleadnet.com/2012/02/why-autoresetevent-is-slow-and-how-to.html) – Simple Fellow Oct 19 '15 at 14:27
It is faster, but will you notice that here? Don't micro-optimize at the cost of readability. – H H Oct 21 '15 at 07:16

Mike_G · Answer 2 · 2010-04-07T21:50:19.773

-1

I usually declare a volatile bool and do something like:

private volatile bool _isBusy;
private static Sample _lastSample;

private Sample DoSomething()
{
     lock(_lastSample)
     {
       if(_isBusy)
          return _lastSample;
       _isBusy = true;
     }

     try
     {
       _lastSample = new sameple//do something
     }
     finally
     {
        lock(_lastSample)
        {
           _isBusy = false;
        }
     }
     return _lastSample;
}

edited Apr 07 '10 at 21:50

answered Apr 07 '10 at 21:37

Mike_G

16,237
14
70
101

1

It's unnecessary to declare the bool volatile if you're only using it inside a lock anyway. Besides, it's a very bad practice to lock on a mutable field. In your sample, it's possible two lock blocks are entered at the same time. When you lock on a mutable field I would expect a detailed comment about why it couldn't hurt (that much) if two (or maybe even more) threads entered a lock block simultaneously. – Patrick Huizinga Apr 18 '11 at 08:53

Is this a correct Interlocked synchronization design?

2 Answers2