Should Timer be waiting for callback to finish before firing a new one?

Question

I am using the System.Threading.Timer class in one of my projects and I've noticed that the callback methods are called before the previous ones get to finish which is different than what I was expecting.

For example the following code

var delta = new TimeSpan(0,0,1);
var timer = new Timer(TestClass.MethodAsync, null, TimeSpan.Zero, delta);

static class TestClass
{
    static int i = 0;
    public static async void MethodAsync(object _)
    {
        i++;
        Console.WriteLine("method " + i + "started");

        await Task.Delay(10000);
        
        Console.WriteLine("method " + i + "finished");
    }
}

has this output

method 1started
method 2started
method 3started
method 4started
method 5started
method 6started
method 7started
method 8started
method 9started
method 10started
method 11started
method 11finished
method 12started
method 12finished

Which of course is not thread safe. What I was expecting is that a new call would be made to the callback method after the previous call has succeeded and additionally after the delta period is elapsed.

What I am looking for is where in the docs from Microsoft is this behavior documented and maybe if there is a built in way to make it wait for the callback calls to finish before starting new ones

Answer 1

The problem of overlapping event handlers is inherent with the classic multithreaded .NET timers (the System.Threading.Timer and the System.Timers.Timer). Attempting to solve this problem while remaining on the event-publisher-subscriber model is difficult, tricky, and error prone. The .NET 6 introduced a new timer, the PeriodicTimer, that attempts to solve this problem once and for all. Instead of handling an event, you start an asynchronous loop and await the PeriodicTimer.WaitForNextTickAsync method on each iteration. Example:

class TestClass : IDisposable
{
    private int i = 0;
    private PeriodicTimer _timer;

    public async Task StartAsynchronousLoop()
    {
        if (_timer != null) throw new InvalidOperationException();
        _timer = new(TimeSpan.FromSeconds(1));
        while (await _timer.WaitForNextTickAsync())
        {
            i++;
            Console.WriteLine($"Iteration {i} started");
            await Task.Delay(10000); // Simulate an I/O-bound operation
            Console.WriteLine($"Iteration {i} finished");
        }
    }

    public void Dispose() => _timer?.Dispose();
}

This way there is no possibility for overlapping executions, provided that you will start only one asynchronous loop.

The await _timer.WaitForNextTickAsync() returns false when the timer is disposed. You can also stop the loop be passing a CancellationToken as argument. When the token is canceled, the WaitForNextTickAsync will complete with an OperationCanceledException.

In case the periodic action is not asynchronous, you can offload it to the ThreadPool, by wrapping it in Task.Run:

await Task.Run(() => Thread.Sleep(10000)); // Simulate a blocking operation

If you are targeting a .NET platform older than .NET 6, you can find alternatives to the PeriodicTimer here.

Answer 2

What I am looking for is where in the docs from Microsoft is this behavior documented...

System.Threading.Timer

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.

System.Timers.Timer

The callback method executed by the timer should be reentrant, because it is called on ThreadPool threads.

For System.Windows.Forms.Timer this post asserts that the event does wait. The documentation doesn't seem very specific, but in the Microsoft Timer.Tick Event official example the code shows turning the timer off and on in the handler. So it seems that, regardless, steps are taken to prevent ticks and avoid reentrancy.

...and if there is a built in way to make it wait for the callback calls to finish before starting new ones.

According to the first Microsoft link (you might consider this a workaround, but it's straight from the horse's mouth):

One way to resolve this race condition is to set a flag that tells the event handler for the Elapsed event to ignore subsequent events.

The way I personally go about achieving this objective this is to call Wait(0) on the synchronization object of choice as a robust way to ignore reentrancy without having timer events piling up in a queue:

static SemaphoreSlim _sslim = new SemaphoreSlim(1, 1);
public static async void MethodAsync(object _)
{
    if (_sslim.Wait(0))
    {
        try
        {
            i++;
            Console.WriteLine($"method {i} started @ {DateTime.Now}");

            await Task.Delay(10000);

            Console.WriteLine($"method {i} finished @ {DateTime.Now}");
        }
        catch (Exception ex)
        {
            Debug.Assert(false, ex.Message);
        }
        finally
        {
            _sslim.Release();
        }
    }
}

In which case your MethodAsync generates this output: