wait_for_all(), spawn(), and a deadlock

[trevorthomson]

Hi,

We're having a deadlocking problem and I wonder if our usage of wait_for_all() and spawn() is causing the problem.

Basically, a task that is waiting for all its children never wakes up even though all the children (which are empty_tasks) have been spawned.

I believe that the spawned children are never executed and I'm wondering if the way we're spawning the children is the problem. If myObj is being initialized by task A, then a task (task B) that needs to wait for A creates a child, adds it to a list and calls wait_for_all(). Once A has finished, it loops over that list and spawns the children of B.

At first I figured there was a problem with another thread spawning B's children (since the docs say the owning thread must do the spawn), but neither mechanism of calling spawn() (see the code snippet below) triggered any warnings from the debug versions of the tbb libraries. Is it really valid to do the spawning as shown below?

I'm using tbb 2.2.13.

Thanks!

void myObj::init (myInfo & info)
{
if (initialized_) {
//
// fill info
//
return;
}

mutextype::scoped_lock lock (initMutex_);
if (initialized_) {
lock.release ();
//
// fill info
//
return;
}
if (initializing_) {
tbb::task& task = tbb::task::self ();
task.set_ref_count (2);
tbb::empty_task * c = new (task.allocate_child ()) tbb::empty_task;
waiters_.push_back (c); // waiters_ is a member of myObj
lock.release ();
task.wait_for_all ();
//
// fill info
//
return;
}

initializing_ = true;
lock.release ();

//
// perform long initialization
// and fill info
//

lock.acquire (initMutex_);
initialized_ = true;
initializing_ = false;
lock.release ();
for (wsize_type i = 0; i < waiters_.size (); ++i) {
tbb::task& c = *waiters_;
c.parent ()->spawn (c);
// tbb::task::self ().spawn (c);
}
waiters_.clear ();
}

[RafSchietekat]

How about just destroy()'ing c instead? That just might do the trick, but I leave any other comments to others (got to run).

[trevorthomson]

I haven't tried that--the docs say the same thing (the calling thread must own 'this') and destroy() seems more dangerous than calling spawn() from a non-owning thread. I'll try it out just to see...

[Alexey-Kukanov]

> We're having a deadlocking problem and I wonder if our usage of wait_for_all() and spawn() is causing the problem.

Yes it is. You should first spawn child tasks, and only then wait for their completion; and you do it in the wrong order.

Also I would recommend to allocate a special task for waiting; I have never seen (and never tried :)) using task::self() the way you do, and off the top of my head I can't say whether it will work as expected. Anyway, a recommended pattern for that is the following:

tbb::empty_task * t = new (tbb::task::allocate_root()) tbb::empty_task;
t->set_ref_count(2);
// allocate and spawn a child of t
tbb::empty_task * c = new (t.allocate_child()) tbb::empty_task;
t.spawn(c);
// do something else
// ...
t->wait_for_all();

And with TBB 2.2, we recommend even a simpler pattern to be used in most cases:

tbb::task_group tg;
tg.run( /*specify a function object here*/ );
// do something else
// ...
tg.wait();

[RafSchietekat]

Ah, I just had a quick look before and it didn't occur to me that this is actually one function with the wait_for_all() before the spawn()... perhaps because that wouldn't work. :-)

"tbb::empty_task * c = new (t.allocate_child()) tbb::empty_task; t.spawn(c);"
Er, spawning an empty task right after it was created isn't going to be terribly exciting...

[Alexey-Kukanov]

> Er, spawning an empty task right after it was created isn't going to be terribly exciting...

Right, but I just used the same code as the author of the topic :)

[RafSchietekat]

"Right, but I just used the same code as the author of the topic :)"

I don't want to study the code further until the author has revised it in the light of what you found, because I don't like the possible double-checked locking (which has been shown not to be reliable unless, e.g., the variable is atomic) and the number of explicit release() calls (code seems so much more accessible and maintainable with the use of locked block scopes instead), and the weird initialized_/initializing_ pair of variables, but I did think I recognised that the intent was to delay spawning as a task-oriented equivalent of waiting. My suggestion was to destroy() instead of spawn() (cheaper and with immediate effect), but of course even if the built-in deadlock is corrected there is still be an issue of required concurrency where TBB is all about optional parallelism instead, so the empty_task should probably be destroy()'ed from a user thread rather than another task.

(Added) Sounds a bit stern, but I only meant that it doesn't seem very useful to guess at this point.

[trevorthomson]

Thanks for all the input everyone. Indeed, the intent was to "delay spawning as a task-oriented equivalent of waiting", but it seems like we need to rethink this initialization problem and see if we can retool it with task groups or continuations.

[trevorthomson]

So, the general question is this:

With tbb, what's the best way to have task A wait for a resource that's in use by task B, and then be woken up when task B is finished? Similar to a mutex but task-based so task A's thread can go do something else...

[RafSchietekat]

If you're waiting for another task rather than an independent thread (let alone for itself, which is bound to fail), my intuition suggests you should probably be thinking "continuation" to avoid scheduler-related deadlocks.