This is an automated email from the ASF dual-hosted git repository.
swebb2066 pushed a commit to branch next_stable
in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
The following commit(s) were added to refs/heads/next_stable by this push:
new 1ba97dfa APR termination must be after destruction of all log4cxx
objects (#122)
1ba97dfa is described below
commit 1ba97dfa01b9b1a73a4a9f032138c72a8a49511c
Author: Stephen Webb <[email protected]>
AuthorDate: Sat Sep 10 12:12:11 2022 +1000
APR termination must be after destruction of all log4cxx objects (#122)
* Make APR initialization/termination book-end all log4cxx object life
times.
As the APRInitializer class has member variables whose destructors call
apr_* functions, apr_terminate() cannot be called
the the APRInitializer class destructor.
* Delay heirarchy destruction until other threads have exited doAppend()
* Added another test for the fallback handling to ensure that multiple
appenders on the same logger will not segfault if replaced while iterating.
* Stop using APR on other threads once exit() has been called
Co-authored-by: Robert Middleton <[email protected]>
---
src/main/cpp/appenderattachableimpl.cpp | 43 +++++++-----------
src/main/cpp/aprinitializer.cpp | 18 ++++++--
src/main/cpp/asyncappender.cpp | 2 -
src/main/cpp/asyncappender_nonblocking.cpp | 12 -----
src/main/cpp/hierarchy.cpp | 4 ++
src/main/cpp/logger.cpp | 24 ++++++++--
.../log4cxx/helpers/appenderattachableimpl.h | 2 -
src/main/include/log4cxx/logger.h | 5 --
src/test/cpp/varia/errorhandlertestcase.cpp | 53 ++++++++++++++++++++--
src/test/resources/input/xml/fallback1.xml | 2 +-
.../input/xml/{fallback1.xml => fallback2.xml} | 9 +++-
11 files changed, 115 insertions(+), 59 deletions(-)
diff --git a/src/main/cpp/appenderattachableimpl.cpp
b/src/main/cpp/appenderattachableimpl.cpp
index 3de8ccf4..d936d9c6 100644
--- a/src/main/cpp/appenderattachableimpl.cpp
+++ b/src/main/cpp/appenderattachableimpl.cpp
@@ -32,7 +32,7 @@ struct AppenderAttachableImpl::priv_data
{
/** Array of appenders. */
AppenderList appenderList;
- mutable std::mutex m_mutex;
+ mutable std::recursive_mutex m_mutex;
};
@@ -54,7 +54,7 @@ void AppenderAttachableImpl::addAppender(const AppenderPtr
newAppender)
return;
}
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
AppenderList::iterator it = std::find(
m_priv->appenderList.begin(),
m_priv->appenderList.end(), newAppender);
@@ -68,24 +68,16 @@ int AppenderAttachableImpl::appendLoopOnAppenders(
const spi::LoggingEventPtr& event,
Pool& p)
{
-
- AppenderList allAppenders;
int numberAppended = 0;
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
+ // FallbackErrorHandler::error() may modify our list of appenders
+ // while we are iterating over them (if it holds the same logger).
+ // So, make a local copy of the appenders that we want to iterate over
+ // before actually iterating over them.
+ AppenderList allAppenders = m_priv->appenderList;
+ for (auto appender : allAppenders)
{
- // There are occasions where we want to modify our list of
appenders
- // while we are iterating over them. For example, if one of the
- // appenders fails, we may want to swap it out for a new one.
- // So, make a local copy of the appenders that we want to
iterate over
- // before actually iterating over them.
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
- allAppenders = m_priv->appenderList;
- }
-
- for (AppenderList::iterator it = allAppenders.begin();
- it != allAppenders.end();
- it++)
- {
- (*it)->doAppend(event, p);
+ appender->doAppend(event, p);
numberAppended++;
}
@@ -94,6 +86,7 @@ int AppenderAttachableImpl::appendLoopOnAppenders(
AppenderList AppenderAttachableImpl::getAllAppenders() const
{
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
return m_priv->appenderList;
}
@@ -104,7 +97,7 @@ AppenderPtr AppenderAttachableImpl::getAppender(const
LogString& name) const
return 0;
}
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
AppenderList::const_iterator it, itEnd = m_priv->appenderList.end();
AppenderPtr appender;
@@ -128,7 +121,7 @@ bool AppenderAttachableImpl::isAttached(const AppenderPtr
appender) const
return false;
}
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
AppenderList::const_iterator it = std::find(
m_priv->appenderList.begin(),
m_priv->appenderList.end(), appender);
@@ -137,7 +130,7 @@ bool AppenderAttachableImpl::isAttached(const AppenderPtr
appender) const
void AppenderAttachableImpl::removeAllAppenders()
{
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
AppenderList::iterator it, itEnd = m_priv->appenderList.end();
AppenderPtr a;
@@ -157,7 +150,7 @@ void AppenderAttachableImpl::removeAppender(const
AppenderPtr appender)
return;
}
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
AppenderList::iterator it = std::find(
m_priv->appenderList.begin(),
m_priv->appenderList.end(), appender);
@@ -174,7 +167,7 @@ void AppenderAttachableImpl::removeAppender(const
LogString& name)
return;
}
- std::unique_lock<std::mutex> lock( m_priv->m_mutex );
+ std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex );
AppenderList::iterator it, itEnd = m_priv->appenderList.end();
AppenderPtr appender;
@@ -190,8 +183,4 @@ void AppenderAttachableImpl::removeAppender(const
LogString& name)
}
}
-std::mutex& AppenderAttachableImpl::getMutex()
-{
- return m_priv->m_mutex;
-}
diff --git a/src/main/cpp/aprinitializer.cpp b/src/main/cpp/aprinitializer.cpp
index 6e4214c4..47f64198 100644
--- a/src/main/cpp/aprinitializer.cpp
+++ b/src/main/cpp/aprinitializer.cpp
@@ -39,11 +39,24 @@ extern "C" void tlsDestruct(void* ptr)
{
delete ((ThreadSpecificData*) ptr);
}
+
+// The first object created and the last object destroyed
+struct apr_environment
+{
+ apr_environment()
+ {
+ apr_initialize();
+ }
+ ~apr_environment()
+ {
+ apr_terminate();
+ }
+};
+
}
APRInitializer::APRInitializer() : p(0), startTime(0), tlsKey(0)
{
- apr_initialize();
apr_pool_create(&p, NULL);
apr_atomic_init(p);
apr_atomic_init(p);
@@ -62,8 +75,6 @@ APRInitializer::~APRInitializer()
#if APR_HAS_THREADS
std::unique_lock<std::mutex> lock(mutex);
apr_threadkey_private_delete(tlsKey);
-#else
- apr_terminate();
#endif
}
@@ -87,6 +98,7 @@ void APRInitializer::unregisterAll()
APRInitializer& APRInitializer::getInstance()
{
+ static apr_environment env;
static APRInitializer init;
return init;
}
diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp
index 23100830..2a0cf68a 100644
--- a/src/main/cpp/asyncappender.cpp
+++ b/src/main/cpp/asyncappender.cpp
@@ -214,7 +214,6 @@ void AsyncAppender::append(const spi::LoggingEventPtr&
event, Pool& p)
//
if (!priv->dispatcher.joinable() || priv->bufferSize <= 0)
{
- std::unique_lock<std::mutex> lock(priv->appenders->getMutex());
priv->appenders->appendLoopOnAppenders(event, p);
return;
}
@@ -316,7 +315,6 @@ void AsyncAppender::close()
}
{
- std::unique_lock<std::mutex> lock(priv->appenders->getMutex());
AppenderList appenderList = priv->appenders->getAllAppenders();
for (AppenderList::iterator iter = appenderList.begin();
diff --git a/src/main/cpp/asyncappender_nonblocking.cpp
b/src/main/cpp/asyncappender_nonblocking.cpp
index 6c28756e..cceceba9 100644
--- a/src/main/cpp/asyncappender_nonblocking.cpp
+++ b/src/main/cpp/asyncappender_nonblocking.cpp
@@ -28,7 +28,6 @@
#include <apr_thread_mutex.h>
#include <apr_thread_cond.h>
#include <log4cxx/helpers/condition.h>
-#include <log4cxx/helpers/synchronized.h>
#include <log4cxx/helpers/stringhelper.h>
#include <apr_atomic.h>
#include <log4cxx/helpers/optionconverter.h>
@@ -78,7 +77,6 @@ void AsyncAppender::releaseRef() const
void AsyncAppender::addAppender(const AppenderPtr& newAppender)
{
- synchronized sync(appenders->getMutex());
appenders->addAppender(newAppender);
}
@@ -124,7 +122,6 @@ void AsyncAppender::append(const spi::LoggingEventPtr&
event, Pool& p)
//
if (!dispatcher.isAlive() || bufferSize <= 0)
{
- synchronized sync(appenders->getMutex());
appenders->appendLoopOnAppenders(event, p);
return;
}
@@ -196,7 +193,6 @@ void AsyncAppender::append(const spi::LoggingEventPtr&
event, Pool& p)
}
}
#else
- synchronized sync(appenders->getMutex());
appenders->appendLoopOnAppenders(event, p);
#endif
}
@@ -227,7 +223,6 @@ void AsyncAppender::close()
#endif
{
- synchronized sync(appenders->getMutex());
AppenderList appenderList = appenders->getAllAppenders();
for (AppenderList::iterator iter = appenderList.begin();
@@ -241,19 +236,16 @@ void AsyncAppender::close()
AppenderList AsyncAppender::getAllAppenders() const
{
- synchronized sync(appenders->getMutex());
return appenders->getAllAppenders();
}
AppenderPtr AsyncAppender::getAppender(const LogString& n) const
{
- synchronized sync(appenders->getMutex());
return appenders->getAppender(n);
}
bool AsyncAppender::isAttached(const AppenderPtr& appender) const
{
- synchronized sync(appenders->getMutex());
return appenders->isAttached(appender);
}
@@ -264,19 +256,16 @@ bool AsyncAppender::requiresLayout() const
void AsyncAppender::removeAllAppenders()
{
- synchronized sync(appenders->getMutex());
appenders->removeAllAppenders();
}
void AsyncAppender::removeAppender(const AppenderPtr& appender)
{
- synchronized sync(appenders->getMutex());
appenders->removeAppender(appender);
}
void AsyncAppender::removeAppender(const LogString& n)
{
- synchronized sync(appenders->getMutex());
appenders->removeAppender(n);
}
@@ -429,7 +418,6 @@ void* LOG4CXX_THREAD_FUNC
AsyncAppender::dispatch(apr_thread_t* /*thread*/, void
iter != events.end();
iter++)
{
- synchronized sync(pThis->appenders->getMutex());
pThis->appenders->appendLoopOnAppenders(*iter,
p);
}
}
diff --git a/src/main/cpp/hierarchy.cpp b/src/main/cpp/hierarchy.cpp
index 8c399067..1e38efdd 100644
--- a/src/main/cpp/hierarchy.cpp
+++ b/src/main/cpp/hierarchy.cpp
@@ -96,9 +96,13 @@ Hierarchy::~Hierarchy()
for (auto& item : m_priv->loggers)
{
if (auto& pLogger = item.second)
+ {
pLogger->removeHierarchy();
+ pLogger->removeAllAppenders();
+ }
}
m_priv->root->removeHierarchy();
+ m_priv->root->removeAllAppenders();
}
void Hierarchy::addHierarchyEventListener(const
spi::HierarchyEventListenerPtr& listener)
diff --git a/src/main/cpp/logger.cpp b/src/main/cpp/logger.cpp
index e3db0871..361f51bd 100644
--- a/src/main/cpp/logger.cpp
+++ b/src/main/cpp/logger.cpp
@@ -101,7 +101,7 @@ void Logger::addAppender(const AppenderPtr newAppender)
m_priv->aai.addAppender(newAppender);
if (auto rep = getHierarchy())
{
- m_priv->repositoryRaw->fireAddAppenderEvent(this,
newAppender.get());
+ rep->fireAddAppenderEvent(this, newAppender.get());
}
}
@@ -119,7 +119,7 @@ void Logger::reconfigure( const std::vector<AppenderPtr>&
appenders, bool additi
if (auto rep = getHierarchy())
{
- m_priv->repositoryRaw->fireAddAppenderEvent(this,
it->get());
+ rep->fireAddAppenderEvent(this, it->get());
}
}
}
@@ -144,7 +144,7 @@ void Logger::callAppenders(const spi::LoggingEventPtr&
event, Pool& p) const
if (writes == 0 && rep)
{
-
m_priv->repositoryRaw->emitNoAppenderWarning(const_cast<Logger*>(this));
+ rep->emitNoAppenderWarning(const_cast<Logger*>(this));
}
}
@@ -162,6 +162,8 @@ void Logger::closeNestedAppenders()
void Logger::forcedLog(const LevelPtr& level1, const std::string& message,
const LocationInfo& location) const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_CHAR(msg, message);
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, msg,
location));
@@ -171,6 +173,8 @@ void Logger::forcedLog(const LevelPtr& level1, const
std::string& message,
void Logger::forcedLog(const LevelPtr& level1, const std::string& message)
const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_CHAR(msg, message);
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, msg,
@@ -181,6 +185,8 @@ void Logger::forcedLog(const LevelPtr& level1, const
std::string& message) const
void Logger::forcedLogLS(const LevelPtr& level1, const LogString& message,
const LocationInfo& location) const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, message,
location));
callAppenders(event, p);
@@ -694,6 +700,8 @@ LoggerPtr Logger::getLoggerLS(const LogString& name)
void Logger::forcedLog(const LevelPtr& level1, const std::wstring& message,
const LocationInfo& location) const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_WCHAR(msg, message);
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, msg,
location));
@@ -702,6 +710,8 @@ void Logger::forcedLog(const LevelPtr& level1, const
std::wstring& message,
void Logger::forcedLog(const LevelPtr& level1, const std::wstring& message)
const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_WCHAR(msg, message);
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, msg,
@@ -845,6 +855,8 @@ void Logger::warn(const std::wstring& msg) const
void Logger::forcedLog(const LevelPtr& level1, const
std::basic_string<UniChar>& message,
const LocationInfo& location) const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_UNICHAR(msg, message);
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, msg,
location));
@@ -853,6 +865,8 @@ void Logger::forcedLog(const LevelPtr& level1, const
std::basic_string<UniChar>&
void Logger::forcedLog(const LevelPtr& level1, const
std::basic_string<UniChar>& message) const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_UNICHAR(msg, message);
LoggingEventPtr event(new LoggingEvent(m_priv->name, level1, msg,
@@ -993,6 +1007,8 @@ void Logger::warn(const std::basic_string<UniChar>& msg)
const
void Logger::forcedLog(const LevelPtr& level1, const CFStringRef& message,
const LocationInfo& location) const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_CFSTRING(msg, message);
LoggingEventPtr event(new LoggingEvent(name, level1, msg, location));
@@ -1001,6 +1017,8 @@ void Logger::forcedLog(const LevelPtr& level1, const
CFStringRef& message,
void Logger::forcedLog(const LevelPtr& level1, const CFStringRef& message)
const
{
+ if (!getHierarchy()) // Has removeHierarchy() been called?
+ return;
Pool p;
LOG4CXX_DECODE_CFSTRING(msg, message);
LoggingEventPtr event(new LoggingEvent(name, level1, msg,
diff --git a/src/main/include/log4cxx/helpers/appenderattachableimpl.h
b/src/main/include/log4cxx/helpers/appenderattachableimpl.h
index b2e39b00..2317d3a9 100644
--- a/src/main/include/log4cxx/helpers/appenderattachableimpl.h
+++ b/src/main/include/log4cxx/helpers/appenderattachableimpl.h
@@ -102,8 +102,6 @@ class LOG4CXX_EXPORT AppenderAttachableImpl :
*/
virtual void removeAppender(const LogString& name);
- std::mutex& getMutex();
-
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 2f56de12..e4a35408 100644
--- a/src/main/include/log4cxx/logger.h
+++ b/src/main/include/log4cxx/logger.h
@@ -28,11 +28,6 @@
namespace log4cxx
{
-namespace helpers
-{
-class synchronized;
-}
-
namespace spi
{
class LoggerRepository;
diff --git a/src/test/cpp/varia/errorhandlertestcase.cpp
b/src/test/cpp/varia/errorhandlertestcase.cpp
index e86972d1..4f4007e1 100644
--- a/src/test/cpp/varia/errorhandlertestcase.cpp
+++ b/src/test/cpp/varia/errorhandlertestcase.cpp
@@ -20,6 +20,7 @@
#include <log4cxx/fileappender.h>
#include <log4cxx/varia/fallbackerrorhandler.h>
#include <log4cxx/appender.h>
+#include <log4cxx/helpers/loglog.h>
#include "../logunit.h"
#include "../util/transformer.h"
#include "../util/compare.h"
@@ -35,11 +36,19 @@ LOGUNIT_CLASS(ErrorHandlerTestCase)
{
LOGUNIT_TEST_SUITE(ErrorHandlerTestCase);
LOGUNIT_TEST(test1);
+ LOGUNIT_TEST(test2);
LOGUNIT_TEST_SUITE_END();
LoggerPtr root;
LoggerPtr logger;
-
+#ifdef _DEBUG
+ struct Fixture
+ {
+ Fixture() {
+ helpers::LogLog::setInternalDebugging(true);
+ }
+ } suiteFixture;
+#endif
public:
void setUp()
@@ -85,7 +94,45 @@ public:
try
{
- Transformer::transform("output/fallback",
"output/fallbackfiltered", filters);
+ Transformer::transform("output/fallback1",
"output/fallbackfiltered1", filters);
+ }
+ catch (UnexpectedFormatException& e)
+ {
+ std::cout << "UnexpectedFormatException :" << e.what()
<< std::endl;
+ throw;
+ }
+
+
+ LOGUNIT_ASSERT(Compare::compare("output/fallbackfiltered1",
"witness/fallback1"));
+ }
+
+ 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);
+ LOGUNIT_ASSERT(eh != 0);
+ eh->setLogger(logger);
+ common();
+
+ std::string TEST1_PAT =
+ "FALLBACK - (root|test) - Message {0-9}";
+
+ ControlFilter cf;
+ cf << TEST1_PAT;
+
+ LineNumberFilter lineNumberFilter;
+
+ std::vector<Filter*> filters;
+ filters.push_back(&cf);
+ filters.push_back(&lineNumberFilter);
+
+ try
+ {
+ Transformer::transform("output/fallback2",
"output/fallbackfiltered2", filters);
}
catch (UnexpectedFormatException& e)
{
@@ -94,7 +141,7 @@ public:
}
- LOGUNIT_ASSERT(Compare::compare("output/fallbackfiltered",
"witness/fallback1"));
+ LOGUNIT_ASSERT(Compare::compare("output/fallbackfiltered2",
"witness/fallback1"));
}
void common()
diff --git a/src/test/resources/input/xml/fallback1.xml
b/src/test/resources/input/xml/fallback1.xml
index 90072a31..83ed810e 100644
--- a/src/test/resources/input/xml/fallback1.xml
+++ b/src/test/resources/input/xml/fallback1.xml
@@ -45,7 +45,7 @@
</appender>
<appender name="FALLBACK" class="org.apache.log4j.FileAppender">
- <param name="File" value="output/fallback" />
+ <param name="File" value="output/fallback1" />
<param name="Append" value="false" />
<layout class="org.apache.log4j.PatternLayout">
<param name="ConversionPattern" value="FALLBACK - %c - %m%n"/>
diff --git a/src/test/resources/input/xml/fallback1.xml
b/src/test/resources/input/xml/fallback2.xml
similarity index 86%
copy from src/test/resources/input/xml/fallback1.xml
copy to src/test/resources/input/xml/fallback2.xml
index 90072a31..e0f37003 100644
--- a/src/test/resources/input/xml/fallback1.xml
+++ b/src/test/resources/input/xml/fallback2.xml
@@ -45,17 +45,24 @@
</appender>
<appender name="FALLBACK" class="org.apache.log4j.FileAppender">
- <param name="File" value="output/fallback" />
+ <param name="File" value="output/fallback2" />
<param name="Append" value="false" />
<layout class="org.apache.log4j.PatternLayout">
<param name="ConversionPattern" value="FALLBACK - %c - %m%n"/>
</layout>
</appender>
+ <appender name="STDOUT" class="org.apache.log4j.ConsoleAppender">
+ <param name="Target" value="System.out"/>
+ <layout class="org.apache.log4j.PatternLayout">
+ <param name="ConversionPattern" value="STDOUT - %m%n"/>
+ </layout>
+ </appender>
<root>
<priority value ="debug" />
<appender-ref ref="PRIMARY" />
+ <appender-ref ref="STDOUT" />
</root>
</log4j:configuration>
