Dangerous lock in QCallbackConnection

[Kryptos-FR]

In class QCallbackConnection a lock is performed on the instance object (this). This is usually considered a defect as it can leads to deadlock in some cases.

From Microsoft's documentation:

In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

• lock (this) is a problem if the instance can be accessed publicly.
• lock (typeof (MyType)) is a problem if MyType is publicly accessible.
• lock("myLock") is a problem because any other code in the process using the same string, will share the same lock.

Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances.

Since the StartListener() and StopListener() methods are virtual, the best way would be to have a private object to hold the lock, make both methods non-virtual and provide two overridable protected methods, such as:

public void StartListener()
{
    lock (_syncRoot)
    {
        StartListenerInternal();
    }
}

public void StopListener()
{
    lock (_syncRoot)
    {
        StopListenerInternal();
    }
}

protected virtual void StartListenerInternal()
{
    if (_listener != null) return;
    _listener = new QListener(this);
    _listenerThread = new Thread(_listener.Run);
    _listenerThread.Start();
}

protected virtual void StopListenerInternal()
{
    if (_listener == null) return;
    _listener.Running = false;
    _listenerThread.Join(500);
    _listener = null;
}

References:

• https://docs.microsoft.com/en-gb/dotnet/csharp/language-reference/keywords/lock-statement
• https://docs.microsoft.com/en-gb/visualstudio/code-quality/ca2002-do-not-lock-on-objects-with-weak-identity