Please review my UNC path exists monitor

Question

This is my first attempt at trying to write a "monitor" class to determine if a UNC path is available. I need the monitor to not block the main thread and to also event when the UNC is toggled UP/DOWN so that I can respond to that event.

I'm looking for:

1. Best practice suggestions

2. Am I doing this properly? Is there a more efficient way?

3. Will there be threads left alive? will I eat into the pool until it starves?

4. Other comments suggestions related to this code or programming context

EDIT 1

Required .NET 4.0

EDIT 2 updated code to implement suggestions

EDIT 3 updated code to include error handling as I understand it to be

CODE

public class NetworkPathMonitor : INetworkPathMonitor
{
    public event EventHandler<PathAvailabilityChangedArgs> PathAvailabilityChanged;
    public event EventHandler Started;
    public event EventHandler Stopping;
    public event EventHandler Stopped;
    public event EventHandler CheckingStatus;
    public event EventHandler Error;

    protected virtual void OnError(object sender, EventArgs e)
    {
        EventHandler handler = Error;
        if (handler != null)
            handler(sender, e);
    }
    protected virtual void OnStopping(object sender, EventArgs e)
    {
        EventHandler handler = Stopping;
        if (handler != null)
            handler(sender, e);
    }
    protected virtual void OnCheckingStatus(object sender, EventArgs e)
    {
        EventHandler handler = CheckingStatus;
        if (handler != null)
            handler(sender, e);
    }
    protected virtual void OnStopped(object sender, EventArgs e)
    {
        EventHandler handler = Stopped;
        if (handler != null)
            handler(sender, e);
    }
    protected virtual void OnStarted(object sender, EventArgs e)
    {
        EventHandler handler = Started;
        if (handler != null)
            handler(sender, e);
    }
    protected virtual void OnPathAvailabilityChanged(object sender, PathAvailabilityChangedArgs e)
    {
        EventHandler<PathAvailabilityChangedArgs> handler = PathAvailabilityChanged;
        if (handler != null)
            handler(sender, e);
    }

    private PathState CurrentState { get; set; }

    private CancellationTokenSource _tokenSource;

    public string PathToMonitor { get; set; }

    public TimeSpan Interval { get; set; }

    public NetworkPathMonitor(string pathToMonitor, TimeSpan interval, PathState expectedState = PathState.Up)
    {
        this.CurrentState = expectedState;
        this.Interval = interval;
        this.PathToMonitor = pathToMonitor;
    }

    public void Stop()
    {
        OnStopping(this, EventArgs.Empty);
        _tokenSource.Cancel();
    }

    public void Start()
    {
        _tokenSource = new CancellationTokenSource();

        TaskScheduler.UnobservedTaskException += TaskScheduler_UnobservedTaskException;
        var t = Task.Factory.StartNew(() =>
         {
             while (!_tokenSource.Token.WaitHandle.WaitOne(Interval))
             {
                 if (_tokenSource.IsCancellationRequested)
                 {
                     _tokenSource.Dispose();
                     break;
                 }
                 OnCheckingStatus(this, EventArgs.Empty);
                 CheckPathStatus();
             }
         }, _tokenSource.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default)
         .ContinueWith((tsk) =>
         {
             //TODO: Offer the error handler more info about the error
             OnError(this, EventArgs.Empty);
         }, TaskContinuationOptions.OnlyOnFaulted)
         .ContinueWith((tsk) =>
         {
             OnStopped(this, EventArgs.Empty);
         }, TaskContinuationOptions.OnlyOnCanceled);

    }

    void TaskScheduler_UnobservedTaskException(object sender, UnobservedTaskExceptionEventArgs e)
    {
        //TODO: call error handler and actually pass info about the error
        e.SetObserved();
    }


    private void CheckPathStatus()
    {
        //this is synchronous. We just have to wait for it to timeout.
        var currentState = Directory.Exists(PathToMonitor) ? PathState.Up : PathState.Down;

        var shouldEvent = currentState != CurrentState;
        CurrentState = currentState;

        if (shouldEvent)
        {
            OnPathAvailabilityChanged(this, new PathAvailabilityChangedArgs() { Path = PathToMonitor, PathState = CurrentState });
        }
    }
}


public enum PathState
{
    Up,
    Down
}
public class PathAvailabilityChangedArgs : EventArgs
{
    public string Path { get; set; }
    public PathState PathState { get; set; }
    public PathAvailabilityChangedArgs() { }
    public PathAvailabilityChangedArgs(string path)
    {
        Path = path;
    }
}

Answer 1

The implementation you have will forever consume one thread from the ThreadPool. That's not the end of the world, but not ideal either. An easy solution would be to use an overload of Task.Factory.StartNew that accepts a TaskCreationOptions argument, and specify TaskCreationOptions.LongRunning. That will let the system handle it appropriately. Another option would be starting a new thread yourself.

In the task delegate (the code inside StartNew()) I would put a global try/catch block. Any exceptions that might be thrown in there will cause your process to be killed. At the very least you'd want to log those somewhere just so you have some idea what's going on.

I think you're going to Dispose() the IsStopping event on the first time through the loop, then try to use it on the second time through, which will probably cause an exception. I would just remove the AutoResetEvent entirely and use CancellationTokenSource instead. You can use TokenSource.Token.WaitHandle.WaitOne() to wait on instead of the AutoResetEvent. This also has the benefit that when Stop() is called, any pending wait will end immediately.

I would make the Interval property a TimeSpan instead of an int. It's just more clear to users what it means - they don't have to wonder whether it's seconds, milliseconds, or what.

Answer 2

Just to add to breishl's answer:

• don't use private properties, just use fields instead (private CancellationTokenSource TokenSource { get; set; } => private CancellationTokenSource _tokenSource;)
• AutoResetEvent is not needed as already noted, but even now it's used incorrectly, you should have used ManualResetEvent instead (again, not needed here)
• you can also wait (await) with cancellationToken using Task.Delay(TimeSpan, CancellationToken) method.