Safe Dispose of Timer

Question

I am currently refactoring a larger solution where the compiler gave multiple warnings about disposing the used System.Timers.Timer instances. The timers are running in short intervals so I would have to check before I dispose the timer if the elapsed callback is currently active.

Following the implementation with which I want to replace the System.Timers.Timer instances.

public class DisposableSafeTimer : IDisposable
{
    public event ElapsedEventHandler Elapsed;

    private System.Timers.Timer _timer;
    private readonly object _syncObject = new object();
    private volatile bool _isDisposing = false;

    public double Interval
    {
        get { return _timer.Interval; }
        set { _timer.Interval = value; }
    }

    public DisposableSafeTimer()
    {
        _timer = new System.Timers.Timer();
        _timer.Elapsed += _timer_Elapsed;
    }

    public void ExternalStart()
    {
        _timer.Start();
    }

    public void ExternalStop()
    {
        _timer.Stop();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        lock (_syncObject)
        {
            if(_isDisposing)
            {
                return;
            }

            if (disposing)
            {
                _isDisposing = disposing;
                _timer.Stop();
                _timer.Elapsed -= _timer_Elapsed;
                _timer.Dispose();
            }
        }
    }

    private void _timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
    {
        lock (_syncObject)
        {
            if (_isDisposing)
            {
                return;
            }

            try
            {
                _timer.Stop();
                Elapsed?.Invoke(sender, e);
            }
            finally
            {
                _timer.Start();
            }
        }
    }
}

The Methods ExternalStart() and ExternalStop() are named with the intention that I get compiler errors wherever the Timer.Start() and Timer.Stop() methods are called. The stops and starts from the classes which use my timer implementation shouldn't care about the cyclic starts and stops of the internal timer.

So far I had no problems with my tests. I just want to make sure that I haven't overlooked something. Suggestions for improvements are welcome.

Answer 1

Review

• property Interval and methods ExternalStart, ExternalStart should throw ObjectDisposedException if _isDisposing is true
• property Interval and methods ExternalStart, ExternalStart should also acquire a lock on _syncObject
• when implementing the dispose pattern make sure to include a destructor ~DisposableSafeTimer or seal your class
• _isDisposing should be renamed to _disposed
• when disposing, you should also clean your event listeners Elapsed = null to avoid a memory leak
• do you really want to put Elapsed?.Invoke(sender, e); inside the lock? Think about possible race conditions or other side effects. What if a registered listener calls Dispose in the listener?
• check out different suggestions to reset the timer. Yours is fine though.
• a tiny enhancement might be to create the lock only when it is null using Interlocked.CompareExchange, instead of immediately creating an instance.

Answer 2

private void _timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
{
    lock (_syncObject)
    {
        if (_isDisposing)
        {
            return;
        }            
        try
        {
            _timer.Stop();
            Elapsed?.Invoke(sender, e);
        }
        finally
        {
            _timer.Start();
        }
    }
}

As dfhwze writes - you can't lock in this way because it's a candidate for race conditions. And the only reason - I can see - you have to do it, is because you halts the timer, while the event consumers do their job. This also means that you effectively hands over the timer interval to the laziest event handler. Theoretically that could be one that opens a modal message box (which is not closed because the operator is to lunch or on vacation) with an error or something else that prevent it from finishing its job - which will cause all consumers (on different threads) to wait, and you then effectively disables the benefits/necessity of the multithreaded design. I anticipate that you stop and start the timer here, because you don't want the event handlers to be called if the previous call hasn't returned?

The above code actual acts as a single thread bottleneck that synchronize the threads with the slowest thread/event handler. Is that by design?

If you want to let the different threads work independently of each other you could invoke each handler in a thread by it self:

private void _timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
{
  try
  {
    foreach (Delegate handler in Elapsed.GetInvocationList())
    {
      ThreadPool.QueueUserWorkItem(_ =>
      {
        handler.DynamicInvoke(sender, e);
      });
    }
  }
  finally
  {
  }
}

The above doesn't prevent a handler to be called before the previous call to that handler has finished. To Handle that situation, you'll have to maintain a dictionary (ConcurrentDictionary<Delegate, bool> for instancce) that controls if a handler is ready for a new call or not.

I of course have no idea of which impact this will have on your application otherwise - you'll have to test that thoroughly.