From a0bc388b38859ab67067340f836f26fa8f850cbf Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Tue, 16 Jun 2020 08:25:36 +0200 Subject: Use the saner std::condition_variable semantics for JobQueue::m_cndWait This avoids the need for the tricky osl::Condition::reset calls. For example, the first one (in the "disposed !" case) had been added with 2a3ed89284890615ad4bce57be55ba558351cde1 "sb132: #i112448# proper clean up in JobQueue::enter (patch by olistraub)" as > if( 0 == m_lstCallstack.front() ) > { > // disposed ! > + if( m_lstJob.empty() ) > + { > + osl_resetCondition( m_cndWait ); > + } > break; > } > and then change to > if( 0 == m_lstCallstack.front() ) > { > // disposed ! > - if( m_lstJob.empty() ) > + if( m_lstJob.empty() > + && (m_lstCallstack.empty() > + || m_lstCallstack.front() != 0) ) > { > osl_resetCondition( m_cndWait ); > } with cba3ac1eab7acaf8e6efd7a00eee7c5e969fc49b "Avoid deadlocks when disposing recursive JobQueue::enter", which prevented the reset from ever hapening (because m_lstCallstack.front() cannot both be zero and non-zero here at the same time). The std::condition_variable semantics nicely avoid any reasoning whether or not a reset would be necessary here. Change-Id: Ic9b57b836bb6679829f4aa3b68679256726acf60 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96406 Tested-by: Jenkins Reviewed-by: Stephan Bergmann --- cppu/source/threadpool/jobqueue.cxx | 76 ++++++++++++++--------------------- cppu/source/threadpool/jobqueue.hxx | 12 +++--- cppu/source/threadpool/threadpool.hxx | 2 +- 3 files changed, 39 insertions(+), 51 deletions(-) (limited to 'cppu') diff --git a/cppu/source/threadpool/jobqueue.cxx b/cppu/source/threadpool/jobqueue.cxx index 748bc06a422e..1be424024d4b 100644 --- a/cppu/source/threadpool/jobqueue.cxx +++ b/cppu/source/threadpool/jobqueue.cxx @@ -17,12 +17,12 @@ * the License at http://www.apache.org/licenses/LICENSE-2.0 . */ -#include "jobqueue.hxx" -#include "threadpool.hxx" +#include -#include +#include -using namespace ::osl; +#include "jobqueue.hxx" +#include "threadpool.hxx" namespace cppu_threadpool { @@ -35,12 +35,12 @@ namespace cppu_threadpool { void JobQueue::add( void *pThreadSpecificData, RequestFun * doRequest ) { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); Job job = { pThreadSpecificData , doRequest }; m_lstJob.push_back( job ); if( ! m_bSuspended ) { - m_cndWait.set(); + m_cndWait.notify_all(); } m_nToDo ++; } @@ -50,7 +50,7 @@ namespace cppu_threadpool { void *pReturn = nullptr; { // synchronize with the dispose calls - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); if( m_DisposedCallerAdmin->isDisposed( nDisposeId ) ) { return nullptr; @@ -61,21 +61,16 @@ namespace cppu_threadpool { while( true ) { - if( bReturnWhenNoJob ) + struct Job job={nullptr,nullptr}; { - MutexGuard guard( m_mutex ); - if( m_lstJob.empty() ) + std::unique_lock guard( m_mutex ); + + while (m_bSuspended + || (m_lstCallstack.front() != nullptr && !bReturnWhenNoJob + && m_lstJob.empty())) { - break; + m_cndWait.wait(guard); } - } - - m_cndWait.wait(); - - struct Job job={nullptr,nullptr}; - { - // synchronize with add and dispose calls - MutexGuard guard( m_mutex ); if( nullptr == m_lstCallstack.front() ) { @@ -87,38 +82,29 @@ namespace cppu_threadpool { // response here: m_lstJob.pop_front(); } - if( m_lstJob.empty() - && (m_lstCallstack.empty() - || m_lstCallstack.front() != nullptr) ) - { - m_cndWait.reset(); - } break; } - OSL_ASSERT( ! m_lstJob.empty() ); - if( ! m_lstJob.empty() ) - { - job = m_lstJob.front(); - m_lstJob.pop_front(); - } - if( m_lstJob.empty() - && (m_lstCallstack.empty() || m_lstCallstack.front() != nullptr) ) + if( m_lstJob.empty() ) { - m_cndWait.reset(); + assert(bReturnWhenNoJob); + break; } + + job = m_lstJob.front(); + m_lstJob.pop_front(); } if( job.doRequest ) { job.doRequest( job.pThreadSpecificData ); - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); m_nToDo --; } else { pReturn = job.pThreadSpecificData; - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); m_nToDo --; break; } @@ -126,7 +112,7 @@ namespace cppu_threadpool { { // synchronize with the dispose calls - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); m_lstCallstack.pop_front(); } @@ -135,7 +121,7 @@ namespace cppu_threadpool { void JobQueue::dispose( void const * nDisposeId ) { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); for( auto& rId : m_lstCallstack ) { if( rId == nDisposeId ) @@ -147,41 +133,41 @@ namespace cppu_threadpool { if( !m_lstCallstack.empty() && ! m_lstCallstack.front() ) { // The thread is waiting for a disposed pCallerId, let it go - m_cndWait.set(); + m_cndWait.notify_all(); } } void JobQueue::suspend() { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); m_bSuspended = true; } void JobQueue::resume() { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); m_bSuspended = false; if( ! m_lstJob.empty() ) { - m_cndWait.set(); + m_cndWait.notify_all(); } } bool JobQueue::isEmpty() const { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); return m_lstJob.empty(); } bool JobQueue::isCallstackEmpty() const { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); return m_lstCallstack.empty(); } bool JobQueue::isBusy() const { - MutexGuard guard( m_mutex ); + std::scoped_lock guard( m_mutex ); return m_nToDo > 0; } diff --git a/cppu/source/threadpool/jobqueue.hxx b/cppu/source/threadpool/jobqueue.hxx index a0ccf5430385..5a5c80479d2b 100644 --- a/cppu/source/threadpool/jobqueue.hxx +++ b/cppu/source/threadpool/jobqueue.hxx @@ -20,12 +20,14 @@ #ifndef INCLUDED_CPPU_SOURCE_THREADPOOL_JOBQUEUE_HXX #define INCLUDED_CPPU_SOURCE_THREADPOOL_JOBQUEUE_HXX +#include + +#include #include #include -#include +#include -#include -#include +#include namespace cppu_threadpool { @@ -58,12 +60,12 @@ namespace cppu_threadpool bool isBusy() const; private: - mutable ::osl::Mutex m_mutex; + mutable std::mutex m_mutex; std::deque < struct Job > m_lstJob; std::deque m_lstCallstack; sal_Int32 m_nToDo; bool m_bSuspended; - osl::Condition m_cndWait; + std::condition_variable m_cndWait; DisposedCallerAdminHolder m_DisposedCallerAdmin; }; } diff --git a/cppu/source/threadpool/threadpool.hxx b/cppu/source/threadpool/threadpool.hxx index acedbf628a03..c86e6575bb66 100644 --- a/cppu/source/threadpool/threadpool.hxx +++ b/cppu/source/threadpool/threadpool.hxx @@ -24,7 +24,7 @@ #include #include - +#include #include #include #include -- cgit