Timer to poll a Delegate

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:

-
Infinitely running threads

-
Race Conditions

-
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
{
/// 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.
/// The type of the value returned by the polling delegate.
public class ConditionMonitor : 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 threa

Technical

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

-
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 RequestState { get; set; }

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

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

• 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.

• You will call Dispose twice on the timer. This should be harmless but you might add nextRequest = null to StopMonitoring.

-
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.

-
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.

• 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.

StackExchange Code Review Q#13393, answer score: 8