5
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ What compiler errors do you get? It doesn't look like there would be any name conflicts... \$\endgroup\$ – t3chb0t Jul 11 at 6:39
  • \$\begingroup\$ There are no Errors in the class. I want them to appear if the System.Timers.Timer is replaced with my implementation, as I can then check if the Start() and Stop() calls are valid. \$\endgroup\$ – AFrueh Jul 11 at 8:10
4
\$\begingroup\$

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.
\$\endgroup\$
  • 1
    \$\begingroup\$ Why do you mean that it's necessary to implement a finalizer/destructor in this case? There isn't allocated any unmanaged resources to release here. The Dispose() should be sufficient. \$\endgroup\$ – Henrik Hansen Jul 11 at 5:43
  • \$\begingroup\$ @HenrikHansen Recently someone asked about this exact scenario, where there are no unmanaged resources. I can't seem to find the post atm. Since the class is not sealed, it was considered good practice to create the destructor. \$\endgroup\$ – dfhwze Jul 11 at 5:45
3
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ Is there a reason you favor ThreadPool.QueueUserWorkItem over SynchronizationContex.Current.Post or Task.Run? \$\endgroup\$ – dfhwze Jul 11 at 7:25
  • \$\begingroup\$ @dfhwze: Not really, I just read the question as it's about an "old school" multithreaded environment so it was my first choice. Are there any good reasons to choose one of the others? \$\endgroup\$ – Henrik Hansen Jul 11 at 7:35
  • 1
    \$\begingroup\$ In the order I posted them, they get a bit less performant, but with better API support for extensibility, continuations, error handling, dispatching to other threading models etc. \$\endgroup\$ – dfhwze Jul 11 at 7:37
  • \$\begingroup\$ It is a project that communicates with multiple PLC's. Each timer event is only consumed once by the class which creates the timer. In the Event handler, data is checked and written to the PLC. The write must finish before the timer can start again. Concerning the race conditions, how would you handle the check in the dispose method if the timer Event is currently running and wait for it to finish? \$\endgroup\$ – AFrueh Jul 11 at 8:08
  • \$\begingroup\$ how would you handle the check in the dispose method if .. You could use a semaphore or manualresetevent. I would ask a follow-up question if you desire this behavior. It's also good to know you have a single listener. \$\endgroup\$ – dfhwze Jul 11 at 8:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.