Events + Locks = Problems

TLDR; Never raise events from synchronized context.

Let’s imagine that you are creating some middle layer between your forms and business layer (logic). In order not to allow races and data corruption you introduce synchronized context. Obviously you won’t be synchronizing on this. So this is our first implementation:

class ThreadSafe {
    private Object m_lock = new Object();

    public void Foo() { lock(m_lock) { ... } }
}

Everything seems OK, but being event-lovers as we are we want to raise them in order to tell other components that our state changed. From now on I will mostly add only new parts of the code to reduce the clutter.

class ThreadSafe {
    public event EventHandler DoneFoo;

    // As only the DoneFoo enclosing class can call it we want to allow subclasses to call their base class events.
    protected virtual void OnFoo(EventArgs e) {
        if (DoneFoo != null) DoneFoo(this, e);
    }
}

Now we can finally hook the event to our Foo() method.

class ThreadSafe {
    public void Foo() {
        lock(m_lock) { ... DoneFoo(EventArgs.Empty); }
    }
}

And … boom! You’ve got a deadlock, sir. Why? Imagine that you have an event listener class called ListenOnFoo that operates on GUI elements.

class ListenOnFoo {
    private ThreadSafe m_ts;

    // Register event handler
    public ListenOnFoo(ThreadSafe ts) { ts.DoneFoo += OnDoneFoo; m_ts = ts; }

    // Remember that we have to adhere to the EventHandler delegate declaration.
    private void OnDoneFoo(Object sender, EventArgs e) {
        DoInternalLogic();
    }

    // As we are dealing with GUI we have to make sure we call the logic via [Begin]Invoke() in order not to corrupt the GUI.
    private void DoInternalLogic() {
        if (this.InvokeRequired) {
            this.Invoke(new MethodInvoker(DoInternalLogic));
        }

        m_threadSafe.Foo(); // DEADLOCK!
    }
}

Obviously, DoInternalLogic introduces infinite loop problem, but let’s skip this for the moment. As you probably see, we are locking m_lock with the first thread and the second one (GUI). We have got a deadlock.

The problem with such kind of problems is they are very difficult to track and in case of complex GUI logic it could take us much time to find the issue. Because of that, it’s important not to make such mistakes in the first place.

First, the general rule of thumb is not to use Control.Invoke() and instead use its younger sister Control.BeginInvoke(). The difference between these two (as you can probably guess from the popular naming convention) is that the latter calls the delegate asynchronously, allowing the first thread (in our case) to finish execution and unlock the m_lock.

The issue with BeginInvoke() is that it won’t return the result right away (as it is non-blocking). If you happen to have invokable methods with non-void return (or when you do not want to proceed before they successfully modify the inner object state), then consider handling IAsyncResult that is returned from BeginInvoke().

I won’t discuss ways of dealing with asynchronous programs as this is a very broad topic (decently described on MSDN). What I will say for now, is that you should think about logic of your code and find a place where you can block (definitely not in GUI thread!) – while waiting on AsyncResult.

Second general rule of thumb is to keep synchronized regions as small and simple as possible. In our case, the simplest solution would be to move OnFoo() call from the lock.

class ThreadSafe {
    public void Foo() {
        lock(m_lock) { ... }
        DoneFoo(EventArgs.Empty); // Not in lock anymore
    }
}

That was easy, but I would rather go even further and disallow calling methods in locked regions. I know it sounds crazy but if you allow code like the following, you might have serious deadlock issue in future.

void Bar() {
    lock(m_lock) {
        Baz();
        Bay();
        Bax();
    }
}

You won’t know if any of these methods raises an event or invokes a delegate. Obviously with enough care and comments you can safely use methods inside synchronized regions but, please, KISS!

Advertisements

Tags: , , , ,

2 Responses to “Events + Locks = Problems”

  1. mortoray Says:

    The general rule of not doing any events, signals, callbacks, dispatching, or other kind of messaging with a lock held is very good. Basically, any time the control flow leaves the immediate class to external code you should not be holding locks.

    • red1939 Says:

      Yup, this is a general rule. I just wanted to give people a warning if they ever start running into concurrency problems in non-trivial GUI applications.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s


%d bloggers like this: