This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch reconfiguration_synchronization in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 101cd36fd4638e7a2e99ed77737cad47b222da2e Author: Stephen Webb <[email protected]> AuthorDate: Tue Mar 25 11:49:28 2025 +1100 Ensure log events are sent to an appender during a reconfiguration event --- src/main/cpp/appenderattachableimpl.cpp | 33 ++++++++++++++++++++++ src/main/cpp/asyncappender.cpp | 10 +++++++ src/main/cpp/domconfigurator.cpp | 20 ++++++------- src/main/cpp/fallbackerrorhandler.cpp | 18 +++++------- src/main/cpp/logger.cpp | 30 ++++++++++++++------ src/main/include/log4cxx/asyncappender.h | 11 ++++++++ .../log4cxx/helpers/appenderattachableimpl.h | 11 ++++++++ src/main/include/log4cxx/logger.h | 23 +++++++++++---- src/main/include/log4cxx/spi/appenderattachable.h | 18 ++++++++++++ src/test/cpp/varia/errorhandlertestcase.cpp | 27 ++++++++++++++++-- src/test/resources/input/xml/fallback1.xml | 12 +------- src/test/resources/input/xml/fallback2.xml | 11 +------- 12 files changed, 164 insertions(+), 60 deletions(-) diff --git a/src/main/cpp/appenderattachableimpl.cpp b/src/main/cpp/appenderattachableimpl.cpp index ddd4cc3c..1f9e32a6 100644 --- a/src/main/cpp/appenderattachableimpl.cpp +++ b/src/main/cpp/appenderattachableimpl.cpp @@ -167,4 +167,37 @@ void AppenderAttachableImpl::removeAppender(const LogString& name) } } +bool AppenderAttachableImpl::replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) +{ + bool found = false; + if (m_priv && oldAppender && newAppender) + { + auto oldName = oldAppender->getName(); + std::lock_guard<std::mutex> lock( m_priv->m_mutex ); + auto it = std::find_if(m_priv->appenderList.begin(), m_priv->appenderList.end() + , [&oldName](const AppenderPtr& appender) -> bool + { + return oldName == appender->getName(); + }); + if (it != m_priv->appenderList.end()) + { + m_priv->appenderList.erase(it); + m_priv->appenderList.push_back(newAppender); + found = true; + } + } + return found; +} + +void AppenderAttachableImpl::replaceAppenders(const AppenderList& newList) +{ + auto oldAppenders = getAllAppenders(); + if (!m_priv) + m_priv = std::make_unique<AppenderAttachableImpl::priv_data>(); + std::lock_guard<std::mutex> lock( m_priv->m_mutex ); + for (auto a : oldAppenders) + a->close(); + m_priv->appenderList = newList; +} + diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 7c7caa8d..cec9f359 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -413,6 +413,16 @@ void AsyncAppender::removeAppender(const LogString& n) priv->appenders.removeAppender(n); } +bool AsyncAppender::replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) +{ + return priv->appenders.replaceAppender(oldAppender, newAppender); +} + +void AsyncAppender::replaceAppenders( const AppenderList& newList) +{ + priv->appenders.replaceAppenders(newList); +} + bool AsyncAppender::getLocationInfo() const { return priv->locationInfo; diff --git a/src/main/cpp/domconfigurator.cpp b/src/main/cpp/domconfigurator.cpp index 12648642..b8d5cc8f 100644 --- a/src/main/cpp/domconfigurator.cpp +++ b/src/main/cpp/domconfigurator.cpp @@ -535,10 +535,6 @@ void DOMConfigurator::parseChildrenOfLoggerElement( PropertySetter propSetter(logger); std::vector<AppenderPtr> newappenders; - // Remove all existing appenders from logger. They will be - // reconstructed if need be. - logger->removeAllAppenders(); - for (apr_xml_elem* currentElement = loggerElement->first_child; currentElement; currentElement = currentElement->next) @@ -550,12 +546,12 @@ void DOMConfigurator::parseChildrenOfLoggerElement( AppenderPtr appender = findAppenderByReference(p, utf8Decoder, currentElement, doc, appenders); LogString refName = subst(getAttribute(utf8Decoder, currentElement, REF_ATTR)); - if (!LogLog::isDebugEnabled()) - ; - else if (appender != 0) + if (appender) { - LogLog::debug(LOG4CXX_STR("Adding appender named [") + refName + - LOG4CXX_STR("] to logger [") + logger->getName() + LOG4CXX_STR("].")); + if (LogLog::isDebugEnabled()) + LogLog::debug(LOG4CXX_STR("Adding appender named [") + refName + + LOG4CXX_STR("] to logger [") + logger->getName() + LOG4CXX_STR("].")); + newappenders.push_back(appender); } else { @@ -563,8 +559,6 @@ void DOMConfigurator::parseChildrenOfLoggerElement( LOG4CXX_STR("] not found.")); } - logger->addAppender(appender); - } else if (tagName == LEVEL_TAG) { @@ -579,6 +573,10 @@ void DOMConfigurator::parseChildrenOfLoggerElement( setParameter(p, utf8Decoder, currentElement, propSetter); } } + if (newappenders.empty()) + logger->removeAllAppenders(); + else + logger->replaceAppenders(newappenders); propSetter.activate(p); } diff --git a/src/main/cpp/fallbackerrorhandler.cpp b/src/main/cpp/fallbackerrorhandler.cpp index 0b5f7d93..bed0530e 100644 --- a/src/main/cpp/fallbackerrorhandler.cpp +++ b/src/main/cpp/fallbackerrorhandler.cpp @@ -87,22 +87,18 @@ void FallbackErrorHandler::error(const LogString& message, { if (LogLog::isDebugEnabled()) { - LogLog::debug(((LogString) LOG4CXX_STR("FB: Searching for [")) - + primaryLocked->getName() + LOG4CXX_STR("] in logger [") - + l->getName() + LOG4CXX_STR("].")); - LogLog::debug(((LogString) LOG4CXX_STR("FB: Replacing [")) - + primaryLocked->getName() + LOG4CXX_STR("] by [") + LogLog::debug(LOG4CXX_STR("FB: Replacing [") + + primaryLocked->getName() + LOG4CXX_STR("] with [") + backupLocked->getName() + LOG4CXX_STR("] in logger [") + l->getName() + LOG4CXX_STR("].")); } - l->removeAppender(primaryLocked); - if (LogLog::isDebugEnabled()) + if (!l->replaceAppender(primaryLocked, backupLocked)) { - LogLog::debug(((LogString) LOG4CXX_STR("FB: Adding appender [")) - + backupLocked->getName() + LOG4CXX_STR("] to logger ") - + l->getName()); + LogLog::warn(LOG4CXX_STR("FB: Failed to replace [") + + primaryLocked->getName() + LOG4CXX_STR("] with [") + + backupLocked->getName() + LOG4CXX_STR("] in logger [") + + l->getName() + LOG4CXX_STR("].")); } - l->addAppender(backupLocked); } m_priv->errorReported = true; } diff --git a/src/main/cpp/logger.cpp b/src/main/cpp/logger.cpp index 75781453..e814620f 100644 --- a/src/main/cpp/logger.cpp +++ b/src/main/cpp/logger.cpp @@ -112,23 +112,35 @@ void Logger::addAppender(const AppenderPtr newAppender) } } -void Logger::reconfigure( const std::vector<AppenderPtr>& appenders, bool additive1 ) +bool Logger::replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) { - m_priv->additive = additive1; + bool result = m_priv->aai.replaceAppender(oldAppender, newAppender); + if (result) + { + if (auto rep = getHierarchy()) + rep->fireAddAppenderEvent(this, newAppender.get()); + } + return result; +} - m_priv->aai.removeAllAppenders(); +void Logger::replaceAppenders( const AppenderList& newList) +{ + m_priv->aai.replaceAppenders(newList); - for (auto const& item : appenders) + if (auto rep = getHierarchy()) { - m_priv->aai.addAppender(item); - - if (auto rep = getHierarchy()) - { + for (auto const& item : newList) rep->fireAddAppenderEvent(this, item.get()); - } } } +void Logger::reconfigure( const AppenderList& newList, bool newAdditivity ) +{ + m_priv->additive = newAdditivity; + + replaceAppenders(newList); +} + void Logger::callAppenders(const spi::LoggingEventPtr& event, Pool& p) const { int writes = 0; diff --git a/src/main/include/log4cxx/asyncappender.h b/src/main/include/log4cxx/asyncappender.h index b957d03a..e1ea6c8e 100644 --- a/src/main/include/log4cxx/asyncappender.h +++ b/src/main/include/log4cxx/asyncappender.h @@ -152,6 +152,17 @@ class LOG4CXX_EXPORT AsyncAppender : */ void removeAppender(const LogString& name) override; + /** + * Replace \c oldAppender with \c newAppender. + * @return true if oldAppender was replaced with newAppender. + */ + bool replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) LOG4CXX_16_VIRTUAL_SPECIFIER; + + /** + * Replace any previously added appenders with \c newList. + */ + void replaceAppenders(const AppenderList& newList) LOG4CXX_16_VIRTUAL_SPECIFIER; + /** * The <b>LocationInfo</b> attribute is provided for compatibility * with log4j and has no effect on the log output. diff --git a/src/main/include/log4cxx/helpers/appenderattachableimpl.h b/src/main/include/log4cxx/helpers/appenderattachableimpl.h index 73c370ec..289512e9 100644 --- a/src/main/include/log4cxx/helpers/appenderattachableimpl.h +++ b/src/main/include/log4cxx/helpers/appenderattachableimpl.h @@ -101,6 +101,17 @@ class LOG4CXX_EXPORT AppenderAttachableImpl : */ void removeAppender(const LogString& name) override; + /** + * Replace \c oldAppender with \c newAppender. + * @return true if oldAppender was replaced with newAppender. + */ + bool replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) LOG4CXX_16_VIRTUAL_SPECIFIER; + + /** + * Replace any previously added appenders with \c newList. + */ + void replaceAppenders(const AppenderList& newList) LOG4CXX_16_VIRTUAL_SPECIFIER; + private: LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(priv_data, m_priv) diff --git a/src/main/include/log4cxx/logger.h b/src/main/include/log4cxx/logger.h index f16aaf58..91181504 100644 --- a/src/main/include/log4cxx/logger.h +++ b/src/main/include/log4cxx/logger.h @@ -788,8 +788,7 @@ class LOG4CXX_EXPORT Logger Return the the LoggerRepository where this <code>Logger</code> is attached. */ - LOG4CXX_NS::spi::LoggerRepository* getLoggerRepository() const; - + spi::LoggerRepository* getLoggerRepository() const; /** * Get the logger name. @@ -1791,6 +1790,17 @@ class LOG4CXX_EXPORT Logger */ void removeAppender(const LogString& name) override; + /** + * Replace \c oldAppender with \c newAppender. + * @return true if oldAppender was replaced with newAppender. + */ + bool replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) LOG4CXX_16_VIRTUAL_SPECIFIER; + + /** + * Replace all previously added appenders with \c newList. + */ + void replaceAppenders(const AppenderList& newList) LOG4CXX_16_VIRTUAL_SPECIFIER; + /** Set the additivity flag for this logger. */ @@ -2095,12 +2105,13 @@ class LOG4CXX_EXPORT Logger void trace(const std::string& msg) const; /** - * Reconfigure this logger by configuring all of the appenders. + * Replace all current appenders with \c newList and + * set the additivity flag to \c newAdditivity. * - * @param appenders The appenders to set. Any currently existing appenders are removed. - * @param additivity The additivity of this logger + * @param newList The appenders to set. + * @param newAdditivity Whether this logger should send events to its parent. */ - void reconfigure( const std::vector<AppenderPtr>& appenders, bool additivity ); + void reconfigure( const AppenderList& newList, bool newAdditivity ); private: // diff --git a/src/main/include/log4cxx/spi/appenderattachable.h b/src/main/include/log4cxx/spi/appenderattachable.h index 93f77605..2498dc96 100644 --- a/src/main/include/log4cxx/spi/appenderattachable.h +++ b/src/main/include/log4cxx/spi/appenderattachable.h @@ -72,6 +72,19 @@ class LOG4CXX_EXPORT AppenderAttachable : public virtual helpers::Object */ virtual void removeAppender(const LogString& name) = 0; +#if 15 < LOG4CXX_ABI_VERSION + /** + * Replace \c oldAppender with \c newAppender. + * @return true if oldAppender was replaced with newAppender. + */ + virtual bool replaceAppender(const AppenderPtr& oldAppender, const AppenderPtr& newAppender) = 0; + + /** + * Replace any previously added appenders with \c newList. + */ + virtual void replaceAppenders(const AppenderList& newList) = 0; +#endif + // Dtor virtual ~AppenderAttachable() {} }; @@ -80,5 +93,10 @@ LOG4CXX_PTR_DEF(AppenderAttachable); } } +#if 15 < LOG4CXX_ABI_VERSION +#define LOG4CXX_16_VIRTUAL_SPECIFIER override +#else +#define LOG4CXX_16_VIRTUAL_SPECIFIER +#endif #endif //_LOG4CXX_SPI_APPENDER_ATTACHABLE_H_ diff --git a/src/test/cpp/varia/errorhandlertestcase.cpp b/src/test/cpp/varia/errorhandlertestcase.cpp index c8cf517f..223cc10d 100644 --- a/src/test/cpp/varia/errorhandlertestcase.cpp +++ b/src/test/cpp/varia/errorhandlertestcase.cpp @@ -21,6 +21,7 @@ #include <log4cxx/varia/fallbackerrorhandler.h> #include <log4cxx/appender.h> #include <log4cxx/helpers/loglog.h> +#include <log4cxx/helpers/pool.h> #include "../logunit.h" #include "../util/transformer.h" #include "../util/compare.h" @@ -28,6 +29,20 @@ #include "../util/linenumberfilter.h" #include <iostream> +namespace +{ + /* + The following path is carefully designed to fail on Linux and Windows, + so that the appender FALLBACK is used instead and the test succeeds in + the end. Linux considers "." as current directory, which can not be + created as a file, while Windows strips "/."[1] internally and fails at + ":", which is an invalid name. + + [1]: https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations + */ + log4cxx::LogString BadPath{ LOG4CXX_STR("output/xyz/:/.") }; +} + using namespace log4cxx; using namespace log4cxx::helpers; using namespace log4cxx::xml; @@ -77,9 +92,12 @@ public: log4cxx::spi::ErrorHandlerPtr errHandle = primary->getErrorHandler(); eh = log4cxx::cast<log4cxx::varia::FallbackErrorHandler>(errHandle); LOGUNIT_ASSERT(eh != 0); + primary->setOption(LOG4CXX_STR("FILE"), BadPath); + Pool p; + primary->activateOptions(p); + LOGUNIT_ASSERT(eh->errorReported()); common(); - LOGUNIT_ASSERT(eh->errorReported()); std::string TEST1_PAT = "FALLBACK - (root|test) - Message {0-9}"; @@ -117,9 +135,14 @@ public: eh = log4cxx::cast<log4cxx::varia::FallbackErrorHandler>(errHandle); LOGUNIT_ASSERT(eh != 0); eh->setLogger(logger); - common(); + + primary->setOption(LOG4CXX_STR("FILE"), BadPath); + Pool p; + primary->activateOptions(p); LOGUNIT_ASSERT(eh->errorReported()); + common(); + std::string TEST1_PAT = "FALLBACK - (root|test) - Message {0-9}"; diff --git a/src/test/resources/input/xml/fallback1.xml b/src/test/resources/input/xml/fallback1.xml index 83ed810e..5b21911c 100644 --- a/src/test/resources/input/xml/fallback1.xml +++ b/src/test/resources/input/xml/fallback1.xml @@ -26,17 +26,7 @@ <root-ref/> <appender-ref ref="FALLBACK" /> </errorHandler> - - <!-- - The following path is carefully designed to fail on Linux and Windows, - so that the appender FALLBACK is used instead and the test succeeds in - the end. Linux considers "." as current directory, which can not be - created as a file, while Windows strips "/."[1] internally and fails at - ":", which is an invalid name. - - [1]: https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations - --> - <param name="File" value="output/xyz/:/." /> + <param name="File" value="output/xyz/x.log" /> <param name="Append" value="false" /> <layout class="org.apache.log4j.PatternLayout"> diff --git a/src/test/resources/input/xml/fallback2.xml b/src/test/resources/input/xml/fallback2.xml index e0f37003..4fc6e5bc 100644 --- a/src/test/resources/input/xml/fallback2.xml +++ b/src/test/resources/input/xml/fallback2.xml @@ -27,16 +27,7 @@ <appender-ref ref="FALLBACK" /> </errorHandler> - <!-- - The following path is carefully designed to fail on Linux and Windows, - so that the appender FALLBACK is used instead and the test succeeds in - the end. Linux considers "." as current directory, which can not be - created as a file, while Windows strips "/."[1] internally and fails at - ":", which is an invalid name. - - [1]: https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations - --> - <param name="File" value="output/xyz/:/." /> + <param name="File" value="output/xyz/x.log" /> <param name="Append" value="false" /> <layout class="org.apache.log4j.PatternLayout">
