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 7981fab LOGCXX-546] Prevent serialization of a multi-threaded
application (#94)
7981fab is described below
commit 7981fabdf8b5633452af5db206d16805abc7f0d4
Author: Stephen Webb <[email protected]>
AuthorDate: Sun Jan 2 21:53:48 2022 +1100
LOGCXX-546] Prevent serialization of a multi-threaded application (#94)
Improve performance by avoiding unnecessary locking when using disabled
logging statements
---
src/main/cpp/hierarchy.cpp | 9 +++-
src/main/cpp/logger.cpp | 58 ++++++++++++++-----------
src/main/cpp/rootlogger.cpp | 2 +-
src/main/include/log4cxx/logger.h | 10 +++--
src/main/include/log4cxx/spi/rootlogger.h | 2 +-
src/test/cpp/customlogger/xlogger.cpp | 8 ++--
src/test/cpp/customlogger/xloggertestcase.cpp | 2 +-
src/test/cpp/encodingtest.cpp | 2 +-
src/test/cpp/hierarchythresholdtestcase.cpp | 2 +-
src/test/cpp/l7dtestcase.cpp | 2 +-
src/test/cpp/mdctestcase.cpp | 2 +-
src/test/cpp/minimumtestcase.cpp | 2 +-
src/test/cpp/ndctestcase.cpp | 2 +-
src/test/cpp/patternlayouttest.cpp | 2 +-
src/test/cpp/varia/errorhandlertestcase.cpp | 2 +-
src/test/cpp/varia/levelmatchfiltertestcase.cpp | 2 +-
src/test/cpp/varia/levelrangefiltertestcase.cpp | 2 +-
src/test/cpp/xml/domtestcase.cpp | 2 +-
src/test/cpp/xml/xmllayouttestcase.cpp | 2 +-
19 files changed, 65 insertions(+), 50 deletions(-)
diff --git a/src/main/cpp/hierarchy.cpp b/src/main/cpp/hierarchy.cpp
index 3d79a23..312e62e 100644
--- a/src/main/cpp/hierarchy.cpp
+++ b/src/main/cpp/hierarchy.cpp
@@ -94,8 +94,13 @@ Hierarchy::Hierarchy() :
Hierarchy::~Hierarchy()
{
- // TODO LOGCXX-430
- //
https://issues.apache.org/jira/browse/LOGCXX-430?focusedCommentId=15175254&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15175254
+ std::unique_lock<std::mutex> lock(m_priv->mutex);
+ for (auto& item : *m_priv->loggers)
+ {
+ if (auto& pLogger = item.second)
+ pLogger->removeHierarchy();
+ }
+ m_priv->root->removeHierarchy();
#ifndef APR_HAS_THREADS
delete loggers;
delete provisionNodes;
diff --git a/src/main/cpp/logger.cpp b/src/main/cpp/logger.cpp
index 026960a..1f979c7 100644
--- a/src/main/cpp/logger.cpp
+++ b/src/main/cpp/logger.cpp
@@ -44,10 +44,7 @@ struct Logger::LoggerPrivate
LoggerPrivate(Pool& p, const LogString& name1):
pool(&p),
name(name1),
- level(),
- parent(),
- resourceBundle(),
- repository(),
+ repositoryRaw(0),
aai(new AppenderAttachableImpl(*pool)),
additive(true) {}
@@ -81,6 +78,7 @@ struct Logger::LoggerPrivate
// Loggers need to know what Hierarchy they are in
log4cxx::spi::LoggerRepositoryWeakPtr repository;
+ log4cxx::spi::LoggerRepository* repositoryRaw;
helpers::AppenderAttachableImplPtr aai;
@@ -109,12 +107,8 @@ Logger::~Logger()
void Logger::addAppender(const AppenderPtr newAppender)
{
- log4cxx::spi::LoggerRepositoryPtr rep;
-
m_priv->aai->addAppender(newAppender);
- rep = m_priv->repository.lock();
-
- if (rep)
+ if (auto rep = getHierarchy())
{
rep->fireAddAppenderEvent(this, newAppender.get());
}
@@ -134,7 +128,7 @@ void Logger::reconfigure( const std::vector<AppenderPtr>&
appenders, bool additi
{
m_priv->aai->addAppender( *it );
- if (log4cxx::spi::LoggerRepositoryPtr rep =
m_priv->repository.lock())
+ if (auto rep = getHierarchy())
{
rep->fireAddAppenderEvent(this, it->get());
}
@@ -157,7 +151,7 @@ void Logger::callAppenders(const spi::LoggingEventPtr&
event, Pool& p) const
}
}
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (writes == 0 && rep)
{
@@ -219,7 +213,7 @@ AppenderPtr Logger::getAppender(const LogString& name1)
const
return m_priv->aai->getAppender(name1);
}
-const LevelPtr Logger::getEffectiveLevel() const
+const LevelPtr& Logger::getEffectiveLevel() const
{
for (const Logger* l = this; l != 0; l = l->m_priv->parent.get())
{
@@ -235,9 +229,14 @@ const LevelPtr Logger::getEffectiveLevel() const
#endif
}
-LoggerRepositoryWeakPtr Logger::getLoggerRepository() const
+LoggerRepositoryPtr Logger::getLoggerRepository() const
+{
+ return m_priv->repository.lock();
+}
+
+LoggerRepository* Logger::getHierarchy() const
{
- return m_priv->repository;
+ return m_priv->repositoryRaw;
}
ResourceBundlePtr Logger::getResourceBundle() const
@@ -287,7 +286,7 @@ LoggerPtr Logger::getParent() const
return m_priv->parent;
}
-LevelPtr Logger::getLevel() const
+const LevelPtr& Logger::getLevel() const
{
return m_priv->level;
}
@@ -299,7 +298,7 @@ bool Logger::isAttached(const AppenderPtr appender) const
bool Logger::isTraceEnabled() const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(Level::TRACE_INT))
{
@@ -311,7 +310,7 @@ bool Logger::isTraceEnabled() const
bool Logger::isDebugEnabled() const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(Level::DEBUG_INT))
{
@@ -323,7 +322,7 @@ bool Logger::isDebugEnabled() const
bool Logger::isEnabledFor(const LevelPtr& level1) const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(level1->toInt()))
{
@@ -336,7 +335,7 @@ bool Logger::isEnabledFor(const LevelPtr& level1) const
bool Logger::isInfoEnabled() const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(Level::INFO_INT))
{
@@ -348,7 +347,7 @@ bool Logger::isInfoEnabled() const
bool Logger::isErrorEnabled() const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(Level::ERROR_INT))
{
@@ -360,7 +359,7 @@ bool Logger::isErrorEnabled() const
bool Logger::isWarnEnabled() const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(Level::WARN_INT))
{
@@ -372,7 +371,7 @@ bool Logger::isWarnEnabled() const
bool Logger::isFatalEnabled() const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(Level::FATAL_INT))
{
@@ -385,7 +384,9 @@ bool Logger::isFatalEnabled() const
/*void Logger::l7dlog(const LevelPtr& level, const String& key,
const char* file, int line)
{
- if (repository == 0 || repository->isDisabled(level->level))
+ auto rep = getHierarchy();
+
+ if (!rep || rep->isDisabled(level->level))
{
return;
}
@@ -410,7 +411,7 @@ bool Logger::isFatalEnabled() const
void Logger::l7dlog(const LevelPtr& level1, const LogString& key,
const LocationInfo& location, const std::vector<LogString>& params)
const
{
- log4cxx::spi::LoggerRepositoryPtr rep = m_priv->repository.lock();
+ auto rep = getHierarchy();
if (!rep || rep->isDisabled(level1->toInt()))
{
@@ -502,14 +503,21 @@ void Logger::removeAppender(const LogString& name1)
m_priv->aai->removeAppender(name1);
}
+void Logger::removeHierarchy()
+{
+ m_priv->repository.reset();
+ m_priv->repositoryRaw = 0;
+}
+
void Logger::setAdditivity(bool additive1)
{
m_priv->additive = additive1;
}
-void Logger::setHierarchy(spi::LoggerRepositoryWeakPtr repository1)
+void Logger::setHierarchy(const spi::LoggerRepositoryPtr& repository1)
{
m_priv->repository = repository1;
+ m_priv->repositoryRaw = repository1.get();
}
void Logger::setParent(LoggerPtr parentLogger)
diff --git a/src/main/cpp/rootlogger.cpp b/src/main/cpp/rootlogger.cpp
index 5c3529f..d2c8ed4 100644
--- a/src/main/cpp/rootlogger.cpp
+++ b/src/main/cpp/rootlogger.cpp
@@ -30,7 +30,7 @@ RootLogger::RootLogger(Pool& pool, const LevelPtr level1) :
setLevel(level1);
}
-const LevelPtr RootLogger::getEffectiveLevel() const
+const LevelPtr& RootLogger::getEffectiveLevel() const
{
return getLevel();
}
diff --git a/src/main/include/log4cxx/logger.h
b/src/main/include/log4cxx/logger.h
index b81c8fc..e8b40c3 100644
--- a/src/main/include/log4cxx/logger.h
+++ b/src/main/include/log4cxx/logger.h
@@ -577,13 +577,13 @@ class LOG4CXX_EXPORT Logger :
@throws RuntimeException if all levels are null in the hierarchy
*/
- virtual const LevelPtr getEffectiveLevel() const;
+ virtual const LevelPtr& getEffectiveLevel() const;
/**
Return the the LoggerRepository where this
<code>Logger</code> is attached.
*/
- log4cxx::spi::LoggerRepositoryWeakPtr getLoggerRepository()
const;
+ log4cxx::spi::LoggerRepositoryPtr getLoggerRepository() const;
/**
@@ -633,7 +633,7 @@ class LOG4CXX_EXPORT Logger :
@return Level - the assigned Level, can be null.
*/
- LevelPtr getLevel() const;
+ const LevelPtr& getLevel() const;
/**
* Retrieve a logger by name in current encoding.
@@ -1422,8 +1422,10 @@ class LOG4CXX_EXPORT Logger :
friend class Hierarchy;
/**
Only the Hierarchy class can set the hierarchy of a logger.*/
- void setHierarchy(spi::LoggerRepositoryWeakPtr repository);
+ void removeHierarchy();
+ void setHierarchy(const spi::LoggerRepositoryPtr& repository);
void setParent(LoggerPtr parentLogger);
+ spi::LoggerRepository* getHierarchy() const;
public:
/**
diff --git a/src/main/include/log4cxx/spi/rootlogger.h
b/src/main/include/log4cxx/spi/rootlogger.h
index 812f449..50c0502 100644
--- a/src/main/include/log4cxx/spi/rootlogger.h
+++ b/src/main/include/log4cxx/spi/rootlogger.h
@@ -48,7 +48,7 @@ class LOG4CXX_EXPORT RootLogger : public Logger
Return the assigned level value without walking the logger
hierarchy.
*/
- virtual const LevelPtr getEffectiveLevel() const;
+ virtual const LevelPtr& getEffectiveLevel() const;
/**
Setting a null value to the level of the root
logger may have catastrophic
diff --git a/src/test/cpp/customlogger/xlogger.cpp
b/src/test/cpp/customlogger/xlogger.cpp
index 3aaf2bf..9a3760a 100644
--- a/src/test/cpp/customlogger/xlogger.cpp
+++ b/src/test/cpp/customlogger/xlogger.cpp
@@ -32,7 +32,7 @@ XFactoryPtr XLogger::factory = XFactoryPtr(new XFactory());
void XLogger::lethal(const LogString& message, const LocationInfo&
locationInfo)
{
- log4cxx::spi::LoggerRepositoryPtr rep = getLoggerRepository().lock();
+ auto rep = getLoggerRepository();
if (rep->isDisabled(XLevel::LETHAL_INT))
{
@@ -47,7 +47,7 @@ void XLogger::lethal(const LogString& message, const
LocationInfo& locationInfo)
void XLogger::lethal(const LogString& message)
{
- log4cxx::spi::LoggerRepositoryPtr rep = getLoggerRepository().lock();
+ auto rep = getLoggerRepository();
if (rep->isDisabled(XLevel::LETHAL_INT))
{
@@ -72,7 +72,7 @@ LoggerPtr XLogger::getLogger(const helpers::Class& clazz)
void XLogger::trace(const LogString& message, const LocationInfo& locationInfo)
{
- log4cxx::spi::LoggerRepositoryPtr rep = getLoggerRepository().lock();
+ auto rep = getLoggerRepository();
if (rep->isDisabled(XLevel::TRACE_INT))
{
@@ -87,7 +87,7 @@ void XLogger::trace(const LogString& message, const
LocationInfo& locationInfo)
void XLogger::trace(const LogString& message)
{
- log4cxx::spi::LoggerRepositoryPtr rep = getLoggerRepository().lock();
+ auto rep = getLoggerRepository();
if (rep->isDisabled(XLevel::TRACE_INT))
{
diff --git a/src/test/cpp/customlogger/xloggertestcase.cpp
b/src/test/cpp/customlogger/xloggertestcase.cpp
index 8023406..74302e5 100644
--- a/src/test/cpp/customlogger/xloggertestcase.cpp
+++ b/src/test/cpp/customlogger/xloggertestcase.cpp
@@ -52,7 +52,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
logger->getLoggerRepository().lock();
+ auto rep = logger->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/encodingtest.cpp b/src/test/cpp/encodingtest.cpp
index 2b9d334..7e190b1 100644
--- a/src/test/cpp/encodingtest.cpp
+++ b/src/test/cpp/encodingtest.cpp
@@ -58,7 +58,7 @@ public:
*/
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
Logger::getRootLogger()->getLoggerRepository().lock();
+ auto rep = Logger::getRootLogger()->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/hierarchythresholdtestcase.cpp
b/src/test/cpp/hierarchythresholdtestcase.cpp
index 3744ca4..de836f0 100644
--- a/src/test/cpp/hierarchythresholdtestcase.cpp
+++ b/src/test/cpp/hierarchythresholdtestcase.cpp
@@ -49,7 +49,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
logger->getLoggerRepository().lock();
+ auto rep = logger->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/l7dtestcase.cpp b/src/test/cpp/l7dtestcase.cpp
index 9a1bb11..7c70b74 100644
--- a/src/test/cpp/l7dtestcase.cpp
+++ b/src/test/cpp/l7dtestcase.cpp
@@ -67,7 +67,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
root->getLoggerRepository().lock();
+ auto rep = root->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/mdctestcase.cpp b/src/test/cpp/mdctestcase.cpp
index e05383b..06f4f3a 100644
--- a/src/test/cpp/mdctestcase.cpp
+++ b/src/test/cpp/mdctestcase.cpp
@@ -42,7 +42,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
Logger::getRootLogger()->getLoggerRepository().lock();
+ auto rep = Logger::getRootLogger()->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/minimumtestcase.cpp b/src/test/cpp/minimumtestcase.cpp
index 08012cd..aa23221 100644
--- a/src/test/cpp/minimumtestcase.cpp
+++ b/src/test/cpp/minimumtestcase.cpp
@@ -53,7 +53,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
root->getLoggerRepository().lock();
+ auto rep = root->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/ndctestcase.cpp b/src/test/cpp/ndctestcase.cpp
index b9fcd7b..ec96220 100644
--- a/src/test/cpp/ndctestcase.cpp
+++ b/src/test/cpp/ndctestcase.cpp
@@ -47,7 +47,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
logger->getLoggerRepository().lock();
+ auto rep = logger->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/patternlayouttest.cpp
b/src/test/cpp/patternlayouttest.cpp
index 9dbd2e6..b49dd3d 100644
--- a/src/test/cpp/patternlayouttest.cpp
+++ b/src/test/cpp/patternlayouttest.cpp
@@ -94,7 +94,7 @@ public:
void tearDown()
{
MDC::clear();
- log4cxx::spi::LoggerRepositoryPtr rep =
root->getLoggerRepository().lock();
+ auto rep = root->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/varia/errorhandlertestcase.cpp
b/src/test/cpp/varia/errorhandlertestcase.cpp
index 314c9ca..e86972d 100644
--- a/src/test/cpp/varia/errorhandlertestcase.cpp
+++ b/src/test/cpp/varia/errorhandlertestcase.cpp
@@ -50,7 +50,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
logger->getLoggerRepository().lock();
+ auto rep = logger->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/varia/levelmatchfiltertestcase.cpp
b/src/test/cpp/varia/levelmatchfiltertestcase.cpp
index 083580b..d4198e5 100644
--- a/src/test/cpp/varia/levelmatchfiltertestcase.cpp
+++ b/src/test/cpp/varia/levelmatchfiltertestcase.cpp
@@ -56,7 +56,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
root->getLoggerRepository().lock();
+ auto rep = root->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/varia/levelrangefiltertestcase.cpp
b/src/test/cpp/varia/levelrangefiltertestcase.cpp
index 6a3c95e..2c297aa 100644
--- a/src/test/cpp/varia/levelrangefiltertestcase.cpp
+++ b/src/test/cpp/varia/levelrangefiltertestcase.cpp
@@ -56,7 +56,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
root->getLoggerRepository().lock();
+ auto rep = root->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/xml/domtestcase.cpp b/src/test/cpp/xml/domtestcase.cpp
index fce5874..e254868 100644
--- a/src/test/cpp/xml/domtestcase.cpp
+++ b/src/test/cpp/xml/domtestcase.cpp
@@ -76,7 +76,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
root->getLoggerRepository().lock();
+ auto rep = root->getLoggerRepository();
if (rep)
{
diff --git a/src/test/cpp/xml/xmllayouttestcase.cpp
b/src/test/cpp/xml/xmllayouttestcase.cpp
index 4311e97..05711da 100644
--- a/src/test/cpp/xml/xmllayouttestcase.cpp
+++ b/src/test/cpp/xml/xmllayouttestcase.cpp
@@ -82,7 +82,7 @@ public:
void tearDown()
{
- log4cxx::spi::LoggerRepositoryPtr rep =
logger->getLoggerRepository().lock();
+ auto rep = logger->getLoggerRepository();
if (rep)
{
