From 3222dc7c0be62b9ad0fdfd303c5c4f53b39063e6 Mon Sep 17 00:00:00 2001 From: Eric Frias Date: Thu, 28 Aug 2014 15:42:01 -0400 Subject: [PATCH] When reusing a context, re-initialize most of its fields. This fixes at least two errors: - we were canceling tasks that hadn't been canceled, because the canceled flag was left set to true and the next task assigned to the context then became canceled as soon as it yielded - we were resumeing blocked tasks before they should have resumed, because their blocking_promises list wasn't cleared and they were unblocking because the erroneous promises were fulfilled As a debugging aid, we also record the cancellation reason whenever a task is canceled, and include that in the canceled_exception (this is only enabled in debug builds) --- include/fc/thread/future.hpp | 6 +++++- src/thread/context.hpp | 17 ++++++++++++++++- src/thread/future.cpp | 6 +++--- src/thread/task.cpp | 8 +++++++- src/thread/thread.cpp | 4 ++-- src/thread/thread_d.hpp | 16 +++++++++++++--- 6 files changed, 46 insertions(+), 11 deletions(-) diff --git a/include/fc/thread/future.hpp b/include/fc/thread/future.hpp index 7233274..4068dd0 100644 --- a/include/fc/thread/future.hpp +++ b/include/fc/thread/future.hpp @@ -13,7 +13,7 @@ # define FC_TASK_NAME_DEFAULT_ARG = "?" #endif -#define FC_CANCELATION_REASONS_ARE_MANDATORY 1 +//#define FC_CANCELATION_REASONS_ARE_MANDATORY 1 #ifdef FC_CANCELATION_REASONS_ARE_MANDATORY # define FC_CANCELATION_REASON_DEFAULT_ARG #else @@ -98,7 +98,11 @@ namespace fc { time_point _timeout; fc::exception_ptr _exceptp; bool _canceled; +#ifndef NDEBUG + protected: const char* _cancellation_reason; + private: +#endif const char* _desc; detail::completion_handler* _compl; }; diff --git a/src/thread/context.hpp b/src/thread/context.hpp index ecd360f..fdede1d 100644 --- a/src/thread/context.hpp +++ b/src/thread/context.hpp @@ -111,6 +111,21 @@ namespace fc { #endif } + void reinitialize() + { + canceled = false; +#ifndef NDEBUG + cancellation_reason = nullptr; +#endif + blocking_prom.clear(); + caller_context = nullptr; + resume_time = fc::time_point(); + next_blocked = nullptr; + next_blocked_mutex = nullptr; + next = nullptr; + complete = false; + } + struct blocked_promise { blocked_promise( promise_base* p=0, bool r=true ) :prom(p),required(r){} @@ -168,7 +183,7 @@ namespace fc { i->prom->set_exception( std::make_shared() ); } } - void except_blocking_promises( const exception_ptr& e ) { + void set_exception_on_blocking_promises( const exception_ptr& e ) { for( auto i = blocking_prom.begin(); i != blocking_prom.end(); ++i ) { i->prom->set_exception( e ); } diff --git a/src/thread/future.cpp b/src/thread/future.cpp index 9ad6648..92d4fea 100644 --- a/src/thread/future.cpp +++ b/src/thread/future.cpp @@ -30,11 +30,11 @@ namespace fc { void promise_base::cancel(const char* reason /* = nullptr */){ // wlog("${desc} canceled!", ("desc", _desc? _desc : "")); - _canceled = true; + _canceled = true; #ifndef NDEBUG - _cancellation_reason = reason; + _cancellation_reason = reason; #endif - } + } bool promise_base::ready()const { return _ready; } diff --git a/src/thread/task.cpp b/src/thread/task.cpp index 46af23f..4d9acfc 100644 --- a/src/thread/task.cpp +++ b/src/thread/task.cpp @@ -42,7 +42,13 @@ namespace fc { if( !canceled() ) _run_functor( _functor, _promise_impl ); else - FC_THROW_EXCEPTION( canceled_exception, "${description}", ("description", get_desc() ) ); +#ifdef NDEBUG + FC_THROW_EXCEPTION( canceled_exception, "task ${description} canceled before starting", ("description", get_desc())); +#else + FC_THROW_EXCEPTION( canceled_exception, "task ${description} canceled before starting, reason ${reason}", + ("description", get_desc()) + ("reason", _cancellation_reason ? _cancellation_reason : "[none given]")); +#endif } catch ( const exception& e ) { diff --git a/src/thread/thread.cpp b/src/thread/thread.cpp index ae9486b..e9b8efb 100644 --- a/src/thread/thread.cpp +++ b/src/thread/thread.cpp @@ -157,8 +157,8 @@ namespace fc { fc::context* n = cur->next; // this will move the context into the ready list. //cur->prom->set_exception( boost::copy_exception( error::thread_quit() ) ); - //cur->except_blocking_promises( thread_quit() ); - cur->except_blocking_promises( std::make_shared() ); + //cur->set_exception_on_blocking_promises( thread_quit() ); + cur->set_exception_on_blocking_promises( std::make_shared(FC_LOG_MESSAGE(error, "cancellation reason: thread quitting")) ); cur = n; } diff --git a/src/thread/thread_d.hpp b/src/thread/thread_d.hpp index 48a8879..af6e6b0 100644 --- a/src/thread/thread_d.hpp +++ b/src/thread/thread_d.hpp @@ -284,10 +284,14 @@ namespace fc { */ void check_fiber_exceptions() { if( current && current->canceled ) { +#ifdef NDEBUG FC_THROW_EXCEPTION( canceled_exception, "" ); +#else + FC_THROW_EXCEPTION( canceled_exception, "cancellation reason: ${reason}", ("reason", current->cancellation_reason ? current->cancellation_reason : "[none given]")); +#endif } else if( done ) { ilog( "throwing canceled exception" ); - FC_THROW_EXCEPTION( canceled_exception, "" ); + FC_THROW_EXCEPTION( canceled_exception, "cancellation reason: thread quitting" ); // BOOST_THROW_EXCEPTION( thread_quit() ); } } @@ -335,6 +339,7 @@ namespace fc { next = pt_head; pt_head = pt_head->next; next->next = 0; + next->reinitialize(); } else { // create new context. next = new fc::context( &thread_d::start_process_tasks, stack_alloc, &fc::thread::current() ); @@ -356,8 +361,12 @@ namespace fc { } if( current->canceled ) { - current->canceled = false; - FC_THROW_EXCEPTION( canceled_exception, "" ); + //current->canceled = false; +#ifdef NDEBUG + FC_THROW_EXCEPTION( canceled_exception, "" ); +#else + FC_THROW_EXCEPTION( canceled_exception, "cancellation reason: ${reason}", ("reason", current->cancellation_reason ? current->cancellation_reason : "[none given]")); +#endif } return true; @@ -390,6 +399,7 @@ namespace fc { current->cur_task = 0; next->_set_active_context(0); next->release(); + current->reinitialize(); return true; } return false;