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?

or you could stop the timer on entry to the tick event handler....and re-enable on exit... — Mitch Wheat, Feb 26 '11 at 00:46
I want the timer to keep ticking for auditing reasons, but thanks for the effort. — Grant Thomas, Feb 26 '11 at 01:27
you want to keep the timer ticking so that it can no work? That's a strange requirement! — Mitch Wheat, Feb 26 '11 at 01:38

score 13 · Accepted Answer · answered Feb 26 '11 at 01:09

13

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.

answered Feb 26 '11 at 01:09

Hans Passant

922,412
146
1,693
2,536

Rather than stop the timer I'd like to do something else; namely write log entries based on occurrences. Just one of the things I'm on the lookout for is such executions overlapping. Using `Monitor` to wrap the concerned code seems the most appropriate suggestion, I think. Thanks. – Grant Thomas Feb 26 '11 at 01:21
Not sure what you mean, I never mentioned stopping the timer. Using Monitor.TryEnter() lets you log overlaps. – Hans Passant Feb 26 '11 at 01:23
Sorry, I was referring to your 'one-shot timer' suggestion. – Grant Thomas Feb 26 '11 at 01:25
BTW, are there any noticeable performance implications of any suggestions? – Grant Thomas Feb 26 '11 at 01:26
No, acquiring a lock isn't expensive. A couple of dozen nanoseconds, give or take. – Hans Passant Feb 26 '11 at 01:29
For one-shot `System.Threading.Timer`s does one still need to lock since each invocation could/will be a different thread? – markmnl Oct 31 '16 at 05:33
To answer my own question looking at the reference source `Fire()` will always lock on something before invoking the callback https://referencesource.microsoft.com/#mscorlib/system/threading/timer.cs,9c6e2403064ef532. However its probably not a good idea to rely on an implementation detail.. – markmnl Oct 31 '16 at 05:38

score 1 · Answer 2 · answered Oct 12 '20 at 19:37

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

score 0 · Answer 3 · answered Feb 26 '11 at 01:05

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

Thanks for this info, but I mention in other comments that I actually need to audit the fact that this is happening; but at the same time, I don't want to queue up the requests - they can be discarded, just noted. But `0` means I expect the lock immediately, so they won't be queued, maybe `-1` will wait indefinitely, however. — Grant Thomas, Feb 26 '11 at 01:31

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

3 Answers3

Linked