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);
    }
}

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.

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.

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.