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 b452c0ea Ensure log events are sent to an appender during a
reconfiguration event (#491)
b452c0ea is described below
commit b452c0eaaef1c2d10bfca276d8776fd2995b3b85
Author: Stephen Webb <[email protected]>
AuthorDate: Fri Mar 28 09:59:34 2025 +1100
Ensure log events are sent to an appender during a reconfiguration event
(#491)
* Do not add the fallback appender to a logger without a reference to the
failed appender
---
src/main/cpp/appenderattachableimpl.cpp | 32 +++++++++++++++
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/cpp/rollingfileappender.cpp | 6 +--
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 | 48 ++++++++++++++++------
src/test/resources/input/xml/fallback1.xml | 12 +-----
src/test/resources/input/xml/fallback2.xml | 11 +----
13 files changed, 176 insertions(+), 74 deletions(-)
diff --git a/src/main/cpp/appenderattachableimpl.cpp
b/src/main/cpp/appenderattachableimpl.cpp
index ddd4cc3c..73d67355 100644
--- a/src/main/cpp/appenderattachableimpl.cpp
+++ b/src/main/cpp/appenderattachableimpl.cpp
@@ -167,4 +167,36 @@ 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())
+ {
+ *it = 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..3d5f2bef 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::debug(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/cpp/rollingfileappender.cpp
b/src/main/cpp/rollingfileappender.cpp
index f4c78ed5..954cd929 100644
--- a/src/main/cpp/rollingfileappender.cpp
+++ b/src/main/cpp/rollingfileappender.cpp
@@ -253,11 +253,9 @@ void RollingFileAppender::activateOptions(Pool& p)
FileAppender::activateOptionsInternal(p);
}
- catch (std::exception&)
+ catch (std::exception& ex)
{
- LogLog::warn(
- LogString(LOG4CXX_STR("Exception will
initializing RollingFileAppender named "))
- + getName());
+ LogLog::warn(LOG4CXX_STR("Exception activating
RollingFileAppender ") + getName(), ex);
}
}
}
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..468ab4bf 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;
@@ -71,16 +86,19 @@ public:
void test1()
{
DOMConfigurator::configure("input/xml/fallback1.xml");
- AppenderPtr appender =
root->getAppender(LOG4CXX_STR("PRIMARY"));
- FileAppenderPtr primary = log4cxx::cast<FileAppender>(appender);
- log4cxx::varia::FallbackErrorHandlerPtr eh;
- log4cxx::spi::ErrorHandlerPtr errHandle =
primary->getErrorHandler();
- eh =
log4cxx::cast<log4cxx::varia::FallbackErrorHandler>(errHandle);
+ auto appender = root->getAppender(LOG4CXX_STR("PRIMARY"));
+ auto primary = log4cxx::cast<FileAppender>(appender);
+ auto errHandle = primary->getErrorHandler();
+ auto eh =
log4cxx::cast<log4cxx::varia::FallbackErrorHandler>(errHandle);
LOGUNIT_ASSERT(eh != 0);
- 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}";
@@ -110,16 +128,22 @@ public:
void test2()
{
DOMConfigurator::configure("input/xml/fallback2.xml");
- AppenderPtr appender =
root->getAppender(LOG4CXX_STR("PRIMARY"));
- FileAppenderPtr primary = log4cxx::cast<FileAppender>(appender);
- log4cxx::varia::FallbackErrorHandlerPtr eh;
- log4cxx::spi::ErrorHandlerPtr errHandle =
primary->getErrorHandler();
- eh =
log4cxx::cast<log4cxx::varia::FallbackErrorHandler>(errHandle);
+ auto appender = root->getAppender(LOG4CXX_STR("PRIMARY"));
+ auto primary = log4cxx::cast<FileAppender>(appender);
+ auto errHandle = primary->getErrorHandler();
+ auto eh = log4cxx::cast<varia::FallbackErrorHandler>(errHandle);
LOGUNIT_ASSERT(eh != 0);
+
+ // Also check the logger named "test" for a reference to
"PRIMARY" when "PRIMARY" fails
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">
