diff --git a/include/fc/thread/future.hpp b/include/fc/thread/future.hpp index 0adc69a..40dd62f 100644 --- a/include/fc/thread/future.hpp +++ b/include/fc/thread/future.hpp @@ -72,10 +72,6 @@ namespace fc { void set_exception( const fc::exception_ptr& e ); - // HERE BE DRAGONS - void retain(); - void release(); - protected: promise_base(const char* desc FC_TASK_NAME_DEFAULT_ARG); @@ -108,8 +104,6 @@ namespace fc { #endif const char* _desc; detail::completion_handler* _compl; - std::shared_ptr _self; - boost::atomic _retain_count; }; template diff --git a/include/fc/thread/task.hpp b/include/fc/thread/task.hpp index a3162da..e29cd14 100644 --- a/include/fc/thread/task.hpp +++ b/include/fc/thread/task.hpp @@ -33,6 +33,25 @@ namespace fc { virtual void cancel(const char* reason FC_CANCELATION_REASON_DEFAULT_ARG) override; ~task_base(); + /* HERE BE DRAGONS + * + * Tasks are handled by an fc::thread . To avoid concurrency issues, fc::thread keeps a reference to tha + * task in the form of a simple pointer. + * At the same time, a task is also a promise that will be fulfilled with the task result, so typically the + * creator of the task also keeps a reference to the task (but not necessarily always). + * + * Because effectively neither fc::thread nor the task creator are responsible for releasing resources + * associated with a task, and neither can delete the task without knowing if the other still needs it, + * the task object is managed by a shared_ptr. + * However, fc::thread doesn't hold a shared_ptr but a native pointer. To work around this, the task can + * be made to contain a shared_ptr holding itself (by calling retain()), which happens before the task + * is handed to an fc::thread, e. g. in fc::async(). Once the thread has processed the task, it calls + * release() which deletes the self-referencing shared_ptr and deletes the task object if it's no longer + * in use anywhere. + */ + void retain(); + void release(); + protected: /// Task priority looks like unsupported feature. uint64_t _posted_num; @@ -65,6 +84,9 @@ namespace fc { void run_impl(); void cleanup_task_specific_data(); + private: + std::shared_ptr _self; + boost::atomic _retain_count; }; namespace detail { diff --git a/src/thread/future.cpp b/src/thread/future.cpp index d1c9a94..c9dbffa 100644 --- a/src/thread/future.cpp +++ b/src/thread/future.cpp @@ -19,8 +19,7 @@ namespace fc { _cancellation_reason(nullptr), #endif _desc(desc), - _compl(nullptr), - _retain_count(0) + _compl(nullptr) { } const char* promise_base::get_desc()const{ @@ -151,13 +150,5 @@ namespace fc { _compl = c; } } - void promise_base::retain() { - if( _retain_count.fetch_add(1, boost::memory_order_relaxed) == 0 ) - _self = shared_from_this(); - } - void promise_base::release() { - if( _retain_count.fetch_sub(1, boost::memory_order_release) == 1 ) - _self.reset(); - } } diff --git a/src/thread/task.cpp b/src/thread/task.cpp index b081872..23027e0 100644 --- a/src/thread/task.cpp +++ b/src/thread/task.cpp @@ -22,7 +22,8 @@ namespace fc { _next(nullptr), _task_specific_data(nullptr), _promise_impl(nullptr), - _functor(func){ + _functor(func), + _retain_count(0){ } void task_base::run() { @@ -101,4 +102,12 @@ namespace fc { } } + void task_base::retain() { + if( _retain_count.fetch_add(1, boost::memory_order_relaxed) == 0 ) + _self = shared_from_this(); + } + void task_base::release() { + if( _retain_count.fetch_sub(1, boost::memory_order_release) == 1 ) + _self.reset(); + } } diff --git a/src/thread/thread_d.hpp b/src/thread/thread_d.hpp index b0b6916..055ae8b 100644 --- a/src/thread/thread_d.hpp +++ b/src/thread/thread_d.hpp @@ -335,7 +335,7 @@ namespace fc { if( (*task_itr)->canceled() ) { (*task_itr)->run(); - (*task_itr)->release(); + (*task_itr)->release(); // HERE BE DRAGONS task_itr = task_sch_queue.erase(task_itr); canceled_task = true; continue; @@ -531,10 +531,10 @@ namespace fc { next->_set_active_context( current ); current->cur_task = next; - fc::shared_ptr next_ptr(next); - next_ptr->run(); + next->run(); current->cur_task = 0; - next_ptr->_set_active_context(0); + next->_set_active_context(0); + next->release(); // HERE BE DRAGONS current->reinitialize(); }