Use Interlocked.CompareExchange sync with a sync object in a Timer HandleElapsed handler

Question

I m reading a MSDN example http://msdn.microsoft.com/en-us/library/system.timers.timer.stop.aspx

In the timer.stop example, i suspected its way of using Interlocked.CompareExchange is not right.

private static void HandleElapsed(object sender, ElapsedEventArgs e)
{
    numEvents += 1;

    // This example assumes that overlapping events can be 
    // discarded. That is, if an Elapsed event is raised before  
    // the previous event is finished processing, the second 
    // event is ignored.  
    // 
    // CompareExchange is used to take control of syncPoint,  
    // and to determine whether the attempt was successful.  
    // CompareExchange attempts to put 1 into syncPoint, but 
    // only if the current value of syncPoint is zero  
    // (specified by the third parameter). If another thread 
    // has set syncPoint to 1, or if the control thread has 
    // set syncPoint to -1, the current event is skipped.  
    // (Normally it would not be necessary to use a local  
    // variable for the return value. A local variable is  
    // used here to determine the reason the event was  
    // skipped.) 
    // 
    int sync = Interlocked.CompareExchange(ref syncPoint, 1, 0);
    if (sync == 0)
    {
        // No other event was executing. 
        // The event handler simulates an amount of work 
        // lasting between 50 and 200 milliseconds, so that 
        // some events will overlap. 
        int delay = timerIntervalBase 
            - timerIntervalDelta / 2 + rand.Next(timerIntervalDelta);
        Thread.Sleep(delay);
        numExecuted += 1;

        // Release control of syncPoint.
        syncPoint = 0;
    }
    else
    {
        if (sync == 1) { numSkipped += 1; } else { numLate += 1; }
    }
}

My question is this block

int sync = Interlocked.CompareExchange(ref syncPoint, 1, 0);
if (sync == 0)
{
   // lots of code here
    syncPoint = 0;
}

Should it be like

int sync = Interlocked.CompareExchange(ref syncPoint, 1, 0);
if (sync == 0)
{
// lots of code here
   Interlocked.CompareExchange(ref syncPoint, 0, 1)
}

Because the original assigment syncPoint = 0; is not thread safe. Am I right?

UPDATE:

I updated the example, i have no question about the return value of ComareExchange, my question is about the the assignment of variable syncpoint in the end of this if block. There is no interlock on it at all.

Answer 1

This is atomical, so threadsafe

int sync = Interlocked.CompareExchange(ref syncPoint, 1, 0);
if (sync == 0)

It means: if the syncPoint is 0, put it at 1, give me the old value (so 0) and put it in sync (a local variable). Then check if the exchange was done (so if the old value was 0)

What I would make different is instead of

syncPoint = 0;

I would use

Interlocked.Exchange(ref syncPoint, 0);

just to be sure that other threads can immediately see that the "lock" is free, otherwise the current thread could delay "writing" that syncPoint = 0 and no other threads could enter the "lock".

BUT

in the example given this is not a problem: HandleElapsed is called in response to a event (the Timer probably uses OS timers)... I can't even imagine that after the event has been handled there isn't any code that generates a memory barrier (inside the .NET code or inside the Windows OS code), so that the syncPoint update will be made visible to other threads. The only difference is that perhaps it will be made visible "some hundred lines below that point" instead of "immediately now".

Let's check this theory: from here

If the SynchronizingObject property is null, the Elapsed event is raised on a ThreadPool thread. If processing of the Elapsed event lasts longer than Interval, the event might be raised again on another ThreadPool thread. In this situation, the event handler should be reentrant.

So, the event is fired on a new ThreadPool thread... That after the event is surely returned to the ThreadPool... Ok... There is surely a MemoryBarrier somewhere (at least to return the thread to the ThreadPool or to Sleep if the same thread is used more than once)