HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

UNC path exists monitor

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
monitorpathuncexists

Problem

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:

  • Best practice suggestions



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



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



  • Other comments suggestions related to this code or programming context



```
public class NetworkPathMonitor
{
public event EventHandler PathAvailabilityChanged;
public event EventHandler Started;
public event EventHandler Stopping;
public event EventHandler Stopped;
public event EventHandler CheckingStatus;

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 handler = PathAvailabilityChanged;
if (handler != null)
handler(sender, e);
}

private PathState CurrentState { get; set; }

private CancellationTokenSource TokenSource { get; set; }

private AutoResetEvent IsStopping { get; set; }

public string PathToMonitor { get; set; }

public int Interval { get; set; }

public NetworkPathMonitor

Solution

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.

Context

StackExchange Code Review Q#19868, answer score: 3

Revisions (0)

No revisions yet.