Thread-safe execution using System.Threading.Timer and Monitor

Question

Using a System.Threading.Timer results in threads being spun from a ThreadPool, which means if the interval of execution for the timer expires while a thread is still processing by order of a previous request, then the same callback will be delegated to execute on another thread. This is obviously going to cause problems in most cases unless the callback is re-entrant aware, but I'm wondering how to go about it the best (meaning safe) way.

Let's say we have the following:

ReaderWriterLockSlim OneAtATimeLocker = new ReaderWriterLockSlim();

OneAtATimeCallback = new TimerCallback(OnOneAtATimeTimerElapsed);
OneAtATimeTimer = new Timer(OneAtATimeCallback , null, 0, 1000);

Should the whole shebang be be locked down, as such:

private void OnOneAtATimeTimerElapsed(object state)
{
    if (OneAtATimeLocker.TryEnterWriteLock(0))
    {
        //get real busy for two seconds or more

        OneAtATimeLocker.ExitWriteLock();
    }
}

Or, should only entry be managed, and kick out 'trespassers', as such:

private void OnOneAtATimeTimerElapsed(object state)
{
    if (!RestrictOneAtATime())
    {
        return;
    }

    //get real busy for two seconds or more

    if(!ReleaseOneAtATime())
    {
        //Well, Hell's bells and buckets of blood!
    }       
}

bool OneAtATimeInProgress = false;

private bool RestrictToOneAtATime()
{
    bool result = false;
    if (OneAtATimeLocker.TryEnterWriteLock(0))
    {
        if(!OneAtATimeInProgress)
        {
            OneAtATimeInProgress = true;
            result = true;
        }
        OneAtATimeLocker.ExitWriteLock();
    }
    return result;
}

private bool ReleaseOneAtATime()
{
    bool result = false;
    //there shouldn't be any 'trying' about it...
    if (OneAtATimeLocker.TryEnterWriteLock(0))
    {            
        if(OneAtATimeInProgress)
        {
            OneAtATimeInProgress = false;
            result = true;
        }
        OneAtATimeLocker.ExitWriteLock();
    }
    return result;
}

Does the first have any performance implications because it locks for the extent of the method?

Does the second even offer the safety one might think it does - is something escaping me?

Are there any other ways to go about this reliably, and preferably?

Answer 1

Lots of ways to deal with this. A simple way is to just not make the timer periodic, make it a one shot by only setting the dueTime argument. Then re-enable the timer in the callback in a finally block. That guarantees that the callback cannot run concurrently.

This is of course makes the interval variable by the execution time of the callback. If that's not desirable and the callback only occasionally takes longer than the timer period then a simple lock will get the job done. Yet another strategy is Monitor.TryEnter and just give up on the callback if it returns false. None of these are particularly superior, pick what you like best.

Answer 2

A slightly different riff on the same concept, if you do not have to process every tick. This simply bails on the current timer tick if you are still processing a previous tick.

private int reentrancyCount = 0;
...
OnTick()
{
    try
    {
        if (Interlocked.Increment(ref reentrancyCount) > 1)
            return;

        // Do stuff
    }
    finally
    {
        Interlocked.Decrement(ref reentrancyCount);
    }
}