Boost C++ Libraries: Ticket #5727: race condition between ~basic_condition_variable() and notify_all()

viboes — Fri, 02 Dec 2011 23:54:02 GMT

I'm surely misunderstanding your code

void foo() {
    condition_variable notifier;
    mutex m, m2;
    mutex::scoped_lock lock(m); //1
    bool done=false;
    thread t([&]{
        mutex::scoped_lock lock(m); //2
        done = true;
        lock.unlock();
        notifier.notify_all();
    });
    while(!done) notifier.wait(lock); //3
}

Doesn't this code deadlock. In 1 foo owns the lock m. In 2 the lambda thread is blocked until the lock variable exits his scope (at the end of foo). In 3 As done is false thread foo waits for notifier, which can not be notified by the lambda as blocked.

What am I missing?

owner, status changed; cc set

viboes — Fri, 02 Dec 2011 23:54:19 GMT

cc viboes added
owner changed from Anthony Williams to viboes
status new → assigned

sbear.powell@… — Sat, 03 Dec 2011 02:16:52 GMT

That's mostly correct, but it won't deadlock because notifier.wait(lock) will unlock foo's lock while it's waiting.

So: In 1, foo owns m. In 2, the lamda thread will block until m is available. In 3, done is false, so notifier.wait(lock) is called--releasing ownership of m. The lambda proceeds, setting done to true, unlocking m, and notifying any waiting threads (ie. foo). foo returns from its call to notifier.wait(), checks done (which is now true) and can return.

(If you want experimental proof that it doesn't deadlock, try running my workaround code... the logic is pretty much the same)

Replying to viboes:

I'm surely misunderstanding your code
void foo() {
    condition_variable notifier;
    mutex m, m2;
    mutex::scoped_lock lock(m); //1
    bool done=false;
    thread t([&]{
        mutex::scoped_lock lock(m); //2
        done = true;
        lock.unlock();
        notifier.notify_all();
    });
    while(!done) notifier.wait(lock); //3
}
Doesn't this code deadlock. In 1 foo owns the lock m. In 2 the lambda thread is blocked until the lock variable exits his scope (at the end of foo). In 3 As done is false thread foo waits for notifier, which can not be notified by the lambda as blocked.

What am I missing?

viboes — Wed, 07 Dec 2011 01:04:10 GMT

Replying to sbear.powell@…:

That's mostly correct, but it won't deadlock because notifier.wait(lock) will unlock foo's lock while it's waiting.

So: In 1, foo owns m. In 2, the lamda thread will block until m is available. In 3, done is false, so notifier.wait(lock) is called--releasing ownership of m. The lambda proceeds, setting done to true, unlocking m, and notifying any waiting threads (ie. foo). foo returns from its call to notifier.wait(), checks done (which is now true) and can return.

You are right. I forget always that wait unlocks the lock.

(If you want experimental proof that it doesn't deadlock, try running my workaround code... the logic is pretty much the same)

Replying to viboes:
I'm surely misunderstanding your code
void foo() {
    condition_variable notifier;
    mutex m, m2;
    mutex::scoped_lock lock(m); //1
    bool done=false;
    thread t([&]{
        mutex::scoped_lock lock(m); //2
        done = true;
        lock.unlock();
        notifier.notify_all();
    });
    while(!done) notifier.wait(lock); //3
}
Doesn't this code deadlock. In 1 foo owns the lock m. In 2 the lambda thread is blocked until the lock variable exits his scope (at the end of foo). In 3 As done is false thread foo waits for notifier, which can not be notified by the lambda as blocked.

What am I missing?

viboes — Wed, 07 Dec 2011 01:08:33 GMT

I think that Boost.Thread can not do any think here. You are sharing the variable notifier allocated on the stack between two thread. When the function foo returns, the lambda thread will continue using a destroyed instance.

Note that the same issue will occur with any shared variable. IMO, it is up to the application to manage with this kind of errors.

Let me know if you share my point of view.

sbear.powell@… — Wed, 07 Dec 2011 20:32:02 GMT

Replying to viboes:

I think that Boost.Thread can not do any think here. You are sharing the variable notifier allocated on the stack between two thread. When the function foo returns, the lambda thread will continue using a destroyed instance.

The only potential fix I can think of on Boost.Thread's side would be to lock the condition_variable's internal mutex in its destructor--that way nothing will be destroyed until after notify_all() is out of its critical section.

Note that the same issue will occur with any shared variable. IMO, it is up to the application to manage with this kind of errors.

Right. Is there any general rule of thumb on thread-safety of destructors? What do the other Boost.Thread classes do in regards to that?

Let me know if you share my point of view.

Yeah, after some thought I agree with you--it's not really a bug in Boost.Thread, unless the rest of the Boost.Thread library keeps destructors thread-safe. In the end, it's not so hard to add the second mutex, or to just stick t.join() before foo() returns.

viboes — Wed, 07 Dec 2011 21:31:56 GMT

Replying to sbear.powell@…:

Replying to viboes:

I think that Boost.Thread can not do any think here. You are sharing the variable notifier allocated on the stack between two thread. When the function foo returns, the lambda thread will continue using a destroyed instance.

The only potential fix I can think of on Boost.Thread's side would be to lock the condition_variable's internal mutex in its destructor--that way nothing will be destroyed until after notify_all() is out of its critical section.

Maybe I'm wrong, but I think this is out of the philosophy "don't pay for what you don't use".

Note that the same issue will occur with any shared variable. IMO, it is up to the application to manage with this kind of errors.

Right. Is there any general rule of thumb on thread-safety of destructors? What do the other Boost.Thread classes do in regards to that?

AFAIK no destructor takes care of these kind of thread-safety. If you want you can start a thread on the ML so others could give their advice.

Let me know if you share my point of view.

Yeah, after some thought I agree with you--it's not really a bug in Boost.Thread, unless the rest of the Boost.Thread library keeps destructors thread-safe. In the end, it's not so hard to add the second mutex, or to just stick t.join() before foo() returns.

Yes the user has everything to prevent the deadlock.

status changed; resolution set

viboes — Wed, 07 Dec 2011 21:32:21 GMT

status assigned → closed
resolution → invalid