Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
add comment

2 Answers

up vote 3 down vote accepted

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.

share|improve this answer
    
Very good feeback. It's actually not as bad as I thought it would be. All of your suggestions are valid. I will attempt to make these changes and post a revision when complete. Additionally, if there is no more feedback afterwards, I will mark this as the answer. –  Hardrada Dec 22 '12 at 16:47
    
Just a note about the disposing of the AutoEvent: That only happened upon cancellation (token.Cancel()). When the call to cancel was made, the loop would only then check the cancellation status and dispose. The code as entered in my OP didn't throw any errors during testing, however, that's not to say that it is absolute. I just can't say for sure due to lack of confidence. –  Hardrada Dec 22 '12 at 17:37
    
I have updated the code per the suggestions. However, I am uncertain how to handle exceptions from within the Task thread. I read albahari.com/threading/… and thought I had applied it properly, but it doesn't appear to bubble up. –  Hardrada Dec 22 '12 at 20:27
    
Since this is a simple case I would probably throw a try/catch block inside the thread method, rather than using continuations. But the way you've done it looks OK to me. Have you tried intentionally causing an exception and following it in the debugger? –  breischl Dec 23 '12 at 16:46
    
I have done an intentional throw and it never popped in the debugger. Everything looks and runs as expected. Thank you for your time, effort and expertise. I've marked your reply as the answer. –  Hardrada Dec 23 '12 at 18:22
add comment

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.
share|improve this answer
    
I updated my comment to include a formerly missing requirement of .NET 4.0. I have taken note of the incorrect usage of AutoResetEvent and will research more for my own knowledge; however, it seems it's somewhat obsolete now with the existence of Tasks? Can you confirm that? –  Hardrada Dec 22 '12 at 19:53
1  
I wouldn't say that ManualResetEvent and AutoResetEvent are already obsolete. That's true that most of day-to-day work is covered with new async API (Tasks in .NET 4.0 and async compiler support in .NET 4.5), but there are cases when you need to synchronise work done in different threads. It's like the main use case of Monitor class is covered with lock keyword, but there may be cases when you need features like Monitor.Wait and Monitor.Pulse –  almaz Dec 22 '12 at 21:49
    
Thanks for the clarification. –  Hardrada Dec 22 '12 at 22:25
add comment

Your Answer

 
discard

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

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