Determining if a connection has been made to a communications device

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

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.

StackExchange Code Review Q#185, answer score: 8