c# System.Threading.Timer wait for dispose

Question

I have a class which uses a Timer. This class implements IDispose. I would like to wait in the Dispose method until the timer will not fire again.

I implement it like this:

private void TimerElapsed(object state)
{
    // do not execute the callback if one callback is still executing
    if (Interlocked.Exchange(ref _timerIsExecuting, 1) == 1) 
        return;

    try
    {
        _callback();
    }
    finally
    {
        Interlocked.Exchange(ref _timerIsExecuting, 0);
    }
}

public void Dispose()
{
    if (Interlocked.Exchange(ref _isDisposing, 1) == 1)
        return;

    _timer.Dispose();

    // wait until the callback is not executing anymore, if it was
    while (_timerIsExecuting == 0) 
    { }

    _callback = null;
}

Is this implementation correct? I think it mainly depends on the question if _timerIsExecuting == 0 is an atomic operation. Or would I have to use a WaitHandle. For me it seems it would make the code unnecessarily complicated...

I am not an expert in multi-threading, so would be happy about any advice.

Answer 1

Unless you have a reason not to use System.Threading.Timer This has a Dispose method with a wait handle

And you can do something like,

private readonly Timer Timer;
private readonly ManualResetEvent TimerDisposed;
public Constructor()
{
    Timer = ....;
    TimerDisposed = new ManualResetEvent(false);
}

public void Dispose()
{
    Timer.Dispose(TimerDisposed);
    TimerDisposed.WaitOne();
    TimerDisposed.Dispose();
}

Answer 2

Generally one can use the Timer.Dispose(WaitHandle) method, but there's a few pitfalls:

Pitfalls

• Support for multiple-disposal (see here)

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times. Instance methods other than Dispose can throw an ObjectDisposedException when resources are already disposed.

• Timer.Dispose(WaitHandle) can return false. It does so in case it's already been disposed (i had to look at the source code). In that case it won't set the WaitHandle - so don't wait on it! (Note: multiple disposal should be supported)

• not handling a WaitHandle timeout. Seriously - what are you waiting for in case you're not interested in a timeout?
• Concurrency issue as mentioned here on msdn where an ObjectDisposedException can occur during (not after) disposal.
• Timer.Dispose(WaitHandle) does not work properly with -Slim waithandles, or not as one would expect. For example, the following does not work (it blocks forever):

 using(var manualResetEventSlim = new ManualResetEventSlim)
 {
     timer.Dispose(manualResetEventSlim.WaitHandle);
     manualResetEventSlim.Wait();
 }

Solution

Well the title is a bit "bold" i guess, but below is my attempt to deal with the issue - a wrapper which handles double-disposal, timeouts, and ObjectDisposedException. It does not provide all of the methods on Timer though - but feel free to add them.

internal class Timer
{
    private readonly TimeSpan _disposalTimeout;

    private readonly System.Threading.Timer _timer;

    private bool _disposeEnded;

    public Timer(TimeSpan disposalTimeout)
    {
        _disposalTimeout = disposalTimeout;
        _timer = new System.Threading.Timer(HandleTimerElapsed);
    }

    public event Signal Elapsed;

    public void TriggerOnceIn(TimeSpan time)
    {
        try
        {
            _timer.Change(time, Timeout.InfiniteTimeSpan);
        }
        catch (ObjectDisposedException)
        {
            // race condition with Dispose can cause trigger to be called when underlying
            // timer is being disposed - and a change will fail in this case.
            // see 
            // https://msdn.microsoft.com/en-us/library/b97tkt95(v=vs.110).aspx#Anchor_2
            if (_disposeEnded)
            {
                // we still want to throw the exception in case someone really tries
                // to change the timer after disposal has finished
                // of course there's a slight race condition here where we might not
                // throw even though disposal is already done.
                // since the offending code would most likely already be "failing"
                // unreliably i personally can live with increasing the
                // "unreliable failure" time-window slightly
                throw;
            }
        }
    }

    private void HandleTimerElapsed(object state)
    {
        Elapsed.SafeInvoke();
    }

    public void Dispose()
    {
        using (var waitHandle = new ManualResetEvent(false))
        {
            // returns false on second dispose
            if (_timer.Dispose(waitHandle))
            {
                if (!waitHandle.WaitOne(_disposalTimeout))
                {
                    throw new TimeoutException(
                        "Timeout waiting for timer to stop. (...)");
                }
                _disposeEnded = true;
            }
        }
    }
}

Answer 3

Why you need to dispose the Timer manually? Isn't there any other solution. As a rule of thumb, you're better leaving this job to GAC. – LMB 56 mins ago

I am developing an ASP.NET application. The timer is disposed on the call of Dispose of the HttpApplication. The reason: A callback could access the logging system. So i have to assure the before disposing the logging system the timer is disposed. – SACO 50 mins ago

It looks like you have a Producer/Consumer pattern, using the timer as Porducer.

What I'd do in this case, would be to create a ConcurrentQueue() and make the timer enqueue jobs to the queue. And then, use another safe thread to read and execute the jobs.

This would prevent a job from overlapping another, which seems to be a requirement in your code, and also solve the timer disposing problem, since you could yourQueue == null before adding jobs.

This is the best design.

Another simple, but not robust, solution, is running the callbacks in a try block. I don't recommend to dispose the Timer manually.