This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch discard_appender_log_requests in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 341e4bcd27e0ab1450f2b7bb0881915fc4dfb198 Author: Stephen Webb <[email protected]> AuthorDate: Mon Oct 6 14:06:07 2025 +1100 Discard messages from an appender that sends logging requests to itself --- src/main/cpp/asyncappender.cpp | 84 +++++++++++++++++++--------------- src/test/cpp/asyncappendertestcase.cpp | 3 +- 2 files changed, 47 insertions(+), 40 deletions(-) diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 35ae409a..963080b3 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -58,17 +58,24 @@ class DiscardSummary */ int count; + /** + * Why created + */ + LogString reason; + public: /** * Create new instance. * * @param event event, may not be null. */ - DiscardSummary(const LoggingEventPtr& event); + DiscardSummary(const LoggingEventPtr& event, const LogString& reason); + /** Move constructor. */ + DiscardSummary(DiscardSummary&& src); /** Copy constructor. */ - DiscardSummary(const DiscardSummary& src); + DiscardSummary(const DiscardSummary& src) = delete; /** Assignment operator. */ - DiscardSummary& operator=(const DiscardSummary& src); + DiscardSummary& operator=(const DiscardSummary& src) = delete; /** * Add discarded event to summary. @@ -297,8 +304,16 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) if (!priv->dispatcher.joinable()) priv->dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AsyncAppender"), &AsyncAppender::dispatch, this ); } - bool isDispatchThread = (priv->dispatcher.get_id() == std::this_thread::get_id()); - while (true) + if (priv->dispatcher.get_id() == std::this_thread::get_id()) // From an appender attached to this? + { + auto loggerName = event->getLoggerName(); + auto iter = priv->discardMap.find(loggerName); + if (priv->discardMap.end() == iter) + priv->discardMap.emplace(loggerName, DiscardSummary{ event, LOG4CXX_STR("from an attached appender") }); + else + iter->second.add(event); + } + else while (true) { auto pendingCount = priv->eventCount - priv->dispatchedCount; if (0 <= pendingCount && pendingCount < priv->bufferSize) @@ -307,7 +322,7 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) auto oldEventCount = priv->eventCount++; auto index = oldEventCount % priv->buffer.size(); // Wait for a free slot - while (priv->bufferSize <= oldEventCount - priv->dispatchedCount && !isDispatchThread) + while (priv->bufferSize <= oldEventCount - priv->dispatchedCount) std::this_thread::yield(); // Allow the dispatch thread to free a slot // Write to the ring buffer priv->buffer[index] = AsyncAppenderPriv::EventData{event, pendingCount}; @@ -335,8 +350,7 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) bool discard = true; if (priv->blocking - && !priv->closed - && !isDispatchThread) + && !priv->closed) { ++priv->blockedCount; priv->bufferNotFull.wait(lock, [this]() @@ -358,12 +372,11 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) if (iter == priv->discardMap.end()) { - DiscardSummary summary(event); - priv->discardMap.insert(DiscardMap::value_type(loggerName, summary)); + priv->discardMap.emplace(loggerName, DiscardSummary{ event, LOG4CXX_STR("due to a full event buffer") }); } else { - (*iter).second.add(event); + iter->second.add(event); } break; @@ -466,44 +479,39 @@ bool AsyncAppender::getBlocking() const return priv->blocking; } -DiscardSummary::DiscardSummary(const LoggingEventPtr& event) : - maxEvent(event), count(1) -{ -} - -DiscardSummary::DiscardSummary(const DiscardSummary& src) : - maxEvent(src.maxEvent), count(src.count) +DiscardSummary::DiscardSummary(const LoggingEventPtr& event, const LogString& reasonArg) + : maxEvent(event) + , count(1) + , reason(reasonArg) { } -DiscardSummary& DiscardSummary::operator=(const DiscardSummary& src) +DiscardSummary::DiscardSummary(DiscardSummary&& other) + : maxEvent(std::move(other.maxEvent)) + , count(other.count) + , reason(std::move(other.reason)) { - maxEvent = src.maxEvent; - count = src.count; - return *this; } void DiscardSummary::add(const LoggingEventPtr& event) { - if (event->getLevel()->toInt() > maxEvent->getLevel()->toInt()) - { - maxEvent = event; - } - - count++; + if (this->maxEvent->getLevel()->toInt() < event->getLevel()->toInt()) + this->maxEvent = event; + ++this->count; } LoggingEventPtr DiscardSummary::createEvent(Pool& p) - { +{ LogString msg(LOG4CXX_STR("Discarded ")); - StringHelper::toString(count, p, msg); - msg.append(LOG4CXX_STR(" messages due to a full event buffer including: ")); - msg.append(maxEvent->getRenderedMessage()); - return std::make_shared<LoggingEvent>( - maxEvent->getLoggerName(), - maxEvent->getLevel(), - msg, - LocationInfo::getLocationUnavailable() ); + StringHelper::toString(this->count, p, msg); + msg.append(LOG4CXX_STR(" messages ") + this->reason + LOG4CXX_STR(" including: ")); + msg.append(this->maxEvent->getRenderedMessage()); + return std::make_shared<LoggingEvent> + ( this->maxEvent->getLoggerName() + , this->maxEvent->getLevel() + , msg + , LocationInfo::getLocationUnavailable() + ); } #if LOG4CXX_ABI_VERSION <= 15 @@ -563,7 +571,7 @@ void AsyncAppender::dispatch() { std::lock_guard<std::mutex> lock(priv->bufferMutex); blockedCount += priv->blockedCount; - for (auto discardItem : priv->discardMap) + for (auto& discardItem : priv->discardMap) { events.push_back(discardItem.second.createEvent(p)); discardCount += discardItem.second.getCount(); diff --git a/src/test/cpp/asyncappendertestcase.cpp b/src/test/cpp/asyncappendertestcase.cpp index f4e5d909..1053307a 100644 --- a/src/test/cpp/asyncappendertestcase.cpp +++ b/src/test/cpp/asyncappendertestcase.cpp @@ -529,8 +529,7 @@ class AsyncAppenderTestCase : public AppenderSkeletonTestCase LogLog::debug(msg); } LOGUNIT_ASSERT(12 < events.size()); - // A race condition in AsyncAppender can result in a lost message when the dispatch thread is logging events - LOGUNIT_ASSERT(10 <= eventCount[0]); + LOGUNIT_ASSERT_EQUAL(12, eventCount[0]); } #if LOG4CXX_HAS_DOMCONFIGURATOR
