While tracking down the issue discussed in the commit message of
78dc7d982b "Remove a potentially already enqueued
response when a bridge is disposed", it occurred to me that there should be a
race in those
uno_threadpool_putJob(
bridge_->getThreadPool(), ...);
calls in binaryurp/source/reader.cxx, when the bridge gets disposed (through
some other thread) between the time the bridge_->getThreadPool() call checks for
the bridge being disposed (in which case it would throw a DisposedException) and
the actual uno_threadpool_putJob call.
I tried to catch that with a previous incarnation of this change
(<https://gerrit.libreoffice.org/c/core/+/96120/1> "Jenkins Slides Through the
Tiny Window"), but couldn't---presumably because this race would be very rare
after all, and the issue I was chasing turned out to be caused by something
different anyway. Nevertheless, I wanted to address this potential race now.
We can only reliably check for disposed'ness after having locked ThreadPool's
m_mutex in uno_threadpool_putJob -> ThreadPool::addJob, but at which time we can
no longer indicate this condition to the caller---uno_threapool_putJob is part
of the stable URE interface, has a void return type, and should not throw any
exceptions as it is a C function. However, if the bridge gets disposed, any
threads that would wait for this job (in cppu_threadpool::JobQueue::enter,
either from cppu_threadpool::ORequestThread::run waiting to process new incoming
calls, or from a bridge's call to uno_threadpool_enter waiting for a respose to
an outgoing call) should already learn about the bridge being disposed by
falling out of cppu_threadpool::JobQueue::enter with a null return value. So it
should be OK if uno_threadpool_putJob silently discards the job in that case.
Change-Id: I36fe996436f55a93d84d66cc0b164e2e45a37e81
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96120
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
This avoids the need for the tricky osl::Condition::reset calls. For example,
the first one (in the "disposed !" case) had been added with
2a3ed89284 "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 cba3ac1eab "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 <sbergman@redhat.com>
The issue of 362d4f0cd4 "Explicitly mark
overriding destructors as 'virtual'" appears to no longer be a problem with
MSVC 2013.
(The little change in the rewriting code of compilerplugins/clang/override.cxx
was necessary to prevent an endless loop when adding "override" to
OOO_DLLPUBLIC_CHARTTOOLS virtual ~CloseableLifeTimeManager();
in chart2/source/inc/LifeTime.hxx, getting stuck in the leading
OOO_DLLPUBLIC_CHARTTOOLS macro. Can't remember what that
isAtEndOfImmediateMacroExpansion thing was originally necessary for, anyway.)
Change-Id: I534c634504d7216b9bb632c2775c04eaf27e927e
This improves on b68640c44e "Prevent creation of
new ORequestThreads during shutdown," which added throwing the DisposedException
from ThreadAdmin::add. But ThreadAdmin::m_disposed can only become true via
uno_threadpool_destroy -> ThreadPool::joinWorkers -> ThreadAdmin::join, and
ThreadAdmin::add observing that can only happen via uno_threadpool_putJob ->
ThreadPool::addJob -> ThreadPool::createThread -> ORequestThread::launch ->
ThradAdmin::add, where the bridges should ensure that uno_threadpool_destroy
does not run in parallel with uno_threadpool_putJob. So demote this from a
DisposedException to a SAL_WARN.
Change-Id: I3912ea077b7fa35827c41e82dd0a8f962ba412b6
It appears that the C++ standard allows overriding destructors to be marked
"override," but at least some MSVC versions complain about it, so at least make
sure such destructors are explicitly marked "virtual."
Change-Id: I0e1cafa7584fd16ebdce61f569eae2373a71b0a1
This is a follow up to d015384e1d "Fixed
ThreadPool (and dependent ORequestThread) life cycle" that still had some
problems:
* First, if Bridge::terminate was first entered from the reader or writer
thread, it would not join on that thread, so that thread could still be running
during exit.
That has been addressed by giving Bridge::dispose new semantics: It waits until
both Bridge::terminate has completed (even if that was called from a different
thread) and all spawned threads (reader, writer, ORequestThread workers) have
been joined. (This implies that Bridge::dispose must not be called from such a
thread, to avoid deadlock.)
* Second, if Bridge::terminate was first entered from an ORequestThread, the
call to uno_threadpool_dispose(0) to join on all such worker threads could
deadlock.
That has been addressed by making the last call to uno_threadpool_destroy wait
to join on all worker threads, and by calling uno_threadpool_destroy only from
the final Bridge::terminate (from Bridge::dispose), to avoid deadlock. (The
special semantics of uno_threadpool_dispose(0) are no longer needed and have
been removed, as they conflicted with the fix for the third problem below.)
* Third, once uno_threadpool_destroy had called uno_threadpool_dispose(0), the
ThreadAdmin singleton had been disposed, so no new remote bridges could
successfully be created afterwards.
That has been addressed by making ThreadAdmin a member of ThreadPool, and making
(only) those uno_ThreadPool handles with overlapping life spans share one
ThreadPool instance (which thus is no longer a singleton, either).
Additionally, ORequestThread has been made more robust (in the style of
salhelper::Thread) to avoid races.
Change-Id: I2cbd1b3f9aecc1bf4649e482d2c22b33b471788f
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
2006/03/22 10:14:07 sb 1.2.38.1: #i63397# Keep objects alive long enough so that threads still running while atexit handlers are processed do not access dead objects.
Noel Grandin