Timer class implementation

Question

I have made a timer class:

• The timer should call an async method and wait between two calls.
• I want to wake up the timer to skip the wait period.
• When I dispose the timer from outside the timer I want to wait until the current call is completed.
• I want to stop the timer from inside the timer.

public sealed class CompletionTimer : IDisposable
{
    private readonly CancellationTokenSource disposeToken = new CancellationTokenSource();
    private readonly Task runTask;
    private int requiresAtLeastOne;
    private CancellationTokenSource wakeupToken;

    public CompletionTimer(int delayInMs, Func<CancellationToken, Task> callback, int initialDelay = 0)
    {
        runTask = RunInternal(delayInMs, initialDelay, callback);
    }

    public void Dispose()
    {
        disposeToken.Cancel();

        runTask.Wait();
    }

    public void Wakeup()
    {
        ThrowIfDisposed();

        Interlocked.CompareExchange(ref requiresAtLeastOne, 2, 0);

        wakeupToken?.Cancel();
    }

    private async Task RunInternal(int delay, int initialDelay, Func<CancellationToken, Task> callback)
    {
        if (initialDelay > 0)
        {
            await WaitAsync(initialDelay).ConfigureAwait(false);
        }

        while (requiresAtLeastOne == 2 || !disposeToken.IsCancellationRequested)
        {
            try
            {
                await callback(disposeToken.Token).ConfigureAwait(false);
            }
            catch (OperationCanceledException)
            {
            }
            finally
            {
                requiresAtLeastOne = 1;
            }

            await WaitAsync(delay).ConfigureAwait(false);
        }
    }

    private async Task WaitAsync(int intervall)
    {
        try
        {
            wakeupToken = new CancellationTokenSource();

            using (var cts = CancellationTokenSource.CreateLinkedTokenSource(disposeToken.Token, wakeupToken.Token))
            {
                await Task.Delay(intervall, cts.Token).ConfigureAwait(false);
            }
        }
        catch (OperationCanceledException)
        {
        }
    }
}

The requiresAtLeastOne field is to enforce at least one run when I do something like that (in a test or so):

var timer = new CompletionTimer(10000, ct => ... );
timer.Wakeup();
timer.Dispose();

It almost works perfectly, but one time (of thousand of calls) I had a deadlock in the Dispose method, when I was calling it from within a timer. I am more confused why it does not deadlock all the time.

Answer 1

I'm a bit hesitant to comment on anything related to concurrency, but I'll give it a try:

• Why does Dispose wait for runTask to finish? To me, that's a rather surprising side-effect. I'd expect Dispose to stop the timer, and to abort the current invocation if possible, but not to block.
• Letting a callback dispose the CompletionTimer that's running it sounds like a bad idea - it seems like an inverted ownership situation.
• The deadlock seems fairly straightforward: RunInternal invokes callback asynchronously, so the rest of the code is registered as a continuation and will be run when callback finishes. However, callback disposes the CompletionTimer, which waits for runTask (RunInternal) to finish. The callback and the run task are now both waiting for each other to finish.
• If you want the callback to be able to stop the timer, perhaps changing its return type to Task<bool> is a better idea: the timer can then terminate its loop after the callback returns false.
• The requiresAtLeastOne construction looks a little confusing, with it being compared against a few 'magic values'. A requiredInvocationsLeft field that is decremented after each invocation seems a little easier to grasp - and would also allow you to configure the number of required invocations after a wake-up.
• Perhaps SkipCurrentDelay is a more descriptive name than WakeUp. WakeUp could be interpreted as the timer being in sleep mode (as in, it's paused, it's not periodically invoking its callback).
• You may want to document the intended behavior of your class. As my comments may show, other people likely have different assumptions about how a timer will behave.

Creating a separate StopAndWaitForCompletionAsync method is probably a better idea if you need to wait for the last invocation to finish.