Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dangerous lock in QCallbackConnection #10

Open
Kryptos-FR opened this issue May 25, 2018 · 0 comments
Open

Dangerous lock in QCallbackConnection #10

Kryptos-FR opened this issue May 25, 2018 · 0 comments

Comments

@Kryptos-FR
Copy link

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant