Using lock with Threading.Timer

Question

I have a Windows Service application which uses a Threading.Timer and a TimerCallback to do some processing at particular intervals. I need to lock down this processing code to only 1 thread at a time.

So for example, the service is started and the first callback is triggered and a thread is started and begins processing. This works ok as long as the processing is completed before the next callback. So say for instance the processing is taking a little longer than usual and the TimerCallback is triggered again whilst another thread is processing, I need to make that thread wait until the other thread is done.

Here's a sample of my code:

static Timer timer;
static object locker = new object();

public void Start()
{
    var callback = new TimerCallback(DoSomething);
    timer = new Timer(callback, null, 0, 10000);
}

public void DoSomething()
{
      lock(locker)
      {
           // my processing code
      }
}

Is this a safe way of doing this? What happens if the queue gets quite substantial? Is there a better option?

Answer 1

If it's OK for you to have the events fire with a constant interval between them (as opposed to the current code which fires them at a constant interval) then you can start the timer without a period, and each time queue up a new callback, e.g.

static Timer timer;

public void Start()
{
    var callback = new TimerCallback(DoSomething);
    timer = new Timer(callback, null, 0, Timeout.Infinite);
}

public void DoSomething()
{
      try
      {
           // my processing code
      }
      finally
      {
          timer.Change(10000, Timeout.Infinite);
      }
}

This code tells the newly created timer to fire immediately, once only. In the processing code it does the work and then tells the timer to fire again in 10 seconds, once only. Because the timer is now not firing periodically but is being re-started by its callback method then the callback is guaranteed to be single-threaded with no queue.

If you want to keep a constant interval, then it's a bit trickier as you have to decide what to do if the processing starts taking longer than the timer interval. One option is to do what you're currently doing but that will essentially end up with a lot of queued threads and eventual thread pool starvation. Another option is to simply discard the callback if there is already one in progress, e.g.

static Timer timer;
static object locker = new object();

public void Start()
{
    var callback = new TimerCallback(DoSomething);
    timer = new Timer(callback, null, 0, 10000);
}

public void DoSomething()
{
      if (Monitor.TryEnter(locker))
      {
           try
           {
               // my processing code
           }
           finally
           {
               Monitor.Exit(locker);
           }
      }
}

Answer 2

The worst that can happen if the processing code takes more than 10s to execute is that you will be wasting 1 threadpool thread every time there's a new callback called (they will be waiting for in the lock statement). And if you take all the threadpool threads HttpWebRequest, ASP.NET, asynchronous delegate invocations... will suffer.

What I would do is to schedule the first callback immediately. Then, if you really need your DoSomething() to be called every 10s:

public void DoSomething ()
{
       DateTime start = DateTime.UtcNow;
       ...
       TimeSpan elapsed = (DateTime.UtcNow - start);
       int due_in = (int) (10000 - elapsed.TotalMilliseconds);
       if (due_in < 0)
           due_in = 0;
       timer.Change (due_in, Timeout.Infinite);
}

Or something along that line.