[REVIEW-3-5] Fixed ThreadPool (and dependent ORequestThread) life cycle
Stephan Bergmann
sbergman at redhat.com
Fri May 18 14:04:07 PDT 2012
Coincidentally, this bug that, for whatever reason, started to
repeatedly break "make check" for my local master builds also got
reported against (Fedora) LO 3.5.2 (see
<https://bugzilla.redhat.com/show_bug.cgi?id=821134>) at about the same
time I came up with the below master fix.
Therefore, I'd like to get this reviewed and backported to
libreoffice-3-5. As the commit message explains, only part of it is
strictly necessary to fix the bug, so I would also be fine if reviewers
insisted on only backporting the two parts that are really necessary and
leaving out the two cleanup topics. As the fix is mildly delicate
(introducing a thread join, which in principle has the potential to
introduce a deadlock) and only relevant for specific scenarios (where
UNO remote communication is involved; i.e., during deployment of
extensions, or in explicit client/server applications), I would not
suggest it for inclusion into upcoming LO 3.5.4.
The below master commit will not cleanly apply to libreoffice-3-5 (at
least not to libreoffice-3-5-4, where I tried it), so find attached an
adapted 0001-Fixed-ThreadPool-and-dependent-ORequestThread-life-c.patch.
Thanks,
Stephan
On 05/16/2012 10:21 PM, Stephan Bergmann wrote:
> binaryurp/source/bridge.cxx | 52 ++++++++++++++++++++--------------
> binaryurp/source/bridge.hxx | 4 +-
> cppu/source/threadpool/threadpool.cxx | 24 +++++++++++----
> cppu/source/threadpool/threadpool.hxx | 4 +-
> 4 files changed, 52 insertions(+), 32 deletions(-)
>
> New commits:
> commit d015384e1d98fe77fd59339044f58efb1ab9fb25
> Author: Stephan Bergmann<sbergman at redhat.com>
> Date: Wed May 16 22:09:21 2012 +0200
>
> Fixed ThreadPool (and dependent ORequestThread) life cycle
>
> At least with sw_complex test under load, it happened that an ORequestThread
> could still process a remote release request while the main thread was already
> in exit(3). This was because (a) ThreadPool never joined with the spawned
> worker threads (which has been rectified by calling uno_threadpool_dispose(0)
> from the final uno_threadpool_destroy), and (b) binaryurp::Bridge called
> uno_threadpool_destroy only from its destructor (which could go as late as
> exit(3)) instead of from terminate.
>
> Additional clean up:
> * Access to Bridge's threadPool_ is now cleanly controlled by mutex_ (even
> though that might not be necessary in every case).
> * ThreadPool's stopDisposing got renamed to destroy, to make meaning clearer.
>
> Change-Id: I45fa76e80e790a11065e7bf8ac9d92af2e62f262
>
> diff --git a/binaryurp/source/bridge.cxx b/binaryurp/source/bridge.cxx
> index 50b873f..2d1f622 100644
> --- a/binaryurp/source/bridge.cxx
> +++ b/binaryurp/source/bridge.cxx
> @@ -235,22 +235,28 @@ Bridge::Bridge(
> }
>
> void Bridge::start() {
> - assert(threadPool_ == 0&& !writer_.is()&& !reader_.is());
> - threadPool_ = uno_threadpool_create();
> - assert(threadPool_ != 0);
> - writer_.set(new Writer(this));
> - writer_->launch();
> - reader_.set(new Reader(this));
> - reader_->launch();
> - // it is important to call reader_->launch() last here; both
> - // Writer::execute and Reader::execute can call Bridge::terminate, but
> - // Writer::execute is initially blocked in unblocked_.wait() until
> - // Reader::execute has called bridge_->sendRequestChangeRequest(), so
> - // effectively only reader_->launch() can lead to an early call to
> - // Bridge::terminate
> + rtl::Reference< Reader> r(new Reader(this));
> + rtl::Reference< Writer> w(new Writer(this));
> + {
> + osl::MutexGuard g(mutex_);
> + assert(threadPool_ == 0&& !writer_.is()&& !reader_.is());
> + threadPool_ = uno_threadpool_create();
> + assert(threadPool_ != 0);
> + reader_ = r;
> + writer_ = w;
> + }
> + // It is important to call reader_->launch() last here; both
> + // Writer::execute and Reader::execute can call Bridge::terminate, but
> + // Writer::execute is initially blocked in unblocked_.wait() until
> + // Reader::execute has called bridge_->sendRequestChangeRequest(), so
> + // effectively only reader_->launch() can lead to an early call to
> + // Bridge::terminate
> + w->launch();
> + r->launch();
> }
>
> void Bridge::terminate() {
> + uno_ThreadPool tp;
> rtl::Reference< Reader> r;
> rtl::Reference< Writer> w;
> Listeners ls;
> @@ -259,6 +265,7 @@ void Bridge::terminate() {
> if (terminated_) {
> return;
> }
> + tp = threadPool_;
> std::swap(reader_, r);
> std::swap(writer_, w);
> ls.swap(listeners_);
> @@ -273,8 +280,8 @@ void Bridge::terminate() {
> w->stop();
> joinThread(r.get());
> joinThread(w.get());
> - assert(threadPool_ != 0);
> - uno_threadpool_dispose(threadPool_);
> + assert(tp != 0);
> + uno_threadpool_dispose(tp);
> Stubs s;
> {
> osl::MutexGuard g(mutex_);
> @@ -301,6 +308,7 @@ void Bridge::terminate() {
> "binaryurp", "caught runtime exception '"<< e.Message<< '\'');
> }
> }
> + uno_threadpool_destroy(tp);
> }
>
> css::uno::Reference< css::connection::XConnection> Bridge::getConnection()
> @@ -330,7 +338,8 @@ BinaryAny Bridge::mapCppToBinaryAny(css::uno::Any const& cppAny) {
> return out;
> }
>
> -uno_ThreadPool Bridge::getThreadPool() const {
> +uno_ThreadPool Bridge::getThreadPool() {
> + osl::MutexGuard g(mutex_);
> assert(threadPool_ != 0);
> return threadPool_;
> }
> @@ -571,7 +580,8 @@ bool Bridge::makeCall(
> {
> std::auto_ptr< IncomingReply> resp;
> {
> - AttachThread att(threadPool_);
> + uno_ThreadPool tp = getThreadPool();
> + AttachThread att(tp);
> PopOutgoingRequest pop(
> outgoingRequests_, att.getTid(),
> OutgoingRequest(OutgoingRequest::KIND_NORMAL, member, setter));
> @@ -582,7 +592,7 @@ bool Bridge::makeCall(
> incrementCalls(true);
> incrementActiveCalls();
> void * job;
> - uno_threadpool_enter(threadPool_,&job);
> + uno_threadpool_enter(tp,&job);
> resp.reset(static_cast< IncomingReply *>(job));
> decrementActiveCalls();
> decrementCalls();
> @@ -812,8 +822,8 @@ bool Bridge::isCurrentContextMode() {
> }
>
> Bridge::~Bridge() {
> - if (threadPool_ != 0) {
> - uno_threadpool_destroy(threadPool_);
> + if (getThreadPool() != 0) {
> + terminate();
> }
> }
>
> @@ -940,7 +950,7 @@ void Bridge::sendProtPropRequest(
> void Bridge::makeReleaseCall(
> rtl::OUString const& oid, css::uno::TypeDescription const& type)
> {
> - AttachThread att(threadPool_);
> + AttachThread att(getThreadPool());
> sendRequest(
> att.getTid(), oid, type,
> css::uno::TypeDescription(
> diff --git a/binaryurp/source/bridge.hxx b/binaryurp/source/bridge.hxx
> index cf281f2..8d66789 100644
> --- a/binaryurp/source/bridge.hxx
> +++ b/binaryurp/source/bridge.hxx
> @@ -106,7 +106,7 @@ public:
>
> BinaryAny mapCppToBinaryAny(com::sun::star::uno::Any const& cppAny);
>
> - uno_ThreadPool getThreadPool() const;
> + uno_ThreadPool getThreadPool();
>
> rtl::Reference< Writer> getWriter();
>
> @@ -258,11 +258,11 @@ private:
> com::sun::star::uno::TypeDescription protPropType_;
> com::sun::star::uno::TypeDescription protPropRequest_;
> com::sun::star::uno::TypeDescription protPropCommit_;
> - uno_ThreadPool threadPool_;
> OutgoingRequests outgoingRequests_;
>
> osl::Mutex mutex_;
> Listeners listeners_;
> + uno_ThreadPool threadPool_;
> rtl::Reference< Writer> writer_;
> rtl::Reference< Reader> reader_;
> bool currentContextMode_;
> diff --git a/cppu/source/threadpool/threadpool.cxx b/cppu/source/threadpool/threadpool.cxx
> index 9dda867..d14e260 100644
> --- a/cppu/source/threadpool/threadpool.cxx
> +++ b/cppu/source/threadpool/threadpool.cxx
> @@ -26,7 +26,10 @@
> *
> ************************************************************************/
>
> +#include "sal/config.h"
> +
> #include<boost/unordered_map.hpp>
> +#include<cassert>
> #include<stdio.h>
>
> #include<osl/diagnose.h>
> @@ -73,7 +76,7 @@ namespace cppu_threadpool
> m_lst.push_back( nDisposeId );
> }
>
> - void DisposedCallerAdmin::stopDisposing( sal_Int64 nDisposeId )
> + void DisposedCallerAdmin::destroy( sal_Int64 nDisposeId )
> {
> MutexGuard guard( m_mutex );
> for( DisposedCallerList::iterator ii = m_lst.begin() ;
> @@ -172,9 +175,9 @@ namespace cppu_threadpool
> }
> }
>
> - void ThreadPool::stopDisposing( sal_Int64 nDisposeId )
> + void ThreadPool::destroy( sal_Int64 nDisposeId )
> {
> - m_DisposedCallerAdmin->stopDisposing( nDisposeId );
> + m_DisposedCallerAdmin->destroy( nDisposeId );
> }
>
> /******************
> @@ -480,13 +483,14 @@ uno_threadpool_dispose( uno_ThreadPool hPool ) SAL_THROW_EXTERN_C()
> extern "C" void SAL_CALL
> uno_threadpool_destroy( uno_ThreadPool hPool ) SAL_THROW_EXTERN_C()
> {
> - ThreadPool::getInstance()->stopDisposing(
> + assert(hPool != 0);
> +
> + ThreadPool::getInstance()->destroy(
> sal::static_int_cast< sal_Int64>(
> reinterpret_cast< sal_IntPtr>(hPool)) );
>
> - if( hPool )
> + bool empty;
> {
> - // special treatment for 0 !
> OSL_ASSERT( g_pThreadpoolHashSet );
>
> MutexGuard guard( Mutex::getGlobalMutex() );
> @@ -496,12 +500,18 @@ uno_threadpool_destroy( uno_ThreadPool hPool ) SAL_THROW_EXTERN_C()
> g_pThreadpoolHashSet->erase( ii );
> delete hPool;
>
> - if( g_pThreadpoolHashSet->empty() )
> + empty = g_pThreadpoolHashSet->empty();
> + if( empty )
> {
> delete g_pThreadpoolHashSet;
> g_pThreadpoolHashSet = 0;
> }
> }
> +
> + if( empty )
> + {
> + uno_threadpool_dispose( 0 );
> + }
> }
>
> /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
> diff --git a/cppu/source/threadpool/threadpool.hxx b/cppu/source/threadpool/threadpool.hxx
> index 498ea4a..8b64ed1 100644
> --- a/cppu/source/threadpool/threadpool.hxx
> +++ b/cppu/source/threadpool/threadpool.hxx
> @@ -90,7 +90,7 @@ namespace cppu_threadpool {
> static DisposedCallerAdminHolder getInstance();
>
> void dispose( sal_Int64 nDisposeId );
> - void stopDisposing( sal_Int64 nDisposeId );
> + void destroy( sal_Int64 nDisposeId );
> sal_Bool isDisposed( sal_Int64 nDisposeId );
>
> private:
> @@ -109,7 +109,7 @@ namespace cppu_threadpool {
> static ThreadPoolHolder getInstance();
>
> void dispose( sal_Int64 nDisposeId );
> - void stopDisposing( sal_Int64 nDisposeId );
> + void destroy( sal_Int64 nDisposeId );
>
> void addJob( const ByteSequence&aThreadId,
> sal_Bool bAsynchron,
> _______________________________________________
> Libreoffice-commits mailing list
> Libreoffice-commits at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
More information about the LibreOffice
mailing list