From 1c1cd79697ab24e4be76e95d29bae1f2350b55ca Mon Sep 17 00:00:00 2001 From: Stephen Webb Date: Tue, 23 Jul 2024 10:13:58 +1000 Subject: [PATCH] Prevent potential deadlock on shutdown when using AsyncAppender (#396) * Use separate synchronization for the Hierarchy listener list * Ensure the dispatch thread has terminated before destroying the std::thread --- src/main/cpp/asyncappender.cpp | 78 ++++++++++++++++++++-------------- src/main/cpp/hierarchy.cpp | 10 +++-- 2 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 26d5febcd..82894d91a 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -15,10 +15,8 @@ * limitations under the License. */ - #include - #include #include #include @@ -105,30 +103,34 @@ typedef std::map DiscardMap; #endif #ifdef __cpp_lib_hardware_interference_size - using std::hardware_constructive_interference_size; - using std::hardware_destructive_interference_size; + using std::hardware_constructive_interference_size; + using std::hardware_destructive_interference_size; #else - // 64 bytes on x86-64 │ L1_CACHE_BYTES │ L1_CACHE_SHIFT │ __cacheline_aligned │ ... - constexpr std::size_t hardware_constructive_interference_size = 64; - constexpr std::size_t hardware_destructive_interference_size = 64; + // 64 bytes on x86-64 │ L1_CACHE_BYTES │ L1_CACHE_SHIFT │ __cacheline_aligned │ ... + constexpr std::size_t hardware_constructive_interference_size = 64; + constexpr std::size_t hardware_destructive_interference_size = 64; #endif struct AsyncAppender::AsyncAppenderPriv : public AppenderSkeleton::AppenderSkeletonPrivate { - AsyncAppenderPriv() : - AppenderSkeletonPrivate(), - buffer(DEFAULT_BUFFER_SIZE), - bufferSize(DEFAULT_BUFFER_SIZE), - dispatcher(), - locationInfo(false), - blocking(true) + AsyncAppenderPriv() + : AppenderSkeletonPrivate() + , buffer(DEFAULT_BUFFER_SIZE) + , bufferSize(DEFAULT_BUFFER_SIZE) + , dispatcher() + , locationInfo(false) + , blocking(true) #if LOG4CXX_EVENTS_AT_EXIT , atExitRegistryRaii([this]{stopDispatcher();}) #endif , eventCount(0) , dispatchedCount(0) , commitCount(0) + { } + + ~AsyncAppenderPriv() { + stopDispatcher(); } /** @@ -171,10 +173,7 @@ struct AsyncAppender::AsyncAppenderPriv : public AppenderSkeleton::AppenderSkele void stopDispatcher() { - { - std::lock_guard lock(bufferMutex); - closed = true; - } + this->setClosed(); bufferNotEmpty.notify_all(); bufferNotFull.notify_all(); @@ -212,6 +211,18 @@ struct AsyncAppender::AsyncAppenderPriv : public AppenderSkeleton::AppenderSkele * Used to communicate to the dispatch thread when an event is committed in buffer. */ alignas(hardware_constructive_interference_size) std::atomic commitCount; + + bool isClosed() + { + std::lock_guard lock(this->bufferMutex); + return this->closed; + } + + void setClosed() + { + std::lock_guard lock(this->bufferMutex); + this->closed = true; + } }; @@ -280,7 +291,7 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) if (!priv->dispatcher.joinable()) { - std::lock_guard lock(priv->bufferMutex); + std::lock_guard lock(priv->mutex); if (!priv->dispatcher.joinable()) priv->dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AsyncAppender"), &AsyncAppender::dispatch, this ); } @@ -515,7 +526,7 @@ void AsyncAppender::dispatch() { return priv->dispatchedCount != priv->commitCount || priv->closed; } ); } - isActive = !priv->closed; + isActive = !priv->isClosed(); while (events.size() < priv->bufferSize && priv->dispatchedCount != priv->commitCount) { @@ -545,7 +556,7 @@ void AsyncAppender::dispatch() } catch (std::exception& ex) { - if (isActive) + if (!priv->isClosed()) { priv->errorHandler->error(LOG4CXX_STR("async dispatcher"), ex, 0, item); isActive = false; @@ -553,26 +564,27 @@ void AsyncAppender::dispatch() } catch (...) { - if (isActive) + if (!priv->isClosed()) { priv->errorHandler->error(LOG4CXX_STR("async dispatcher")); isActive = false; } } } - if (!isActive && LogLog::isDebugEnabled()) + } + if (LogLog::isDebugEnabled()) + { + Pool p; + LogString msg(LOG4CXX_STR("AsyncAppender")); + msg += LOG4CXX_STR(" discardCount "); + StringHelper::toString(discardCount, p, msg); + msg += LOG4CXX_STR(" pendingCountHistogram"); + for (auto item : pendingCountHistogram) { - LogString msg(LOG4CXX_STR("AsyncAppender")); - msg += LOG4CXX_STR(" discardCount "); - StringHelper::toString(discardCount, p, msg); - msg += LOG4CXX_STR(" pendingCountHistogram"); - for (auto item : pendingCountHistogram) - { - msg += logchar(' '); - StringHelper::toString(item, p, msg); - } - LogLog::debug(msg); + msg += logchar(' '); + StringHelper::toString(item, p, msg); } + LogLog::debug(msg); } } diff --git a/src/main/cpp/hierarchy.cpp b/src/main/cpp/hierarchy.cpp index 84b8046de..bbd8907a9 100644 --- a/src/main/cpp/hierarchy.cpp +++ b/src/main/cpp/hierarchy.cpp @@ -62,6 +62,8 @@ struct Hierarchy::HierarchyPrivate ProvisionNodeMap provisionNodes; std::vector allAppenders; + + mutable std::mutex listenerMutex; }; IMPLEMENT_LOG4CXX_OBJECT(Hierarchy) @@ -91,7 +93,7 @@ Hierarchy::~Hierarchy() void Hierarchy::addHierarchyEventListener(const spi::HierarchyEventListenerPtr& listener) { - std::lock_guard lock(m_priv->mutex); + std::lock_guard lock(m_priv->listenerMutex); if (std::find(m_priv->listeners.begin(), m_priv->listeners.end(), listener) != m_priv->listeners.end()) { @@ -105,7 +107,7 @@ void Hierarchy::addHierarchyEventListener(const spi::HierarchyEventListenerPtr& void Hierarchy::removeHierarchyEventListener(const spi::HierarchyEventListenerPtr& listener) { - std::lock_guard lock(m_priv->mutex); + std::lock_guard lock(m_priv->listenerMutex); auto found = std::find(m_priv->listeners.begin(), m_priv->listeners.end(), listener); if(found != m_priv->listeners.end()){ @@ -194,7 +196,7 @@ void Hierarchy::fireAddAppenderEvent(const Logger* logger, const Appender* appen setConfigured(true); HierarchyEventListenerList clonedList; { - std::lock_guard lock(m_priv->mutex); + std::lock_guard lock(m_priv->listenerMutex); clonedList = m_priv->listeners; } @@ -207,7 +209,7 @@ void Hierarchy::fireRemoveAppenderEvent(const Logger* logger, const Appender* ap { HierarchyEventListenerList clonedList; { - std::lock_guard lock(m_priv->mutex); + std::lock_guard lock(m_priv->listenerMutex); clonedList = m_priv->listeners; } for (auto& item : clonedList)