This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch lazy_ptr_clarity in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit dbab33e59a1321569f8c404e4be9e4380e23a4a1 Author: Stephen Webb <[email protected]> AuthorDate: Thu Aug 27 16:02:42 2026 +1000 Improve Logger appender list robustness --- 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
