From 976bbce66864546fa5b8cd5bc4824b3b565f81e1 Mon Sep 17 00:00:00 2001 From: Eric Frias Date: Mon, 25 Aug 2014 18:44:15 -0400 Subject: [PATCH] When locking a mutex, ensure the task has a context before attempting to lock. --- src/thread/mutex.cpp | 53 +++++++++++++++++++++++++------------------ tests/task_cancel.cpp | 43 ++++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 23 deletions(-) diff --git a/src/thread/mutex.cpp b/src/thread/mutex.cpp index 40e2039..8119ae7 100644 --- a/src/thread/mutex.cpp +++ b/src/thread/mutex.cpp @@ -23,19 +23,22 @@ namespace fc { } /** - * @param next - is set to the next context to get the lock. + * @param last_context - is set to the next context to get the lock (the next-to-last element of the list) * @return the last context (the one with the lock) */ - static fc::context* get_tail( fc::context* h, fc::context*& next ) { - next = 0; - fc::context* n = h; - if( !n ) return n; - while( n->next_blocked_mutex ) { - next = n; - n=n->next_blocked_mutex; + static fc::context* get_tail( fc::context* list_head, fc::context*& context_to_unblock ) { + context_to_unblock = 0; + fc::context* list_context_iter = list_head; + if( !list_context_iter ) + return list_context_iter; + while( list_context_iter->next_blocked_mutex ) + { + context_to_unblock = list_context_iter; + list_context_iter = list_context_iter->next_blocked_mutex; } - return n; + return list_context_iter; } + static fc::context* remove( fc::context* head, fc::context* target ) { fc::context* c = head; fc::context* p = 0; @@ -114,17 +117,21 @@ namespace fc { } void mutex::lock() { - fc::context* n = 0; - fc::context* cc = fc::thread::current().my->current; + fc::context* current_context = fc::thread::current().my->current; + if( !current_context ) + current_context = fc::thread::current().my->current = new fc::context( &fc::thread::current() ); + { fc::unique_lock lock(m_blist_lock); if( !m_blist ) { - m_blist = cc; + m_blist = current_context; + assert(!current_context->next_blocked_mutex); return; } // allow recusive locks - if ( get_tail( m_blist, n ) == cc ) { + fc::context* dummy_context_to_unblock = 0; + if ( get_tail( m_blist, dummy_context_to_unblock ) == current_context ) { assert(false); // EMF: I think recursive locks are currently broken -- we need to // keep track of how many times this mutex has been locked by the @@ -132,9 +139,10 @@ namespace fc { // the next context only if the count drops to zero return; } - cc->next_blocked_mutex = m_blist; - m_blist = cc; + current_context->next_blocked_mutex = m_blist; + m_blist = current_context; +#if 0 int cnt = 0; auto i = m_blist; while( i ) { @@ -142,25 +150,26 @@ namespace fc { ++cnt; } //wlog( "wait queue len %1%", cnt ); +#endif } try { fc::thread::current().yield(false); - BOOST_ASSERT( cc->next_blocked_mutex == 0 ); + BOOST_ASSERT( current_context->next_blocked_mutex == 0 ); } catch ( ... ) { wlog( "lock threw" ); - cleanup( *this, m_blist_lock, m_blist, cc); + cleanup( *this, m_blist_lock, m_blist, current_context); throw; } } void mutex::unlock() { - fc::context* next = 0; + fc::context* context_to_unblock = 0; { fc::unique_lock lock(m_blist_lock); - get_tail(m_blist, next); - if( next ) { - next->next_blocked_mutex = 0; - next->ctx_thread->my->unblock( next ); + get_tail(m_blist, context_to_unblock); + if( context_to_unblock ) { + context_to_unblock->next_blocked_mutex = 0; + context_to_unblock->ctx_thread->my->unblock( context_to_unblock ); } else { m_blist = 0; } diff --git a/tests/task_cancel.cpp b/tests/task_cancel.cpp index 32f9fec..0d6500f 100644 --- a/tests/task_cancel.cpp +++ b/tests/task_cancel.cpp @@ -12,13 +12,54 @@ BOOST_AUTO_TEST_CASE( leave_mutex_locked ) { { fc::mutex test_mutex; - fc::future test_task = fc::async([&](){ fc::scoped_lock test_lock(test_mutex); for (int i = 0; i < 10; ++i) fc::usleep(fc::seconds(1));}); + fc::future test_task = fc::async([&](){ + fc::scoped_lock test_lock(test_mutex); + for (int i = 0; i < 10; ++i) + fc::usleep(fc::seconds(1)); + }, "test_task"); fc::usleep(fc::seconds(3)); test_task.cancel_and_wait(); } BOOST_TEST_PASSPOINT(); } +BOOST_AUTO_TEST_CASE( cancel_task_blocked_on_mutex) +{ + { + fc::mutex test_mutex; + fc::future test_task; + { + fc::scoped_lock test_lock(test_mutex); + test_task = fc::async([&test_mutex](){ + BOOST_TEST_MESSAGE("--- In test_task, locking mutex"); + fc::scoped_lock async_task_test_lock(test_mutex); + BOOST_TEST_MESSAGE("--- In test_task, mutex locked, commencing sleep"); + for (int i = 0; i < 10; ++i) + fc::usleep(fc::seconds(1)); + BOOST_TEST_MESSAGE("--- In test_task, sleeps done, exiting"); + }, "test_task"); + fc::usleep(fc::seconds(3)); + test_task.cancel(); + try + { + test_task.wait(fc::seconds(1)); + BOOST_ERROR("test should have been canceled, not exited cleanly"); + } + catch (const fc::canceled_exception&) + { + BOOST_TEST_PASSPOINT(); + } + catch (const fc::timeout_exception&) + { + BOOST_ERROR("unable to cancel task blocked on mutex"); + } + BOOST_TEST_MESSAGE("Unlocking mutex locked from the main task so the test task will have the opportunity to lock it and be canceled"); + } + test_task.cancel_and_wait(); + } +} + + BOOST_AUTO_TEST_CASE( test_non_preemptable_assertion ) { return; // this isn't a real test, because the thing it tries to test works by asserting, not by throwing