TBB Exception Safety

[mwhenson]

Hey, I have an interesting problem with trying to make tbb exception safe. My code sets up an execution tree similar to 'Two Mouths' example in the TBB book. Here is the psuedocode:

---

tbb::task_group_context context(tbb::task_group_context::isolated);
tbb::task*root = new (tbb::task::allocate_root(context))tbb::empty_task;

Allocate all tasks as child of the root

root->set_ref_count(number of children + 1);

foreach(starttask)
root->spawn(starttask)

//this is exception safe as expected as long as an exception has not already been thrown!
tbb::task::spawn_root_and_wait(*root);

---

This works as expected, unless an exception is thrown before spawn_root_and_wait() is called, then execution is never returned to me and tasks leak. Am I doing something completely wrong and/or is there a better way to do this? Thanks!

Mike

[Andrey_Marochko]

Hello, Mike

I'm not absolutely sure what is the exact reason of the behavior you observe, but your code definitely has one serios error. Instead of using

tbb::task::spawn_root_and_wait(*root);

you instead need to call

root->wait_for_all();

The problem with your variant is that spawn_root_and_wait immediately executes and then destroys the root task passed as its argument disregarding the fact whether it has any children or not. And this may (and usually does) result in children being processed in other (or even in this) thread referencing the freed memory of the root task or even attempting to execute its dead body repeatedly.

So try to call wait_for_all, and if problems remain, please post here a more detailed snapshot.

[mwhenson]

Sure, it's not my exact problem, but here's code nearly straight from the book. If I run wait_for_all, the program hangs. If I run spawn_root_and_wait, everything works as expected. I'm not sure exactly what is different about my problem, but I was running into this problem as well (hence the spawn_root_and_wait). Thanks!

Mike

class Combine1 : public tbb::task
{
public:
tbb::empty_task*filter_input_dummy,*transform_input_dummy;
tbb::task*execute()
{

return 0;
}
};

class Combine2 : public tbb::task
{
public:
tbb::empty_task*filter_input_dummy,*warp_input_dummy;
tbb::task*execute()
{

return 0;
}
};

class Filter : public tbb::task
{
public:
tbb::empty_task*out_dummy1,*out_dummy2;
tbb::task*execute()
{
spawn(*out_dummy1);
return out_dummy2;

}
};

class Transform : public tbb::task
{
public:
tbb::empty_task*out_dummy;
tbb::task*execute()
{
Sleep(10);
throw new exception;
return (out_dummy);
}
};

class Warp : public tbb::task
{
public:
tbb::empty_task*out_dummy;
tbb::task*execute()
{
return out_dummy;
}
};

void main()
{
tbb::task_scheduler_init init;

for(int j=0;j < 10;++j)
{
for(int i=0;i < 10;++i)
{

try
{
tbb::task_group_context*g;
tbb::empty_task*root = new (tbb::task::allocate_root(*g)) tbb::empty_task;
Combine1*combine1 = new (root->allocate_child()) Combine1;
Combine2*combine2 = new (root->allocate_child()) Combine2;

Filter*filterb = new (root->allocate_child()) Filter;
Transform*transform2 = new (root->allocate_child()) Transform;
Warp*warp1 = new (root->allocate_child()) Warp;

filter b->out_dummy2 = new (combine2->allocate_child()) tbb::empty_task;
filterb->out_dummy1 = new (combine1->allocate_child()) tbb::empty_task;

warp1->out_dummy = new (combine2->allocate_child()) tbb::empty_task;
transform2->out_dummy = new (combine1->allocate_child()) tbb::empty_task;

root->set_ref_count(6);

combine1->set_ref_count(2);
combine2->set_ref_count(2);

root->spawn(*filterb);
root->spawn(*transform2);
root->spawn(*warp1);

root->wait_for_all();
root->destroy(*root);
//tbb::task::spawn_root_and_wait(*root);
}
catch(...)
{
cout << "caught!" << endl;
}

}
}
}

[Andrey_Marochko]

Ok, let's see... First of all the lines

tbb::task_group_context*g;
tbb::empty_task*root = new (tbb::task::allocate_root(*g)) tbb::empty_task;

look very suspicious - you seem to be derefencing uninitialized pointer (is it the code you actually run?). Please create the context as an object, not as a pointer.

The reason why wait_for_all hangs in case of exception, is because of the technique you use to spawn the tasks. Look at the method

tbb::task* Transform::execute()
{
Sleep(10);
throw new exception;
return (out_dummy);
}

In case of exception out_dummy is not returned to the scheduler and thus the its ref count of its parent never gets decremented. Try to change it in the following way

tbb::task* Transform::execute()
{
Sleep(10);
spawn (*out_dummy);
throw new exception;
return NULL;
}

In general you should avoid situations where it is possible that after setting refcount for a task its children can be not spawned (e.g. because of the exception or cancellation).

[mwhenson]

Yeah I screwed up on the uninitialized pointer. Good call.

Thanks for the help so far. So, to be clear, if I made the changes below, you would still expect it to hang? Since the exception is canceling some tasks?

How would you design a solution to 'Two Mouths' problem in the book? I've been using preallocated empty dummy tasks as a way of synchronization as in the book. You've mentioned in other threads to prefer not to preallocate, and now your suggesting that this will also make it hard to handle exceptions. Currently I preallocate my whole execution tree that makes extensive use of these empty dummy tasks to synchronize. Thanks again for your help.

Mike

class Filter : public tbb::task
{
public:
tbb::empty_task*out_dummy1,*out_dummy2;
tbb::task*execute()
{
//force exception to be thrown in other task first
Sleep(10);
spawn(*out_dummy1);
return out_dummy2;

}
};

class Transform : public tbb::task
{
public:
tbb::empty_task*out_dummy;
tbb::task*execute()
{
spawn(*out_dummy);
throw new exception();
return 0;
}
};

[ARCH_R_Intel]

I think TBB has a gap here that needs to be filled. There is currently no built in support for automatic deletion of a task when it has not yet been spawned and an exception has occurred.

In the TBB internals src/tbb/task.cpp there is a class auto_empty_task that does exactly this kind of cleanup. We could generalize that to a template. But there is a better solution.

What TBB needs is a method

"void task::operator delete(void*)".

Then users can use the existing solutions for avoiding leaks of heap-allocated objects, namely std::auto_ptr<:TASK> (C++98) or a std::unique_ptr<:TASK> (C++0x).

Here's the diff for a quick hack I put together as proof of concept. As noted in the comment, it is unsafe to use if the thread that deletes the task is not the same thread that allocated the task. This is something we would fix if we adopt the general idea into TBB.

--- task.h (revision 4815)
+++ task.h (working copy)
@@ -360,6 +360,12 @@
 //! Destructor.
 virtual ~task() {}

+ void operator delete( void* p ) {
+ // Hack implementation for proof of concept. This implementation only works correctly
+ // if the thread doing the freeing is the same thread that allocated the task object.
+ internal::allocate_root_proxy::free(*static_cast(p));
+ }
+
 //! Should be overridden by derived classes.
 virtual task* execute() = 0;

Heres a quick example I concocted to test the notion:

#include 
#include 
#include "tbb/task.h"
#include "tbb/task_scheduler_init.h"

struct MyTask: public tbb::task {
 tbb::task* execute() {return NULL;}
 MyTask() {printf("Hi!
");}
 ~MyTask() {printf("Bye!
");}
};

void Foo() {
 std::auto_ptr<:TASK> g( new( tbb::task::allocate_root() ) MyTask );
 throw 0;
}

int main() {
 tbb::task_scheduler_init init;
 try {
 Foo();
 } catch(...) {
 printf("Exception caught.
");
 }
 return 0;
}

It prints:

Hi!
Bye!
Exception caught.

- Arch

[mwhenson]

I agree that there seems to be a gap somewhere, but I'm not sure how to solve it. The problem is, I don't think that solution is sufficient, at least in my problem domain. The issue is that I am preallocating a whole tree, then some task throws, but I don't know which tasks have run and which haven't. So I can't delete, because I don't know which ones are still valid. Right now I don't even keep an explicit list of all the tasks I allocate.

Mike

[Andrey_Marochko]

To Arch.

Yeah, such a delete method would be helpful, and we even considered it with Alexey Murashov just recently when he worked on a similar problem in tbb::pipeline. Unfortunately the problem is that this delete method should either decrement the parent's refcount if it has been incremented on behalf of this task, or do not touch it. Since in TBB usage model refcount management is disjoined with task allocation determining what exactly should be done for this particular task becomes an issue. And I do not have a ready solution at the moment.

BTW Alexey M. has implemented the smart pointer for tasks (using the usual destroy method) that can be used in the local contexts.

To Mike (and Arch)

I've discussed it with Alexey K. shortly, and it looks like your case is one of a few ones when separation of tasks allocation and spawning does not have an elegant alternative. Arch, have you ever thought about adding support for diamond shaped dependencies into TBB, does it look feasible?

As it is now, I could recommend to spawn the tasks that left unspawned because of cancellation from the destructors of the tasks that hold their pointers (their execute methods will not be invoked of course, but the scheduler will update their parent's refcounts appropriately).

The only gotcha here is that destructors of last tasks may still be pending in worker threads when wait_for_all exits in the root thread. So the checks (for whether we were cancelled) in the destructors should not use data that may be destroyed after exiting wait_for_all.

[mwhenson]

Thanks, I would have never thought of spawning in the destructor. I actually find it fairly elegant - more so than other threading packages at least. Support for diamond shape dependencies would be nice, I'll be on the lookout. Keep up the good work,

Mike

[ARCH_R_Intel]

You're right - my solution does not address the reference-counting issue. The smart-pointer solution may be the best we can do.

I've been considering adding more direct support for diamond patterns by providing a means of doing the equivalent of the empty task trick, but without bothering with the empty task.For instance, we could add the following two methods:

//! Atomically increment reference count
void task::increment_ref_count();
//! Atomically decrement reference count and spawn task if it became zero.
void task::decrement_ref_count_and_spawn_if_zero();

The first method would be used in situations where the total number of predecessors is not known in advance, like allocate_additional_child_of is currently used. The second method would be avoid the need to createand spawn adummy child.

More direct support, like building in the ability for multiple parents of a task, has always seemed to impose implementation costs on everyone, even users of the dominate single-parent case. But with the additional methods mentioned, I think users would be able to simulate having multiple parents quite cheaply.

[mwhenson]

If you're looking for feedback - I like that solution. Multiple parents sounds like it would complicate things...

[ARCH_R_Intel]

Do you want to try it as an experiment? It might be useful to find out if there are any "gotchas". I could an experimental version as a diff to the TBB header.

- Arch