This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch real_time_usage in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 0c3aefa20830c1537300ac4196b8b70eb3257a69 Author: Stephen Webb <[email protected]> AuthorDate: Sat Feb 4 16:43:54 2023 +1100 Prevent priority inversions when logging from multiple threads --- CMakeLists.txt | 2 + src/CMakeLists.txt | 3 + src/cmake/pthread/log4cxx-pthread.cmake | 16 +++- src/cmake/pthread/test-pthread-setprotocol.cpp | 18 +++++ src/main/cpp/appenderattachableimpl.cpp | 89 +++++----------------- src/main/cpp/appenderskeleton.cpp | 18 ++--- src/main/cpp/asyncappender.cpp | 36 +++------ src/main/cpp/fileappender.cpp | 18 ++--- src/main/cpp/multiprocessrollingfileappender.cpp | 4 +- src/main/cpp/rollingfileappender.cpp | 4 +- src/main/cpp/socketappenderskeleton.cpp | 4 +- src/main/cpp/telnetappender.cpp | 10 +-- src/main/cpp/writerappender.cpp | 4 +- src/main/cpp/xmlsocketappender.cpp | 2 +- src/main/include/CMakeLists.txt | 19 +---- src/main/include/log4cxx/appenderskeleton.h | 14 ++-- .../log4cxx/private/appenderskeleton_priv.h | 18 ++--- .../include/log4cxx/private/log4cxx_private.h.in | 1 + src/main/include/log4cxx/private/mutex.h | 72 +++++++++++++++++ 19 files changed, 190 insertions(+), 162 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4f55805d..9321bbd8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -202,6 +202,7 @@ get_directory_property( ATOMIC_IMPL DIRECTORY src DEFINITION ATOMIC_IMPL ) get_directory_property( FILESYSTEM_IMPL DIRECTORY src DEFINITION FILESYSTEM_IMPL ) get_directory_property( STD_MAKE_UNIQUE_IMPL DIRECTORY src DEFINITION STD_MAKE_UNIQUE_IMPL ) get_directory_property( STD_LIB_HAS_UNICODE_STRING DIRECTORY src DEFINITION STD_LIB_HAS_UNICODE_STRING ) +get_directory_property( HAS_PTHREAD_SETPROTOCOL DIRECTORY src DEFINITION HAS_PTHREAD_SETPROTOCOL ) foreach(varName HAS_STD_LOCALE HAS_ODBC HAS_MBSRTOWCS HAS_WCSTOMBS HAS_FWIDE HAS_LIBESMTP HAS_SYSLOG HAS_FMT) if(${varName} EQUAL 0) @@ -282,6 +283,7 @@ message(STATUS " Prefer boost: ................... : ${PREFER_BOOST}") message(STATUS " thread implementation ........... : ${THREAD_IMPL}") message(STATUS " thread_local support? ........... : ${HAS_THREAD_LOCAL}") message(STATUS " mutex implementation ............ : ${MUTEX_IMPL}") +message(STATUS " priority inheritance? ........... : ${HAS_PTHREAD_SETPROTOCOL}") message(STATUS " shared_ptr implementation ....... : ${SMART_PTR_IMPL}") message(STATUS " shared_mutex implementation ..... : ${SHARED_MUTEX_IMPL}") message(STATUS " atomic implementation ........... : ${ATOMIC_IMPL}") diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 788320de..b5a1e6e8 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -17,6 +17,9 @@ cmake_policy(SET CMP0079 NEW) include(${CMAKE_CURRENT_LIST_DIR}/cmake/boost-fallback/boost-fallback.cmake) include(${CMAKE_CURRENT_LIST_DIR}/cmake/compiler-features/check-compiler-support.cmake) +if(UNIX) + include(${CMAKE_CURRENT_LIST_DIR}/cmake/pthread/log4cxx-pthread.cmake) +endif(UNIX) add_subdirectory(main) target_compile_definitions(log4cxx PRIVATE ${LOG4CXX_COMPILE_DEFINITIONS} ${APR_COMPILE_DEFINITIONS} ${APR_UTIL_COMPILE_DEFINITIONS} ) diff --git a/src/cmake/pthread/log4cxx-pthread.cmake b/src/cmake/pthread/log4cxx-pthread.cmake index bd9e8590..c411b201 100644 --- a/src/cmake/pthread/log4cxx-pthread.cmake +++ b/src/cmake/pthread/log4cxx-pthread.cmake @@ -1,10 +1,22 @@ include(FindThreads) -try_compile(PTHREAD_SETNAME_NP_FOUND "${CMAKE_BINARY_DIR}/pthread-compile-tests" +set(CMAKE_REQUIRED_LIBRARIES "pthread") +CHECK_SYMBOL_EXISTS(pthread_sigmask "signal.h" HAS_PTHREAD_SIGMASK) + +# Check for the (linux) pthread_setname_np. +# OSX and BSD are special apparently. OSX only lets you name +# the current thread, while BSD calls it pthread_set_name_np. +# Since this is really not a core feature and the end-user can configure +# it anyway, we're only going to worry about linux. +try_compile(HAS_PTHREAD_SETNAME "${CMAKE_BINARY_DIR}/pthread-compile-tests" "${CMAKE_CURRENT_LIST_DIR}/test-pthread-setname.cpp" LINK_LIBRARIES Threads::Threads ) -try_compile(PTHREAD_GETNAME_NP_FOUND "${CMAKE_BINARY_DIR}/pthread-compile-tests" +try_compile(HAS_PTHREAD_GETNAME "${CMAKE_BINARY_DIR}/pthread-compile-tests" "${CMAKE_CURRENT_LIST_DIR}/test-pthread-getname.cpp" LINK_LIBRARIES Threads::Threads ) +try_compile(HAS_PTHREAD_SETPROTOCOL "${CMAKE_BINARY_DIR}/pthread-compile-tests" + "${CMAKE_CURRENT_LIST_DIR}/test-pthread-setprotocol.cpp" + LINK_LIBRARIES Threads::Threads ) + diff --git a/src/cmake/pthread/test-pthread-setprotocol.cpp b/src/cmake/pthread/test-pthread-setprotocol.cpp new file mode 100644 index 00000000..133068a0 --- /dev/null +++ b/src/cmake/pthread/test-pthread-setprotocol.cpp @@ -0,0 +1,18 @@ +#include <stdlib.h> +#include <pthread.h> + +int main(){ + int result = EXIT_SUCCESS; + pthread_mutex_t m; + pthread_mutexattr_t rrAttr; + if (pthread_mutexattr_init(&rrAttr) != 0) + result = EXIT_FAILURE; + else if (pthread_mutexattr_setprotocol(&rrAttr, PTHREAD_PRIO_INHERIT) != 0) + result = EXIT_FAILURE; + else if (pthread_mutex_init(&m, &rrAttr) != 0) + result = EXIT_FAILURE; + else + pthread_mutex_destroy(&m); + pthread_mutexattr_destroy(&rrAttr); + return result; +} diff --git a/src/main/cpp/appenderattachableimpl.cpp b/src/main/cpp/appenderattachableimpl.cpp index d936d9c6..6363a8d7 100644 --- a/src/main/cpp/appenderattachableimpl.cpp +++ b/src/main/cpp/appenderattachableimpl.cpp @@ -55,9 +55,7 @@ void AppenderAttachableImpl::addAppender(const AppenderPtr newAppender) } 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); - + auto it = std::find(m_priv->appenderList.begin(), m_priv->appenderList.end(), newAppender); if (it == m_priv->appenderList.end()) { m_priv->appenderList.push_back(newAppender); @@ -69,13 +67,12 @@ int AppenderAttachableImpl::appendLoopOnAppenders( Pool& p) { 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) + // doAppend() will serialize calls to AppenderSkeleton#append. + for (auto appender : getAllAppenders()) { appender->doAppend(event, p); numberAppended++; @@ -92,95 +89,49 @@ AppenderList AppenderAttachableImpl::getAllAppenders() const AppenderPtr AppenderAttachableImpl::getAppender(const LogString& name) const { - if (name.empty()) - { - return 0; - } - + AppenderPtr result; std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex ); - AppenderList::const_iterator it, itEnd = m_priv->appenderList.end(); - AppenderPtr appender; - - for (it = m_priv->appenderList.begin(); it != itEnd; it++) - { - appender = *it; - - if (name == appender->getName()) - { - return appender; - } - } - - return 0; + auto it = std::find_if(m_priv->appenderList.begin(), m_priv->appenderList.end(), + [&name](const AppenderPtr& appender) -> bool + { return name == appender->getName(); } + ); + if (it != m_priv->appenderList.end()) + result = *it; + return result; } bool AppenderAttachableImpl::isAttached(const AppenderPtr appender) const { - if (appender == 0) - { - return false; - } - 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); - + auto it = std::find(m_priv->appenderList.begin(), m_priv->appenderList.end(), appender); return it != m_priv->appenderList.end(); } void AppenderAttachableImpl::removeAllAppenders() { std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex ); - AppenderList::iterator it, itEnd = m_priv->appenderList.end(); - AppenderPtr a; - - for (it = m_priv->appenderList.begin(); it != itEnd; it++) - { - a = *it; + for (auto a : m_priv->appenderList) a->close(); - } - m_priv->appenderList.clear(); } void AppenderAttachableImpl::removeAppender(const AppenderPtr appender) { - if (appender == 0) - { - return; - } - 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); - + auto it = std::find(m_priv->appenderList.begin(), m_priv->appenderList.end(), appender); if (it != m_priv->appenderList.end()) - { m_priv->appenderList.erase(it); - } } void AppenderAttachableImpl::removeAppender(const LogString& name) { - if (name.empty()) - { - return; - } - std::lock_guard<std::recursive_mutex> lock( m_priv->m_mutex ); - AppenderList::iterator it, itEnd = m_priv->appenderList.end(); - AppenderPtr appender; - - for (it = m_priv->appenderList.begin(); it != itEnd; it++) - { - appender = *it; - - if (name == appender->getName()) - { - m_priv->appenderList.erase(it); - return; - } - } + auto it = std::find_if(m_priv->appenderList.begin(), m_priv->appenderList.end(), + [&name](const AppenderPtr& appender) -> bool + { return name == appender->getName(); } + ); + if (it != m_priv->appenderList.end()) + m_priv->appenderList.erase(it); } diff --git a/src/main/cpp/appenderskeleton.cpp b/src/main/cpp/appenderskeleton.cpp index fdc489cb..9fa9f6c2 100644 --- a/src/main/cpp/appenderskeleton.cpp +++ b/src/main/cpp/appenderskeleton.cpp @@ -64,7 +64,7 @@ void AppenderSkeleton::finalize() void AppenderSkeleton::addFilter(const spi::FilterPtr newFilter) { - std::lock_guard<std::recursive_mutex> lock(m_priv->mutex); + AppenderScopeGuard lock(m_priv->mutex); if (m_priv->headFilter == nullptr) { @@ -79,19 +79,17 @@ void AppenderSkeleton::addFilter(const spi::FilterPtr newFilter) void AppenderSkeleton::clearFilters() { - std::lock_guard<std::recursive_mutex> lock(m_priv->mutex); + AppenderScopeGuard lock(m_priv->mutex); m_priv->headFilter = m_priv->tailFilter = nullptr; } bool AppenderSkeleton::isAsSevereAsThreshold(const LevelPtr& level) const { - return ((level == 0) || level->isGreaterOrEqual(m_priv->threshold)); + return !level || level->isGreaterOrEqual(m_priv->threshold); } void AppenderSkeleton::doAppend(const spi::LoggingEventPtr& event, Pool& pool1) { - std::lock_guard<std::recursive_mutex> lock(m_priv->mutex); - doAppendImpl(event, pool1); } @@ -99,7 +97,7 @@ void AppenderSkeleton::doAppendImpl(const spi::LoggingEventPtr& event, Pool& poo { if (m_priv->closed) { - LogLog::error(((LogString) LOG4CXX_STR("Attempted to append to closed appender named [")) + LogLog::error(LOG4CXX_STR("Attempted to append to closed appender named [") + m_priv->name + LOG4CXX_STR("].")); return; } @@ -109,9 +107,8 @@ void AppenderSkeleton::doAppendImpl(const spi::LoggingEventPtr& event, Pool& poo return; } + AppenderScopeGuard lock(m_priv->mutex); FilterPtr f = m_priv->headFilter; - - while (f != 0) { switch (f->decide(event)) @@ -127,13 +124,12 @@ void AppenderSkeleton::doAppendImpl(const spi::LoggingEventPtr& event, Pool& poo f = f->getNext(); } } - append(event, pool1); } void AppenderSkeleton::setErrorHandler(const spi::ErrorHandlerPtr errorHandler1) { - std::lock_guard<std::recursive_mutex> lock(m_priv->mutex); + AppenderScopeGuard lock(m_priv->mutex); if (errorHandler1 == nullptr) { @@ -149,7 +145,7 @@ void AppenderSkeleton::setErrorHandler(const spi::ErrorHandlerPtr errorHandler1) void AppenderSkeleton::setThreshold(const LevelPtr& threshold1) { - std::lock_guard<std::recursive_mutex> lock(m_priv->mutex); + AppenderScopeGuard lock(m_priv->mutex); m_priv->threshold = threshold1; } diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 20ff3cd8..40339949 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -94,30 +94,22 @@ struct AsyncAppender::AsyncAppenderPriv : public AppenderSkeleton::AppenderSkele appenders(std::make_shared<AppenderAttachableImpl>(pool)), dispatcher(), locationInfo(false), - blocking(true) {} + blocking(true) + { EnablePriorityInheritance(bufferMutex, pool); } /** * Event buffer. */ -#if defined(NON_BLOCKING) - boost::lockfree::queue<log4cxx::spi::LoggingEvent* > buffer; - std::atomic<size_t> discardedCount; -#else LoggingEventList buffer; -#endif /** * Mutex used to guard access to buffer and discardMap. */ - std::mutex bufferMutex; + AppenderMutexType bufferMutex; + -#if defined(NON_BLOCKING) - ::log4cxx::helpers::Semaphore bufferNotFull; - ::log4cxx::helpers::Semaphore bufferNotEmpty; -#else std::condition_variable bufferNotFull; std::condition_variable bufferNotEmpty; -#endif /** * Map of DiscardSummary objects keyed by logger name. @@ -197,8 +189,6 @@ void AsyncAppender::setOption(const LogString& option, void AsyncAppender::doAppend(const spi::LoggingEventPtr& event, Pool& pool1) { - std::lock_guard<std::recursive_mutex> lock(priv->mutex); - doAppendImpl(event, pool1); } @@ -222,7 +212,7 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) { - std::unique_lock<std::mutex> lock(priv->bufferMutex); + AppenderScopedLock lock(priv->bufferMutex); while (true) { @@ -286,7 +276,7 @@ void AsyncAppender::append(const spi::LoggingEventPtr& event, Pool& p) void AsyncAppender::close() { { - std::lock_guard<std::mutex> lock(priv->bufferMutex); + AppenderScopeGuard lock(priv->bufferMutex); priv->closed = true; priv->bufferNotEmpty.notify_all(); priv->bufferNotFull.notify_all(); @@ -298,13 +288,9 @@ void AsyncAppender::close() } { - AppenderList appenderList = priv->appenders->getAllAppenders(); - - for (AppenderList::iterator iter = appenderList.begin(); - iter != appenderList.end(); - iter++) + for (auto item : priv->appenders->getAllAppenders()) { - (*iter)->close(); + item->close(); } } } @@ -362,7 +348,7 @@ void AsyncAppender::setBufferSize(int size) throw IllegalArgumentException(LOG4CXX_STR("size argument must be non-negative")); } - std::lock_guard<std::mutex> lock(priv->bufferMutex); + AppenderScopeGuard lock(priv->bufferMutex); priv->bufferSize = (size < 1) ? 1 : size; priv->bufferNotFull.notify_all(); } @@ -374,7 +360,7 @@ int AsyncAppender::getBufferSize() const void AsyncAppender::setBlocking(bool value) { - std::lock_guard<std::mutex> lock(priv->bufferMutex); + AppenderScopeGuard lock(priv->bufferMutex); priv->blocking = value; priv->bufferNotFull.notify_all(); } @@ -451,7 +437,7 @@ void AsyncAppender::dispatch() Pool p; LoggingEventList events; { - std::unique_lock<std::mutex> lock(priv->bufferMutex); + AppenderScopedLock lock(priv->bufferMutex); priv->bufferNotEmpty.wait(lock, [this]() -> bool { return 0 < priv->buffer.size() || priv->closed; } ); diff --git a/src/main/cpp/fileappender.cpp b/src/main/cpp/fileappender.cpp index 38814855..f4134753 100644 --- a/src/main/cpp/fileappender.cpp +++ b/src/main/cpp/fileappender.cpp @@ -85,13 +85,13 @@ FileAppender::~FileAppender() void FileAppender::setAppend(bool fileAppend1) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->fileAppend = fileAppend1; } void FileAppender::setFile(const LogString& file) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); setFileInternal(file); } @@ -102,7 +102,7 @@ void FileAppender::setFileInternal(const LogString& file) void FileAppender::setBufferedIO(bool bufferedIO1) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->bufferedIO = bufferedIO1; if (bufferedIO1) @@ -117,27 +117,27 @@ void FileAppender::setOption(const LogString& option, if (StringHelper::equalsIgnoreCase(option, LOG4CXX_STR("FILE"), LOG4CXX_STR("file")) || StringHelper::equalsIgnoreCase(option, LOG4CXX_STR("FILENAME"), LOG4CXX_STR("filename"))) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->fileName = stripDuplicateBackslashes(value); } else if (StringHelper::equalsIgnoreCase(option, LOG4CXX_STR("APPEND"), LOG4CXX_STR("append"))) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->fileAppend = OptionConverter::toBoolean(value, true); } else if (StringHelper::equalsIgnoreCase(option, LOG4CXX_STR("BUFFEREDIO"), LOG4CXX_STR("bufferedio"))) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->bufferedIO = OptionConverter::toBoolean(value, true); } else if (StringHelper::equalsIgnoreCase(option, LOG4CXX_STR("IMMEDIATEFLUSH"), LOG4CXX_STR("immediateflush"))) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->bufferedIO = !OptionConverter::toBoolean(value, false); } else if (StringHelper::equalsIgnoreCase(option, LOG4CXX_STR("BUFFERSIZE"), LOG4CXX_STR("buffersize"))) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->bufferSize = OptionConverter::toFileSize(value, 8 * 1024); } else @@ -148,7 +148,7 @@ void FileAppender::setOption(const LogString& option, void FileAppender::activateOptions(Pool& p) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); activateOptionsInternal(p); } diff --git a/src/main/cpp/multiprocessrollingfileappender.cpp b/src/main/cpp/multiprocessrollingfileappender.cpp index 29b02592..8224b1bb 100644 --- a/src/main/cpp/multiprocessrollingfileappender.cpp +++ b/src/main/cpp/multiprocessrollingfileappender.cpp @@ -114,7 +114,7 @@ void MultiprocessRollingFileAppender::activateOptions(Pool& p) } { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->triggeringPolicy->activateOptions(p); _priv->rollingPolicy->activateOptions(p); @@ -202,7 +202,7 @@ void MultiprocessRollingFileAppender::releaseFileLock(apr_file_t* lock_file) */ bool MultiprocessRollingFileAppender::rollover(Pool& p) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); return rolloverInternal(p); } diff --git a/src/main/cpp/rollingfileappender.cpp b/src/main/cpp/rollingfileappender.cpp index df553777..b2c00fe7 100644 --- a/src/main/cpp/rollingfileappender.cpp +++ b/src/main/cpp/rollingfileappender.cpp @@ -229,7 +229,7 @@ void RollingFileAppender::activateOptions(Pool& p) } { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->triggeringPolicy->activateOptions(p); _priv->rollingPolicy->activateOptions(p); @@ -301,7 +301,7 @@ void RollingFileAppender::activateOptions(Pool& p) */ bool RollingFileAppender::rollover(Pool& p) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); return rolloverInternal(p); } diff --git a/src/main/cpp/socketappenderskeleton.cpp b/src/main/cpp/socketappenderskeleton.cpp index 9f0117fa..8a4a4b0b 100644 --- a/src/main/cpp/socketappenderskeleton.cpp +++ b/src/main/cpp/socketappenderskeleton.cpp @@ -68,7 +68,7 @@ void SocketAppenderSkeleton::activateOptions(Pool& p) void SocketAppenderSkeleton::close() { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); if (_priv->closed) { @@ -147,7 +147,7 @@ void SocketAppenderSkeleton::setOption(const LogString& option, const LogString& void SocketAppenderSkeleton::fireConnector() { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); if ( !_priv->thread.joinable() ) { diff --git a/src/main/cpp/telnetappender.cpp b/src/main/cpp/telnetappender.cpp index 0c72a809..af1695f0 100644 --- a/src/main/cpp/telnetappender.cpp +++ b/src/main/cpp/telnetappender.cpp @@ -101,13 +101,13 @@ void TelnetAppender::setOption(const LogString& option, LogString TelnetAppender::getEncoding() const { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); return _priv->encoding; } void TelnetAppender::setEncoding(const LogString& value) { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->encoder = CharsetEncoder::getEncoder(value); _priv->encoding = value; } @@ -115,7 +115,7 @@ void TelnetAppender::setEncoding(const LogString& value) void TelnetAppender::close() { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); if (_priv->closed) { @@ -212,7 +212,7 @@ void TelnetAppender::append(const spi::LoggingEventPtr& event, Pool& p) LogString::const_iterator msgIter(msg.begin()); ByteBuffer buf(bytes, bytesSize); - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); while (msgIter != msg.end()) { @@ -268,7 +268,7 @@ void TelnetAppender::acceptConnections() // // find unoccupied connection // - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); for (ConnectionList::iterator iter = _priv->connections.begin(); iter != _priv->connections.end(); diff --git a/src/main/cpp/writerappender.cpp b/src/main/cpp/writerappender.cpp index ada3bd89..475c0268 100644 --- a/src/main/cpp/writerappender.cpp +++ b/src/main/cpp/writerappender.cpp @@ -158,7 +158,7 @@ bool WriterAppender::checkEntryConditions() const */ void WriterAppender::close() { - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); if (_priv->closed) { @@ -285,7 +285,7 @@ void WriterAppender::writeHeader(Pool& p) void WriterAppender::setWriter(const WriterPtr& newWriter) { - std::unique_lock<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); setWriterInternal(newWriter); } diff --git a/src/main/cpp/xmlsocketappender.cpp b/src/main/cpp/xmlsocketappender.cpp index 594b5905..9f839068 100644 --- a/src/main/cpp/xmlsocketappender.cpp +++ b/src/main/cpp/xmlsocketappender.cpp @@ -102,7 +102,7 @@ void XMLSocketAppender::setSocket(log4cxx::helpers::SocketPtr& socket, Pool& p) { OutputStreamPtr os = std::make_shared<SocketOutputStream>(socket); CharsetEncoderPtr charset(CharsetEncoder::getUTF8Encoder()); - std::lock_guard<std::recursive_mutex> lock(_priv->mutex); + AppenderScopeGuard lock(_priv->mutex); _priv->writer = std::make_shared<OutputStreamWriter>(os, charset); } diff --git a/src/main/include/CMakeLists.txt b/src/main/include/CMakeLists.txt index 205041da..ded2c6d7 100644 --- a/src/main/include/CMakeLists.txt +++ b/src/main/include/CMakeLists.txt @@ -123,24 +123,8 @@ CHECK_FUNCTION_EXISTS(wcstombs HAS_WCSTOMBS) CHECK_FUNCTION_EXISTS(fwide HAS_FWIDE) CHECK_LIBRARY_EXISTS(esmtp smtp_create_session "" HAS_LIBESMTP) CHECK_FUNCTION_EXISTS(syslog HAS_SYSLOG) -if(UNIX) - set(CMAKE_REQUIRED_LIBRARIES "pthread") - CHECK_SYMBOL_EXISTS(pthread_sigmask "signal.h" HAS_PTHREAD_SIGMASK) - - # Check for the (linux) pthread_setname_np. - # OSX and BSD are special apparently. OSX only lets you name - # the current thread, while BSD calls it pthread_set_name_np. - # Since this is really not a core feature and the end-user can configure - # it anyway, we're only going to worry about linux. - include(${LOG4CXX_SOURCE_DIR}/src/cmake/pthread/log4cxx-pthread.cmake) - if(${PTHREAD_SETNAME_NP_FOUND}) - set(HAS_PTHREAD_SETNAME 1) - endif() - if(${PTHREAD_GETNAME_NP_FOUND}) - set(HAS_PTHREAD_GETNAME 1) - endif() -endif(UNIX) +# Ensure log4cxx_private.h.in configurable variables are either 0 or 1 foreach(varName HAS_THREAD_LOCAL HAS_STD_LOCALE @@ -153,6 +137,7 @@ foreach(varName HAS_PTHREAD_SIGMASK HAS_PTHREAD_SETNAME HAS_PTHREAD_GETNAME + HAS_PTHREAD_SETPROTOCOL ) if(${varName} EQUAL 0) continue() diff --git a/src/main/include/log4cxx/appenderskeleton.h b/src/main/include/log4cxx/appenderskeleton.h index f91545e2..b7495a70 100644 --- a/src/main/include/log4cxx/appenderskeleton.h +++ b/src/main/include/log4cxx/appenderskeleton.h @@ -50,6 +50,11 @@ class LOG4CXX_EXPORT AppenderSkeleton : */ virtual void append(const spi::LoggingEventPtr& event, log4cxx::helpers::Pool& p) = 0; + /** + * Allow one thread at a time to check filters before passing \c event to AppenderSkeleton#append. + * + * The sub-class AppenderSkeleton#append method is responsible for the actual output. + */ void doAppendImpl(const spi::LoggingEventPtr& event, log4cxx::helpers::Pool& pool); public: @@ -130,12 +135,11 @@ class LOG4CXX_EXPORT AppenderSkeleton : */ bool isAsSevereAsThreshold(const LevelPtr& level) const; - /** - * This method performs threshold checks and invokes filters before - * delegating actual logging to the subclasses specific - * AppenderSkeleton#append method. - * */ + * Allow one thread at a time to check filters before passing \c event to AppenderSkeleton#append. + * + * The sub-class AppenderSkeleton#append method is responsible for the actual output. + */ void doAppend(const spi::LoggingEventPtr& event, helpers::Pool& pool) override; /** diff --git a/src/main/include/log4cxx/private/appenderskeleton_priv.h b/src/main/include/log4cxx/private/appenderskeleton_priv.h index 67b271f3..5edfcc5c 100644 --- a/src/main/include/log4cxx/private/appenderskeleton_priv.h +++ b/src/main/include/log4cxx/private/appenderskeleton_priv.h @@ -20,6 +20,7 @@ #include <log4cxx/appenderskeleton.h> #include <log4cxx/helpers/onlyonceerrorhandler.h> +#include <log4cxx/private/mutex.h> #include <memory> namespace log4cxx @@ -27,16 +28,12 @@ namespace log4cxx struct AppenderSkeleton::AppenderSkeletonPrivate { - AppenderSkeletonPrivate() : - threshold(Level::getAll()), - errorHandler(std::make_shared<log4cxx::helpers::OnlyOnceErrorHandler>()), - closed(false) {} - - AppenderSkeletonPrivate( LayoutPtr lay ) : + AppenderSkeletonPrivate( LayoutPtr lay = LayoutPtr() ) : layout( lay ), threshold(Level::getAll()), - errorHandler(std::make_shared<log4cxx::helpers::OnlyOnceErrorHandler>()), - closed(false) {} + errorHandler(std::make_shared<helpers::OnlyOnceErrorHandler>()), + closed(false) + { EnablePriorityInheritance(mutex, pool); } virtual ~AppenderSkeletonPrivate(){} @@ -68,8 +65,9 @@ struct AppenderSkeleton::AppenderSkeletonPrivate */ bool closed; - log4cxx::helpers::Pool pool; - mutable std::recursive_mutex mutex; + helpers::Pool pool; + + mutable AppenderMutexType mutex; }; } diff --git a/src/main/include/log4cxx/private/log4cxx_private.h.in b/src/main/include/log4cxx/private/log4cxx_private.h.in index 61e8e7ac..7b158f45 100644 --- a/src/main/include/log4cxx/private/log4cxx_private.h.in +++ b/src/main/include/log4cxx/private/log4cxx_private.h.in @@ -60,6 +60,7 @@ #define LOG4CXX_HAS_PTHREAD_SIGMASK @HAS_PTHREAD_SIGMASK@ #define LOG4CXX_HAS_PTHREAD_SETNAME @HAS_PTHREAD_SETNAME@ #define LOG4CXX_HAS_PTHREAD_GETNAME @HAS_PTHREAD_GETNAME@ +#define LOG4CXX_HAS_PTHREAD_SETPROTOCOL @HAS_PTHREAD_SETPROTOCOL@ #define LOG4CXX_HAS_THREAD_LOCAL @HAS_THREAD_LOCAL@ #endif diff --git a/src/main/include/log4cxx/private/mutex.h b/src/main/include/log4cxx/private/mutex.h new file mode 100644 index 00000000..6993f57d --- /dev/null +++ b/src/main/include/log4cxx/private/mutex.h @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef LOG4CXX_PRIV_MUTEX_HDR_ +#define LOG4CXX_PRIV_MUTEX_HDR_ +#include <mutex> +#include <log4cxx/helpers/loglog.h> +#include <log4cxx/helpers/stringhelper.h> +#if !defined(LOG4CXX) + #define LOG4CXX 1 +#endif +#include <log4cxx/private/log4cxx_private.h> + +namespace log4cxx +{ + +using AppenderMutexType = std::mutex; +using AppenderScopeGuard = std::lock_guard<AppenderMutexType>; +using AppenderScopedLock = std::unique_lock<AppenderMutexType>; + +// Change mutex attributes to avoid priority inversion problems +template <class M> + void +EnablePriorityInheritance(M& m, helpers::Pool& p) +{ +#if LOG4CXX_HAS_PTHREAD_SETPROTOCOL + pthread_mutexattr_t rrAttr; + if (pthread_mutexattr_init(&rrAttr) != 0) + { + LogString msg = LOG4CXX_STR("pthread_mutexattr_init error "); + helpers::toString(errno, p, msg) + helpers::LogLog::warn(msg); + } + else if (pthread_mutexattr_setprotocol(&rrAttr, PTHREAD_PRIO_INHERIT) != 0) + { + LogString msg = LOG4CXX_STR("pthread_mutexattr_setprotocol error "); + helpers::toString(errno, p, msg) + helpers::LogLog::warn(msg); + } + else if (pthread_mutex_destroy(m.native_handle()) != 0) + { + LogString msg = LOG4CXX_STR("pthread_mutex_destroy error "); + helpers::toString(errno, p, msg) + helpers::LogLog::warn(msg); + } + else if (pthread_mutex_init(m.native_handle(), &rrAttr) != 0) + { + LogString msg = LOG4CXX_STR("pthread_mutex_init error "); + helpers::toString(errno, p, msg) + helpers::LogLog::warn(msg); + } + pthread_mutexattr_destroy(&rrAttr); +#endif // LOG4CXX_HAS_PTHREAD_SETPROTOCOL +} + +} // namespace log4cxx + +#endif /* LOG4CXX_PRIV_MUTEX_HDR_ */
