This is an automated email from the ASF dual-hosted git repository.
swebb2066 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
The following commit(s) were added to refs/heads/master by this push:
new 8add61bf Improve Logger appender list robustness (#744)
8add61bf is described below
commit 8add61bf16033b3aa054816dfa20ae0be7e63dfd
Author: Stephen Webb <[email protected]>
AuthorDate: Fri Aug 28 12:26:34 2026 +1000
Improve Logger appender list robustness (#744)
---
src/main/cpp/appenderattachableimpl.cpp | 110 +++++++++++++----------------
src/main/cpp/logger.cpp | 61 ++++++++++------
src/main/include/log4cxx/helpers/lazyptr.h | 110 +++++++++++++++++++++++++++++
3 files changed, 201 insertions(+), 80 deletions(-)
diff --git a/src/main/cpp/appenderattachableimpl.cpp
b/src/main/cpp/appenderattachableimpl.cpp
index e56ac0af..8c2c0f4f 100644
--- a/src/main/cpp/appenderattachableimpl.cpp
+++ b/src/main/cpp/appenderattachableimpl.cpp
@@ -41,6 +41,13 @@ public: // ...structors
: pAppenderList{ std::make_shared<const AppenderList>(newList) }
{}
+public: // Attributes
+ /**
+ Serializes read-copy-update writers (addAppender, removeAppender, etc.)
+ so a concurrent modification is not silently lost. Readers stay
lock-free.
+ */
+ mutable std::mutex m_writeMutex;
+
public: // Accessors
AppenderListPtr getAppenders() const
{
@@ -65,11 +72,13 @@ public: // Modifiers
};
AppenderAttachableImpl::AppenderAttachableImpl()
+ : m_priv(std::make_unique<priv_data>())
{
}
#if LOG4CXX_ABI_VERSION <= 15
AppenderAttachableImpl::AppenderAttachableImpl(Pool& pool)
+ : m_priv(std::make_unique<priv_data>())
{
}
#endif
@@ -82,31 +91,24 @@ void AppenderAttachableImpl::addAppender(const AppenderPtr
newAppender)
{
if (!newAppender)
return;
- if (m_priv)
+ std::lock_guard<std::mutex> lock(m_priv->m_writeMutex);
+ auto allAppenders = m_priv->getAppenders();
+ if (allAppenders->end() == std::find(allAppenders->begin(),
allAppenders->end(), newAppender))
{
- auto allAppenders = m_priv->getAppenders();
- if (allAppenders->end() == std::find(allAppenders->begin(),
allAppenders->end(), newAppender))
- {
- auto newAppenders = *allAppenders;
- newAppenders.push_back(newAppender);
- m_priv->setAppenders(newAppenders);
- }
+ auto newAppenders = *allAppenders;
+ newAppenders.push_back(newAppender);
+ m_priv->setAppenders(newAppenders);
}
- else
- m_priv = std::make_unique<priv_data>(AppenderList{newAppender});
}
int AppenderAttachableImpl::appendLoopOnAppenders(const spi::LoggingEventPtr&
event)
{
int result = 0;
- if (m_priv)
+ auto allAppenders = m_priv->getAppenders();
+ for (auto& appender : *allAppenders)
{
- auto allAppenders = m_priv->getAppenders();
- for (auto& appender : *allAppenders)
- {
- appender->doAppend(event);
- ++result;
- }
+ appender->doAppend(event);
+ ++result;
}
return result;
}
@@ -119,25 +121,19 @@ int AppenderAttachableImpl::appendLoopOnAppenders(const
spi::LoggingEventPtr& ev
AppenderList AppenderAttachableImpl::getAllAppenders() const
{
- AppenderList result;
- if (m_priv)
- result = *m_priv->getAppenders();
- return result;
+ return *m_priv->getAppenders();
}
AppenderPtr AppenderAttachableImpl::getAppender(const LogString& name) const
{
AppenderPtr result;
- if (m_priv)
+ auto allAppenders = m_priv->getAppenders();
+ for (auto& appender : *allAppenders)
{
- auto allAppenders = m_priv->getAppenders();
- for (auto& appender : *allAppenders)
+ if (name == appender->getName())
{
- if (name == appender->getName())
- {
- result = appender;
- break;
- }
+ result = appender;
+ break;
}
}
return result;
@@ -146,7 +142,7 @@ AppenderPtr AppenderAttachableImpl::getAppender(const
LogString& name) const
bool AppenderAttachableImpl::isAttached(const AppenderPtr appender) const
{
bool result = false;
- if (m_priv && appender)
+ if (appender)
{
auto allAppenders = m_priv->getAppenders();
result = allAppenders->end() !=
std::find(allAppenders->begin(), allAppenders->end(), appender);
@@ -156,19 +152,18 @@ bool AppenderAttachableImpl::isAttached(const AppenderPtr
appender) const
void AppenderAttachableImpl::removeAllAppenders()
{
- if (m_priv)
- {
- auto allAppenders = m_priv->getAppenders();
- for (auto& appender : *allAppenders)
- appender->close();
- m_priv->setAppenders({});
- }
+ std::lock_guard<std::mutex> lock(m_priv->m_writeMutex);
+ auto allAppenders = m_priv->getAppenders();
+ for (auto& appender : *allAppenders)
+ appender->close();
+ m_priv->setAppenders({});
}
void AppenderAttachableImpl::removeAppender(const AppenderPtr appender)
{
- if (m_priv && appender)
+ if (appender)
{
+ std::lock_guard<std::mutex> lock(m_priv->m_writeMutex);
auto newAppenders = *m_priv->getAppenders();
auto pItem = std::find(newAppenders.begin(),
newAppenders.end(), appender);
if (newAppenders.end() != pItem)
@@ -181,27 +176,26 @@ void AppenderAttachableImpl::removeAppender(const
AppenderPtr appender)
void AppenderAttachableImpl::removeAppender(const LogString& name)
{
- if (m_priv)
- {
- auto newAppenders = *m_priv->getAppenders();
- auto pItem = std::find_if(newAppenders.begin(),
newAppenders.end()
- , [&name](const AppenderPtr& appender) -> bool
- {
- return name == appender->getName();
- });
- if (newAppenders.end() != pItem)
+ std::lock_guard<std::mutex> lock(m_priv->m_writeMutex);
+ auto newAppenders = *m_priv->getAppenders();
+ auto pItem = std::find_if(newAppenders.begin(), newAppenders.end()
+ , [&name](const AppenderPtr& appender) -> bool
{
- newAppenders.erase(pItem);
- m_priv->setAppenders(newAppenders);
- }
+ return name == appender->getName();
+ });
+ if (newAppenders.end() != pItem)
+ {
+ newAppenders.erase(pItem);
+ m_priv->setAppenders(newAppenders);
}
}
bool AppenderAttachableImpl::replaceAppender(const AppenderPtr& oldAppender,
const AppenderPtr& newAppender)
{
bool found = false;
- if (m_priv && oldAppender && newAppender)
+ if (oldAppender && newAppender)
{
+ std::lock_guard<std::mutex> lock(m_priv->m_writeMutex);
auto name = oldAppender->getName();
auto newAppenders = *m_priv->getAppenders();
auto pItem = std::find_if(newAppenders.begin(),
newAppenders.end()
@@ -221,15 +215,11 @@ bool AppenderAttachableImpl::replaceAppender(const
AppenderPtr& oldAppender, con
void AppenderAttachableImpl::replaceAppenders(const AppenderList& newList)
{
- if (m_priv)
- {
- auto allAppenders = m_priv->getAppenders();
- for (auto& a : *allAppenders)
- a->close();
- m_priv->setAppenders(newList);
- }
- else
- m_priv = std::make_unique<priv_data>(newList);
+ std::lock_guard<std::mutex> lock(m_priv->m_writeMutex);
+ auto allAppenders = m_priv->getAppenders();
+ for (auto& a : *allAppenders)
+ a->close();
+ m_priv->setAppenders(newList);
}
diff --git a/src/main/cpp/logger.cpp b/src/main/cpp/logger.cpp
index 1efcb34f..ef7b717a 100644
--- a/src/main/cpp/logger.cpp
+++ b/src/main/cpp/logger.cpp
@@ -25,6 +25,7 @@
#include <log4cxx/helpers/transcoder.h>
#include <log4cxx/helpers/appenderattachableimpl.h>
#include <log4cxx/helpers/exception.h>
+#include <log4cxx/helpers/lazyptr.h>
#if !defined(LOG4CXX)
#define LOG4CXX 1
#endif
@@ -70,7 +71,7 @@ struct Logger::LoggerPrivate
// Loggers need to know what Hierarchy they are in
spi::LoggerRepository* repositoryRaw;
- helpers::AppenderAttachableImpl aai;
+ LazyPtr<AppenderAttachableImpl> aai;
/** Additivity is set to true by default, that is children inherit
the appenders of their ancestors by default. If this variable is
@@ -105,7 +106,7 @@ Logger::~Logger()
void Logger::addAppender(const AppenderPtr newAppender)
{
- m_priv->aai.addAppender(newAppender);
+ m_priv->aai->addAppender(newAppender);
if (auto rep = getHierarchy())
{
rep->fireAddAppenderEvent(this, newAppender.get());
@@ -114,7 +115,9 @@ void Logger::addAppender(const AppenderPtr newAppender)
bool Logger::replaceAppender(const AppenderPtr& oldAppender, const
AppenderPtr& newAppender)
{
- bool result = m_priv->aai.replaceAppender(oldAppender, newAppender);
+ bool result = false;
+ if (auto p = m_priv->aai.get_ptr())
+ result = p->replaceAppender(oldAppender, newAppender);
if (result)
{
if (auto rep = getHierarchy())
@@ -125,7 +128,7 @@ bool Logger::replaceAppender(const AppenderPtr&
oldAppender, const AppenderPtr&
void Logger::replaceAppenders( const AppenderList& newList)
{
- m_priv->aai.replaceAppenders(newList);
+ m_priv->aai->replaceAppenders(newList);
if (auto rep = getHierarchy())
{
@@ -149,7 +152,8 @@ void Logger::callAppenders(const spi::LoggingEventPtr&
event) const
logger != 0;
logger = logger->m_priv->parent.get())
{
- writes += logger->m_priv->aai.appendLoopOnAppenders(event);
+ if (auto p = logger->m_priv->aai.get_ptr())
+ writes += p->appendLoopOnAppenders(event);
if (!logger->m_priv->additive)
{
@@ -304,12 +308,18 @@ bool Logger::getAdditivity() const
AppenderList Logger::getAllAppenders() const
{
- return m_priv->aai.getAllAppenders();
+ AppenderList result;
+ if (auto p = m_priv->aai.get_ptr())
+ result = p->getAllAppenders();
+ return result;
}
AppenderPtr Logger::getAppender(const LogString& name1) const
{
- return m_priv->aai.getAppender(name1);
+ AppenderPtr result;
+ if (auto p = m_priv->aai.get_ptr())
+ result = p->getAppender(name1);
+ return result;
}
const LevelPtr& Logger::getEffectiveLevel() const
@@ -399,7 +409,10 @@ const LevelPtr& Logger::getLevel() const
bool Logger::isAttached(const AppenderPtr appender) const
{
- return m_priv->aai.isAttached(appender);
+ bool result{ false };
+ if (auto p = m_priv->aai.get_ptr())
+ result = p->isAttached(appender);
+ return result;
}
bool Logger::isThresholdEqualTo(const LevelPtr& level) const
@@ -609,31 +622,39 @@ void Logger::l7dlog(const LevelPtr& level1, const
std::string& key,
void Logger::removeAllAppenders()
{
- AppenderList currentAppenders = m_priv->aai.getAllAppenders();
- m_priv->aai.removeAllAppenders();
+ if (auto p = m_priv->aai.get_ptr())
+ {
+ auto currentAppenders = p->getAllAppenders();
+ p->removeAllAppenders();
- auto rep = getHierarchy();
- if(rep){
- for(AppenderPtr appender : currentAppenders){
- rep->fireRemoveAppenderEvent(this, appender.get());
+ auto rep = getHierarchy();
+ if(rep){
+ for(AppenderPtr appender : currentAppenders){
+ rep->fireRemoveAppenderEvent(this,
appender.get());
+ }
}
}
}
void Logger::removeAppender(const AppenderPtr appender)
{
- m_priv->aai.removeAppender(appender);
- if (auto rep = getHierarchy())
+ if (auto p = m_priv->aai.get_ptr())
{
- rep->fireRemoveAppenderEvent(this, appender.get());
+ p->removeAppender(appender);
+ if (auto rep = getHierarchy())
+ {
+ rep->fireRemoveAppenderEvent(this, appender.get());
+ }
}
}
void Logger::removeAppender(const LogString& name1)
{
- AppenderPtr appender = m_priv->aai.getAppender(name1);
- if(appender){
- removeAppender(appender);
+ if (auto p = m_priv->aai.get_ptr())
+ {
+ if (auto appender = p->getAppender(name1)) {
+ removeAppender(appender);
+ }
}
}
diff --git a/src/main/include/log4cxx/helpers/lazyptr.h
b/src/main/include/log4cxx/helpers/lazyptr.h
new file mode 100644
index 00000000..9fc1ad12
--- /dev/null
+++ b/src/main/include/log4cxx/helpers/lazyptr.h
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _LOG4CXX_HELPERS_LAZY_PTR_H
+#define _LOG4CXX_HELPERS_LAZY_PTR_H
+#include <atomic>
+#include <memory>
+#include <utility>
+
+namespace LOG4CXX_NS { namespace helpers
+{
+
+/// A create-on-first-use smart pointer
+template <typename T>
+class LazyPtr
+{
+private:
+ std::atomic<T*> m_ptr{nullptr};
+
+public: // ...structors
+ LazyPtr() = default;
+
+ // Clean up allocated resource on destruction
+ ~LazyPtr()
+ {
+ delete m_ptr.load(std::memory_order_relaxed);
+ }
+
+ // Prevent copying to avoid double-free errors
+ LazyPtr(const LazyPtr&) = delete;
+ LazyPtr& operator=(const LazyPtr&) = delete;
+
+ // Allow moving
+ LazyPtr(LazyPtr&& other) noexcept
+ : m_ptr(other.m_ptr.exchange(nullptr,
std::memory_order_relaxed))
+ {}
+
+public: // Operators
+ // Allow assignment
+ LazyPtr& operator=(LazyPtr&& other) noexcept
+ {
+ if (this != &other)
+ {
+ delete m_ptr.load(std::memory_order_relaxed);
+ m_ptr.store(other.m_ptr.exchange(nullptr,
std::memory_order_relaxed),
+ std::memory_order_relaxed);
+ }
+ return *this;
+ }
+
+ // Lazy initialization & dereference
+ T& operator*() { return *get_or_throw(); }
+
+ // Lazy initialization
+ T* operator->() { return get_or_throw(); }
+
+ // Explicit bool conversion
+ explicit operator bool() const noexcept
+ {
+ return m_ptr.load(std::memory_order_relaxed) != nullptr;
+ }
+
+ // The raw pointer value
+ T* get_ptr() const noexcept
+ {
+ return m_ptr.load(std::memory_order_relaxed);
+ }
+
+private: // Modifiers
+ // Lazily initialize if required
+ T* get_or_throw()
+ {
+ T* p = m_ptr.load(std::memory_order_relaxed);
+ if (!p)
+ {
+ auto* new_p = new T();
+ if (!m_ptr.compare_exchange_strong(p, new_p
+ , std::memory_order_relaxed
+ , std::memory_order_relaxed
+ )
+ )
+ {
+ delete new_p; // Lost the race
+ }
+ else
+ {
+ p = new_p;
+ }
+ }
+ return p;
+ }
+};
+
+} } // namespace LOG4CXX_NS::helpers
+
+#endif // _LOG4CXX_HELPERS_LAZY_PTR_H
