Timer to poll a Delegate

Question

This is a followup to this where I was initially using a single thread and Thread.Sleep() to poll a delegate. I was recommended to use a timer and the ThreadPool to minimize resources and improve the responsiveness of StopMonitoring().

This is the first time I've used the ThreadPool and timers in this way. I am worried about things like:

1. Infinitely running threads

2. Race Conditions

3. Firing events after StopMonitoring() has been called

Especially since I expect the user to call StopMonitoring() followed by BeginMonitoring() shortly after performing some actions. The only time I want the exceptions in place to be thrown are if the user has forgotten to call StopMonitoring() or if they have passed in a delegate which is stuck in a long-running process or infinite loop.

1. Have I correctly implemented Timer and ThreadPool.QueueUserWorkItem to achieve what I want to achieve?
2. Is there any way I can have a call to StopMonitoring() guarantee that a subsequent call to BeginMonitoring() will always succeed, even if the last thread was a runaway?
3. What if a thread never becomes available on the thread pool, or is queued for an available thread while the user calls Stop and Begin again?

using System;
using System.Timers;
using System.Windows.Threading;
using Timer = System.Timers.Timer;

namespace FlagstoneRe.WPF.Utilities.Common
{
    /// <summary>This class allows a user to easily set up a seperate thread to poll some state,
    /// and set up an event that will fire if the state meets some condition.</summary>
    /// <typeparam name="T">The type of the value returned by the polling delegate.</typeparam>
    public class ConditionMonitor<T> : IDisposable
    {
        #region Private Properties
        private Object multiThreadLock = new Object();   //Prevent BeginMonitoring() race condition.
        private Dispatcher originThread = null;          //For event callbacks on the origin thread.
        private Timer nextRequest;                       //To delay between subsequent thread queuing.
        private volatile bool requestInProgress = false; //Prevent starting more than one thread.
        private volatile bool Halted = false;            //Prevent any further event callbacks.
        #endregion

        #region Delegates
        /// <summary>A delegate provided by the user of this class which returns the current state,
        /// to be tested against a certain condition in the IsConditionMet delegate.</summary>
        public delegate T RequestState();
        public RequestState RequestStateDelegate { get; set; }

        /// <summary>A delegate provided by the user of this class which determines whether given the
        /// current state, the polling program should execute the ConditionMet delegate.</summary>
        public delegate bool IsConditionMet(T state);
        public IsConditionMet IsConditionMetDelegate { get; set; }

        /// <summary>A delegate used to handle ConditionMonitor events.</summary>
        public delegate void ConditionMonitorHandler(ConditionMonitor<T> source, T state);
        /// <summary>An event which fires each time the state is polled (use sparingly).</summary>
        public event ConditionMonitorHandler RequestReceived;
        /// <summary>An event which fires when the condition is met.</summary>
        public event ConditionMonitorHandler ConditionMet;

        /// <summary>A delegate used to handle ConditionMonitor events.</summary>
        public delegate void ConditionMonitorExceptionHandler(ConditionMonitor<T> source, T state, Exception ex);
        /// <summary>An event which fires if an exception is thrown while retrieving the state
        /// or testing whether the condition is met.</summary>
        public event ConditionMonitorExceptionHandler RequestError;
        #endregion

        #region Public Properties
        /// <summary>The time between requests made to the RequestStateDelegate. Default is 1 second (1000ms)</summary>
        public double PollInterval_Milliseconds { get; set; }
        /// <summary>Set to true to automatically halt polling once the condition is met. Default is False.</summary>
        public bool HaltWhenConditionMet { get; set; }
        #endregion

        #region Constructors
        /// <summary>Creates a new instance of a ConditionMonitor</summary>
        public ConditionMonitor()
        {
            originThread = Dispatcher.CurrentDispatcher;
            PollInterval_Milliseconds = TimeSpan.FromSeconds(1).TotalMilliseconds;
            HaltWhenConditionMet = false;
        }
        #endregion

        #region Public Methods
        /// <summary>Begins polling the RequestStateDelegate on a seperate thread.</summary>
        public void BeginMonitoring()
        {
            if( RequestStateDelegate == null ) throw new Exception("No delegate specified for polling - please set the RequestStateDelegate property.");
            lock( multiThreadLock )
            {
                if( !Halted ) throw new Exception("Previous monitoring has not yet been stopped!");
                if( requestInProgress ) throw new Exception("The previous request never completed, which means the thread is still queued, or a delegate is taking a long time to complete.");
                Halted = false;
            }
            if( nextRequest != null ) nextRequest.Dispose();
            nextRequest = new Timer(PollInterval_Milliseconds) { AutoReset = false };
            nextRequest.Elapsed += new ElapsedEventHandler(nextRequest_Elapsed);
            nextRequest.Start();
        }

        /// <summary>Halts polling and ensures that no more requests will be made or events fired.</summary>
        public void StopMonitoring()
        {
            Halted = true;
            nextRequest.Stop();
            nextRequest.Dispose();
        }

        /// <summary>Halts the thread if it is still running so that the instance can be garbage collected.</summary>
        public void Dispose()
        {
            StopMonitoring();
        }
        #endregion

        #region Private Methods
        /// <summary>Timer elapsed handler.</summary>
        private void nextRequest_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
        {
            PollState();
        }

        /// <summary>Responsible for the polling loop and invoking events back on the origin thread.</summary>
        private void PollState()
        {
            T state = default(T);
            requestInProgress = true;
            try
            {
                if( Halted ) return;
                state = RequestStateDelegate();
                InvokeEvent(RequestReceived, state);
                if( IsConditionMetDelegate != null && !Halted )
                {
                    bool bConditionMet = false;
                    bConditionMet = IsConditionMetDelegate(state);
                    if( bConditionMet )
                    {
                        InvokeEvent(ConditionMet, state);
                        if( HaltWhenConditionMet ) Halted = true;
                    }
                }
            }
            catch( Exception ex )
            {
                InvokeExceptionHandler(state, ex);
            }
            finally
            {
                if( !Halted ) nextRequest.Start();
                requestInProgress = false;
            }
        }

        /// <summary>Invokes a delegate of type ConditionMonitorHandler on the origin thread.</summary>
        /// <param name="toInvoke">The delegate to invoke (RequestRecieved or ConditionMet)</param>
        /// <param name="state">The response from the last call to the RequestStateDelegate</param>
        private void InvokeEvent(ConditionMonitorHandler toInvoke, T state)
        {
            if( toInvoke != null && !Halted )
                originThread.BeginInvoke(toInvoke, new object[] { this, state });
        }
        /// <summary>Invokes the exception delegate on the origin thread.</summary>
        /// <param name="state">The response from the last call to the RequestStateDelegate, or null.</param>
        /// <param name="ex">The exception raised while calling the RequestStateDelegate or IsConditionMetDelegate.</param>
        private void InvokeExceptionHandler(T state, Exception ex)
        {
            if( RequestError != null && !Halted )
                originThread.BeginInvoke(RequestError, new object[] { this, state, ex });
        }
        #endregion
    }
}

Answer 1

Technical

1. I'd use Stop rather than Halt - somehow sounds more natural.

2. You should make more use of Action rather defining a bunch of delegate types. For instance this:

   public delegate T RequestState();
   public RequestState RequestStateDelegate { get; set; }
   

   Can be written as

   public Action<T> RequestState { get; set; }
   

   Similar for IsConditionMet and your various event handlers. (I'd also remove the Delegate suffix.)

3. PollInterval_Milliseconds should be a TimeSpan. Then you can remove the unit as part of the name and gives the caller more flexibility.

4. If you call BeginMonitoring in just the right moment again after it has been called before, you can have multiple PollState() calls going on. The right moment is that the timer just elapsed from the previous call and started to execute but hasn't set the flag yet. I don't know if you want to protect against it.
5. You will call Dispose twice on the timer. This should be harmless but you might add nextRequest = null to StopMonitoring.

6. This:

   nextRequest.Elapsed += new ElapsedEventHandler(nextRequest_Elapsed);
   

   Can be written as

   nextRequest.Elapsed += (s, e) => PollState();
   

   Then you can get rid of the nextRequest_Elapsed method.

7. I would rewrite this:

   bool bConditionMet = false;
   bConditionMet = IsConditionMetDelegate(state);
   if( bConditionMet )
   

   As:

   if (IsConditionMetDelegate(state))
   

   I don't really see any benefit into introducing the local variable.

8. Don't throw generic Exception. In your case I'd use InvalidOperationException. Using specific exceptions gives more meaning to the error and also potentially allows the caller to catch some exceptions but not others.

Design

As it stands your code is hard to unit test due to the async nature of the timer. Consider creating an ITimer interface which you can pass in with a thin wrapper implementation around the .NET Timer. This way you can pass in a mock implementation in unit tests which you can control as to when it executes. This is somewhat annoying but so far the only way to reliably test things like this I have found.