Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a followup to Polling loop to run in a background thread 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.

Have I correctly implemented Timer and ThreadPool.QueueUserWorkItem to achieve what I want to achieve? 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? 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
    }
}
share|improve this question
1  
You don't need to manually queue to the thread pool from the Elapsed handler. It already executes on the thread pool. – svick Jul 6 '12 at 14:24
Really? Wow thanks for pointing that out. It's not clear all all in the documentation until way down in the middle of the "Remarks" section: "If the SynchronizingObject property is null, the Elapsed event is raised on a ThreadPool thread." I'll edit that part right now. I'll get rid of QueueWorkItem entirely, I don't mind if the first poll doesn't happen until PollInterval after BeginMonitoring() is called. – Alain Jul 6 '12 at 14:35
1  
You should lock the entire initialization of the timer, and all changes to the Halted property. If StopMonitoring and BeginMonitoring are called simultaneously by two different threads, results will be unpredictable. – Groo Jul 13 '12 at 8:16
1  
That's very true, although my goal is to defend against normal use, not make it idiot proof. If anyone finds themselves in a situation where two different threads are trying to manage one polling class, they're doing something horribly horribly wrong. – Alain Jul 13 '12 at 12:46

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.