CODEWITHSUNDEEP
c#.netthread-safety
Grant Thomas
Grant Thomas

Reputation: 45058

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

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?

Upvotes: 8

Views: 8630

Answers (3)

entiat
entiat

Reputation: 473

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);
    }
}

Upvotes: 1

Hans Passant
Hans Passant

Reputation: 942000

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.

Upvotes: 13

Jim Mischel
Jim Mischel

Reputation: 134035

It really depends on if you have to process every tick. If you just want to update some data periodically to reflect the current state of things, then you're probably okay discarding ticks that occur while the previous tick is being processed. If, on the other hand, you must process each tick, then you have a problem because in those cases you usually have to process each tick in a timely manner.

Also, if your tick handler regularly takes longer than the timer interval, then either your timer interval is too short or your tick handler needs to be optimized. And you run the risk of having a huge backlog of ticks to process, meaning that whatever status you're updating is somewhat delayed.

If you decide to throw out overlapping ticks (i.e. calls that come in while the previous tick is being handled), I would recommend using a one-shot timer that you reset every time after processing. That is, rather than discarding ticks make it impossible for overlapping ticks to occur.

Is there a particular reason you're using ReaderWriterLockSlim.TryEnterWriteLock rather than Monitor.TryEnter?

Also, if you're going to use ReaderWriterLockSlim or Monitor, you should protect them with try...finally. That is, using Monitor:

if (myLock.TryEnter(0))
{
    try
    {
        // process
    }
    finally
    {
        myLock.Exit();
    }
}

Upvotes: 0

Related Questions