[logging-log4cxx] branch master updated: LOGCXX-523 Call error handler if rollover fails (#64)

[rm...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rmiddleton 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 674f43d  LOGCXX-523 Call error handler if rollover fails (#64)
674f43d is described below

commit 674f43db0dce625afe6ec9981afeb1fda6ab923b
Author: Robert Middleton <rm...@users.noreply.github.com>
AuthorDate: Mon Apr 26 20:29:39 2021 -0400

    LOGCXX-523 Call error handler if rollover fails (#64)
    
    * LOGCXX-523 Call error handler if rollover fails
    
    If the rollover fails, call the specified error handler for
    the given appender.  This means that you can now replace appenders
    with a new appender.
    
    The entire API at this point is not very robust at the moment.  A
    better error handling mechanism needs to be made to properly
    handle the errors so that the error handler can have enough
    data to do its job.
    
    * Fix build on Windows
    
    * Add include file needed for windows
---
 src/main/cpp/appenderattachableimpl.cpp | 25 ++++++++--
 src/main/cpp/asyncappender.cpp          |  8 ----
 src/main/cpp/logger.cpp                 | 81 ++++-----------------------------
 src/main/cpp/rollingfileappender.cpp    | 21 +++++++--
 4 files changed, 47 insertions(+), 88 deletions(-)

diff --git a/src/main/cpp/appenderattachableimpl.cpp b/src/main/cpp/appenderattachableimpl.cpp
index 6e63488..df077a1 100644
--- a/src/main/cpp/appenderattachableimpl.cpp
+++ b/src/main/cpp/appenderattachableimpl.cpp
@@ -42,6 +42,7 @@ void AppenderAttachableImpl::addAppender(const AppenderPtr newAppender)
 		return;
 	}
 
+	std::unique_lock<std::mutex> lock( m_mutex );
 	AppenderList::iterator it = std::find(
 			appenderList.begin(), appenderList.end(), newAppender);
 
@@ -55,14 +56,27 @@ int AppenderAttachableImpl::appendLoopOnAppenders(
 	const spi::LoggingEventPtr& event,
 	Pool& p)
 {
-	for (AppenderList::iterator it = appenderList.begin();
-		it != appenderList.end();
+
+	AppenderList allAppenders;
+	int numberAppended = 0;
+	{
+		// 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_mutex );
+		allAppenders = appenderList;
+	}
+	for (AppenderList::iterator it = allAppenders.begin();
+		it != allAppenders.end();
 		it++)
 	{
 		(*it)->doAppend(event, p);
+		numberAppended++;
 	}
 
-	return (int)appenderList.size();
+	return numberAppended;
 }
 
 AppenderList AppenderAttachableImpl::getAllAppenders() const
@@ -77,6 +91,7 @@ AppenderPtr AppenderAttachableImpl::getAppender(const LogString& name) const
 		return 0;
 	}
 
+	std::unique_lock<std::mutex> lock( m_mutex );
 	AppenderList::const_iterator it, itEnd = appenderList.end();
 	AppenderPtr appender;
 
@@ -100,6 +115,7 @@ bool AppenderAttachableImpl::isAttached(const AppenderPtr appender) const
 		return false;
 	}
 
+	std::unique_lock<std::mutex> lock( m_mutex );
 	AppenderList::const_iterator it = std::find(
 			appenderList.begin(), appenderList.end(), appender);
 
@@ -108,6 +124,7 @@ bool AppenderAttachableImpl::isAttached(const AppenderPtr appender) const
 
 void AppenderAttachableImpl::removeAllAppenders()
 {
+	std::unique_lock<std::mutex> lock( m_mutex );
 	AppenderList::iterator it, itEnd = appenderList.end();
 	AppenderPtr a;
 
@@ -127,6 +144,7 @@ void AppenderAttachableImpl::removeAppender(const AppenderPtr appender)
 		return;
 	}
 
+	std::unique_lock<std::mutex> lock( m_mutex );
 	AppenderList::iterator it = std::find(
 			appenderList.begin(), appenderList.end(), appender);
 
@@ -143,6 +161,7 @@ void AppenderAttachableImpl::removeAppender(const LogString& name)
 		return;
 	}
 
+	std::unique_lock<std::mutex> lock( m_mutex );
 	AppenderList::iterator it, itEnd = appenderList.end();
 	AppenderPtr appender;
 
diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp
index 89300bc..6da23b1 100644
--- a/src/main/cpp/asyncappender.cpp
+++ b/src/main/cpp/asyncappender.cpp
@@ -61,7 +61,6 @@ AsyncAppender::~AsyncAppender()
 
 void AsyncAppender::addAppender(const AppenderPtr newAppender)
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	appenders->addAppender(newAppender);
 }
 
@@ -221,19 +220,16 @@ void AsyncAppender::close()
 
 AppenderList AsyncAppender::getAllAppenders() const
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	return appenders->getAllAppenders();
 }
 
 AppenderPtr AsyncAppender::getAppender(const LogString& n) const
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	return appenders->getAppender(n);
 }
 
 bool AsyncAppender::isAttached(const AppenderPtr appender) const
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	return appenders->isAttached(appender);
 }
 
@@ -244,19 +240,16 @@ bool AsyncAppender::requiresLayout() const
 
 void AsyncAppender::removeAllAppenders()
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	appenders->removeAllAppenders();
 }
 
 void AsyncAppender::removeAppender(const AppenderPtr appender)
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	appenders->removeAppender(appender);
 }
 
 void AsyncAppender::removeAppender(const LogString& n)
 {
-	std::unique_lock<std::mutex> lock(appenders->getMutex());
 	appenders->removeAppender(n);
 }
 
@@ -403,7 +396,6 @@ void AsyncAppender::dispatch()
 				iter != events.end();
 				iter++)
 			{
-				std::unique_lock<std::mutex> lock(appenders->getMutex());
 				appenders->appendLoopOnAppenders(*iter, p);
 			}
 		}
diff --git a/src/main/cpp/logger.cpp b/src/main/cpp/logger.cpp
index 71c16ed..fc617ae 100644
--- a/src/main/cpp/logger.cpp
+++ b/src/main/cpp/logger.cpp
@@ -42,7 +42,7 @@ IMPLEMENT_LOG4CXX_OBJECT(Logger)
 
 Logger::Logger(Pool& p, const LogString& name1)
 	: pool(&p), name(), level(), parent(), resourceBundle(),
-	  repository(), aai()
+	  repository(), aai(new AppenderAttachableImpl(*pool))
 {
 	name = name1;
 	additive = true;
@@ -55,17 +55,9 @@ Logger::~Logger()
 void Logger::addAppender(const AppenderPtr newAppender)
 {
 	log4cxx::spi::LoggerRepositoryPtr rep;
-	{
-		log4cxx::shared_lock<log4cxx::shared_mutex> lock(mutex);
-
-		if (aai == 0)
-		{
-			aai = AppenderAttachableImplPtr(new AppenderAttachableImpl(*pool));
-		}
 
-		aai->addAppender(newAppender);
-		rep = repository.lock();
-	}
+	aai->addAppender(newAppender);
+	rep = repository.lock();
 
 	if (rep)
 	{
@@ -79,13 +71,7 @@ void Logger::reconfigure( const std::vector<AppenderPtr>& appenders, bool additi
 
 	additive = additive1;
 
-	if (aai != 0)
-	{
-		aai->removeAllAppenders();
-		aai = 0;
-	}
-
-	aai = AppenderAttachableImplPtr(new AppenderAttachableImpl(*pool));
+	aai->removeAllAppenders();
 
 	for ( std::vector<AppenderPtr>::const_iterator it = appenders.cbegin();
 		it != appenders.cend();
@@ -108,13 +94,7 @@ void Logger::callAppenders(const spi::LoggingEventPtr& event, Pool& p) const
 		logger != 0;
 		logger = logger->parent.get())
 	{
-		// Protected against simultaneous call to addAppender, removeAppender,...
-		std::unique_lock<log4cxx::shared_mutex> lock(logger->mutex);
-
-		if (logger->aai != 0)
-		{
-			writes += logger->aai->appendLoopOnAppenders(event, p);
-		}
+		writes += logger->aai->appendLoopOnAppenders(event, p);
 
 		if (!logger->additive)
 		{
@@ -175,27 +155,11 @@ bool Logger::getAdditivity() const
 
 AppenderList Logger::getAllAppenders() const
 {
-	log4cxx::shared_lock<log4cxx::shared_mutex> lock(mutex);
-
-	if (aai == 0)
-	{
-		return AppenderList();
-	}
-	else
-	{
-		return aai->getAllAppenders();
-	}
+	return aai->getAllAppenders();
 }
 
 AppenderPtr Logger::getAppender(const LogString& name1) const
 {
-	log4cxx::shared_lock<log4cxx::shared_mutex> lock(mutex);
-
-	if (aai == 0 || name1.empty())
-	{
-		return 0;
-	}
-
 	return aai->getAppender(name1);
 }
 
@@ -275,16 +239,7 @@ LevelPtr Logger::getLevel() const
 
 bool Logger::isAttached(const AppenderPtr appender) const
 {
-	log4cxx::shared_lock<log4cxx::shared_mutex> lock(mutex);
-
-	if (appender == 0 || aai == 0)
-	{
-		return false;
-	}
-	else
-	{
-		return aai->isAttached(appender);
-	}
+	return aai->isAttached(appender);
 }
 
 bool Logger::isTraceEnabled() const
@@ -471,36 +426,16 @@ void Logger::l7dlog(const LevelPtr& level1, const std::string& key,
 
 void Logger::removeAllAppenders()
 {
-	std::unique_lock<log4cxx::shared_mutex> lock(mutex);
-
-	if (aai != 0)
-	{
-		aai->removeAllAppenders();
-		aai = 0;
-	}
+	aai->removeAllAppenders();
 }
 
 void Logger::removeAppender(const AppenderPtr appender)
 {
-	std::unique_lock<log4cxx::shared_mutex> lock(mutex);
-
-	if (appender == 0 || aai == 0)
-	{
-		return;
-	}
-
 	aai->removeAppender(appender);
 }
 
 void Logger::removeAppender(const LogString& name1)
 {
-	std::unique_lock<log4cxx::shared_mutex> lock(mutex);
-
-	if (name1.empty() || aai == 0)
-	{
-		return;
-	}
-
 	aai->removeAppender(name1);
 }
 
diff --git a/src/main/cpp/rollingfileappender.cpp b/src/main/cpp/rollingfileappender.cpp
index bb805a6..3e6f2dc 100644
--- a/src/main/cpp/rollingfileappender.cpp
+++ b/src/main/cpp/rollingfileappender.cpp
@@ -39,6 +39,7 @@
 #include <log4cxx/helpers/bytebuffer.h>
 #include <log4cxx/rolling/fixedwindowrollingpolicy.h>
 #include <log4cxx/rolling/manualtriggeringpolicy.h>
+#include <log4cxx/helpers/transcoder.h>
 
 using namespace log4cxx;
 using namespace log4cxx::rolling;
@@ -305,9 +306,12 @@ bool RollingFileAppenderSkeleton::rolloverInternal(Pool& p)
 								{
 									success = rollover1->getSynchronous()->execute(p);
 								}
-								catch (std::exception&)
+								catch (std::exception& ex)
 								{
 									LogLog::warn(LOG4CXX_STR("Exception on rollover"));
+									LogString exmsg;
+									log4cxx::helpers::Transcoder::decode(ex.what(), exmsg);
+									errorHandler->error(exmsg, ex, 0);
 								}
 							}
 
@@ -363,9 +367,12 @@ bool RollingFileAppenderSkeleton::rolloverInternal(Pool& p)
 								{
 									success = rollover1->getSynchronous()->execute(p);
 								}
-								catch (std::exception&)
+								catch (std::exception& ex)
 								{
 									LogLog::warn(LOG4CXX_STR("Exception during rollover"));
+									LogString exmsg;
+									log4cxx::helpers::Transcoder::decode(ex.what(), exmsg);
+									errorHandler->error(exmsg, ex, 0);
 								}
 							}
 
@@ -400,9 +407,12 @@ bool RollingFileAppenderSkeleton::rolloverInternal(Pool& p)
 						return true;
 					}
 				}
-				catch (std::exception&)
+				catch (std::exception& ex)
 				{
 					LogLog::warn(LOG4CXX_STR("Exception during rollover"));
+					LogString exmsg;
+					log4cxx::helpers::Transcoder::decode(ex.what(), exmsg);
+					errorHandler->error(exmsg, ex, 0);
 				}
 
 #ifdef LOG4CXX_MULTI_PROCESS
@@ -458,9 +468,12 @@ void RollingFileAppenderSkeleton::subAppend(const LoggingEventPtr& event, Pool&
 			_event = &(const_cast<LoggingEventPtr&>(event));
 			rolloverInternal(p);
 		}
-		catch (std::exception&)
+		catch (std::exception& ex)
 		{
 			LogLog::warn(LOG4CXX_STR("Exception during rollover attempt."));
+			LogString exmsg;
+			log4cxx::helpers::Transcoder::decode(ex.what(), exmsg);
+			errorHandler->error(exmsg);
 		}
 	}