Determining if a connection has been made to a communications device

Question

I am unsure if my use of Monitor.Wait and Monitor.Pulse is correct. It seems to work alright, but there is a nagging doubt I am doing something wrong.

Am I dealing with a timeout OK, or is there a better way to do it?

All examples I have seen use Monitor.Wait with a while loop, and not with an if to re-check the blocking condition after Pulse, but I need to determine if a timeout occurred.

_cmdDispatcher.EnqueueCommand(cmd) sends a command to the device, and it responds with an event that executes the CommResponseReceived method on another thread.

private readonly object _connectLock = new object();
public bool Connect()
{
 if (this._connected) throw new InvalidOperationException("Plc is already connected.");
 ICommand cmd = new Command(MessageIdentifier.Name);
 try
 {
 if (this._channel.Open() && !this._connected)
 {
 // Wait for communications to be fully established or timeout before continuing.
 lock (this._connectLock)
 {
 this._cmdDispatcher.EnqueueCommand(cmd);
 if (!this._connectSignal)
 {
 if (Monitor.Wait(this._connectLock, this._timeout))
 {
 this._connected = true;
 this._connectSignal = false;
 Debug.WriteLine("Connection succeeded.");
 }
 else
 {
 //TODO Log timeout.
 Debug.WriteLine("Connection timed out.");
 }
 }
 }
 if (this._connected) this.OnConnectionChangedEvent(ConnectionStatus.Connected);
 }
 }
 catch (Exception ex)
 {
 //TODO Log errors.
 Debug.WriteLine("Connection failed.");
 }
 return this._connected;
}

Executed in another thread when the device responds:

private void CommResponseReceived(object sender, CommsResponseEventArgs e)
{
 //
 // Signal that communications are successfully established.
 lock (this._connectLock)
 {
 this._connectSignal = true;
 Monitor.Pulse(this._connectLock);
 }
}

• Pat: Just skimmed your question, but thought you might be interested in albahari.com/threading/part4.aspx#_WaitPulseVsWaitHandles. Joe Albahari is the creator of LINQPad and co-author of C# In A Nutshell. Basically, he's a genius.
• Andy: @Pat: I have indeed seen that site, and it is excellent. With regard to a Monitor.Wait with timeout, it only lightly touches on that subject and doesn't have an example. Otherwise it is a great resource.

Answer 1

I agree with Alex that ManualResetEvent may be a little cleaner simply because you don't need to perform manual signal tracking to ensure you don't wait due to the Monitor already pulsing but, as I understand it, WaitHandle (used by ManualResetEvent) has more overhead. It's also quite possible to use memory barriers and Application.DoEvents/Thread.Sleep in a time-limited loop to achieve a lockless solution, but none of this matters unless you have very strict resource/performance concerns.

Regardless, there's always more than one way to skin a cat and your usage is indeed safe and correct.

If the Connect method is running on your UI thread be sure your timeout isn't really long or windows may think your application is not responding because blocking in Monitor.Wait does not resume the window's message pumping; if you need a long timeout or it is not running on the UI thread you will need to ensure the Connect method cannot be called again before the last has finished (if this._channel.Open() returns false if it is already open that should suffice). In the case of it being on the UI thread, besides preventing double execution, you would want to switch to a small timeout in a time-limited loop that calls Application.DoEvents(). You may already be aware of these concerns but I'm having to make a lot of assumptions to review the code. While I'm at it, it's probably trivial, but you can also move you Command instantiation to just before the lock to save yourself unnecessary instance creation in some cases.

• Andy: Your asumptions are pretty much spot on. The code is not running on the UI thread, so those concerns are not applicable here. Good call with regard to the Command instantiation. Thank you for taking a look.

Answer 2

I believe you are using them correctly as-is, but if I was working on your team, I would deny you a commit on basic principal:

Monitor.Wait and Monitor.Pulse are amazingly low-level constructs with very difficult semantics to try to figure out after you've written them. Assuming this is used in a large application, I'm going to have to search for every possible reference to the object contained by _connectLock and see how its used, just to make sure that the only thing that Pulses it is the connection code.

It doesn't look like you're doing anything here that you couldn't just use System.Threading.ManualResetEvent for instead, which has a much simpler API for your application's future maintenance developers to figure out.

---

P.S. Remember that your application's future maintenance developer might be you. Also remember that in the future you will be older, and thus crankier and more forgetful than you are now.

• Andy: @Alex: My team consists of just me, and I'm already at the older and crankier stage :) Seriously though, thanks for the answer. Good suggestion about the ManualResetEvent and I will consider that. However, wouldn't you still have to search for every reference that calls .Set, in the same way you would search for .Pulse?
• Alex Lyman: Still have to search, yes--but, in my experience, MRE's tend to exude a better sense of single-use-only-ness (in your example, the MRE would only be used to signal connectedness, nothing else). Objects seem to creep in and gather extra uses: maybe you'll add another flag called _disconnectSignal, and rather than adding a second object to Pulse/Wait on, you could just use the one thats already there...
• TheXenocide: @Alex: there are situations for both or only one would exist so I'm hoping that's not a blank-check commit-denial. It's just an assumption, but the "_" in "_connectLock" makes me think it's a private field so you would only have to search the one class. If people are just grabbing up existing sync objects just because they need one it sounds to me like you've got a different problem entirely. It's rare (in my experience) that an instance of System.Object finds other uses or reason to later be publicly exposed.
• Andy: @TheXenocide: _connectLock is indeed a private class member.
• Alex Lyman: @TheXenocide: I don't believe in a blank-check commit-denial, only gut reaction ones: if I see something I disagree with, I always reject. If the coder can back up his choice with a sound argument that their way has an advantage that I have overlooked, then I'm perfectly willing to green light. That said, I have found very few legitimate cases where Monitor.Pulse would provide a clear-cut advantage over MRE's.

Answer 3

I'd just add on more thing on top of the input provided by Alex and TheXenocide.

You're relying quite a lot on the this._connected state. It's probably a shared state within that class so could change mid-way if several threads are trying to open the connection at once. Thus, I'd use something like Thread.VolatileRead, Thread.VolatileWrite outside the critical section.