From 091a9ce1e9961fa8d81473e1a8b72167a9a3f912 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Thu, 19 Aug 2021 21:08:10 -0400 Subject: [PATCH 01/17] LOGCXX-431 Update thread creation In order to make it possible to block signals when a thread is created, a new API to let users configure what to do when a thread is started has been created. Documentation on how Log4cxx handles threads, and information about good ways to handle signals has been added as well. --- src/main/cpp/CMakeLists.txt | 1 + src/main/cpp/asyncappender.cpp | 3 +- src/main/cpp/filewatchdog.cpp | 3 +- src/main/cpp/socketappenderskeleton.cpp | 3 +- src/main/cpp/sockethubappender.cpp | 3 +- src/main/cpp/telnetappender.cpp | 3 +- src/main/cpp/threadutility.cpp | 83 +++++++++++++ src/main/include/CMakeLists.txt | 12 +- .../include/log4cxx/helpers/threadutility.h | 114 ++++++++++++++++++ .../log4cxx/private/log4cxx_private.h.in | 4 + src/site/markdown/1-usage.md | 1 + src/site/markdown/threading.md | 82 +++++++++++++ 12 files changed, 306 insertions(+), 6 deletions(-) create mode 100644 src/main/cpp/threadutility.cpp create mode 100644 src/main/include/log4cxx/helpers/threadutility.h create mode 100644 src/site/markdown/threading.md diff --git a/src/main/cpp/CMakeLists.txt b/src/main/cpp/CMakeLists.txt index ff5375dc7..0f8f40d29 100644 --- a/src/main/cpp/CMakeLists.txt +++ b/src/main/cpp/CMakeLists.txt @@ -147,6 +147,7 @@ target_sources(log4cxx threadlocal.cpp threadpatternconverter.cpp threadspecificdata.cpp + threadutility.cpp throwableinformationpatternconverter.cpp timebasedrollingpolicy.cpp timezone.cpp diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 6da23b18d..2a4c27f30 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -30,6 +30,7 @@ #include #include #include +#include using namespace log4cxx; @@ -50,7 +51,7 @@ AsyncAppender::AsyncAppender() locationInfo(false), blocking(true) { - dispatcher = std::thread( &AsyncAppender::dispatch, this ); + dispatcher = ThreadUtility::createThread( "AyncAppend", &AsyncAppender::dispatch, this ); } AsyncAppender::~AsyncAppender() diff --git a/src/main/cpp/filewatchdog.cpp b/src/main/cpp/filewatchdog.cpp index e3dceb595..fb49a090d 100644 --- a/src/main/cpp/filewatchdog.cpp +++ b/src/main/cpp/filewatchdog.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include using namespace log4cxx; @@ -92,7 +93,7 @@ void FileWatchdog::start() { checkAndConfigure(); - thread = std::thread( &FileWatchdog::run, this ); + thread = ThreadUtility::createThread( "Filewatchdog", &FileWatchdog::run, this ); } bool FileWatchdog::is_interrupted() diff --git a/src/main/cpp/socketappenderskeleton.cpp b/src/main/cpp/socketappenderskeleton.cpp index 122de9d64..66bf9205e 100644 --- a/src/main/cpp/socketappenderskeleton.cpp +++ b/src/main/cpp/socketappenderskeleton.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include using namespace log4cxx; @@ -162,7 +163,7 @@ void SocketAppenderSkeleton::fireConnector() { LogLog::debug(LOG4CXX_STR("Connector thread not alive: starting monitor.")); - thread = std::thread( &SocketAppenderSkeleton::monitor, this ); + thread = ThreadUtility::createThread( "SocketAppend", &SocketAppenderSkeleton::monitor, this ); } } diff --git a/src/main/cpp/sockethubappender.cpp b/src/main/cpp/sockethubappender.cpp index a88eb42a3..dc6eb7cc2 100644 --- a/src/main/cpp/sockethubappender.cpp +++ b/src/main/cpp/sockethubappender.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include using namespace log4cxx; @@ -177,7 +178,7 @@ void SocketHubAppender::append(const spi::LoggingEventPtr& event, Pool& p) void SocketHubAppender::startServer() { - thread = std::thread( &SocketHubAppender::monitor, this ); + thread = ThreadUtility::createThread( "SocketHub", &SocketHubAppender::monitor, this ); } void SocketHubAppender::monitor() diff --git a/src/main/cpp/telnetappender.cpp b/src/main/cpp/telnetappender.cpp index 418aac009..222b982fb 100644 --- a/src/main/cpp/telnetappender.cpp +++ b/src/main/cpp/telnetappender.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include using namespace log4cxx; @@ -61,7 +62,7 @@ void TelnetAppender::activateOptions(Pool& /* p */) serverSocket->setSoTimeout(1000); } - sh = std::thread( &TelnetAppender::acceptConnections, this ); + sh = ThreadUtility::createThread( "TelnetAppend", &TelnetAppender::acceptConnections, this ); } void TelnetAppender::setOption(const LogString& option, diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp new file mode 100644 index 000000000..bdd301bb2 --- /dev/null +++ b/src/main/cpp/threadutility.cpp @@ -0,0 +1,83 @@ +#include "log4cxx/helpers/threadutility.h" +#include "log4cxx/private/log4cxx_private.h" +#include "log4cxx/helpers/loglog.h" + +#include + +#if LOG4CXX_HAS_SETTHREADDESCRIPTION +#include +#include +#endif + +using log4cxx::ThreadUtility; + +log4cxx::pre_thread_start ThreadUtility::pre_start = ThreadUtility::preThreadDoNothing; +log4cxx::thread_started ThreadUtility::thread_start = ThreadUtility::threadStartedDoNothing; +log4cxx::post_thread_start ThreadUtility::post_start = ThreadUtility::postThreadDoNothing; + +ThreadUtility::ThreadUtility(){} + +void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1, + thread_started started, + post_thread_start post_start1 ){ + if( pre_start1 == nullptr ){ + pre_start = ThreadUtility::preThreadDoNothing; + }else{ + pre_start = pre_start1; + } + + if( started == nullptr ){ + thread_start = ThreadUtility::threadStartedDoNothing; + }else{ + thread_start = started; + } + + if( post_start1 == nullptr ){ + post_start = ThreadUtility::postThreadDoNothing; + }else{ + post_start = pre_start1; + } +} + +void ThreadUtility::preThreadDoNothing(){} + +void ThreadUtility::preThreadBlockSignals(){ +#if LOG4CXX_HAS_PTHREAD_SIGMASK + sigset_t set; + sigfillset(&set); + if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){ + LOGLOG_ERROR( "Unable to set thread sigmask" ); + } +#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ +} + +void ThreadUtility::threadStartedDoNothing(LogString, + std::thread::id, + std::thread::native_handle_type){} + +void ThreadUtility::threadStartedNameThread(LogString threadName, + std::thread::id /*thread_id*/, + std::thread::native_handle_type native_handle){ +#if LOG4CXX_HAS_PTHREAD_SETNAME + if( pthread_setname_np( static_cast( native_handle ), threadName.c_str() ) < 0 ){ + LOGLOG_ERROR( "unable to set thread name" ); + } +#elif LOG4CXX_HAS_SETTHREADDESCRIPTION + HRESULT hr = SetThreadDescription(static_cast(native_handle), threadName.c_str()); + if(FAILED(hr)){ + LOGLOG_ERROR( "unable to set thread name" ); + } +#endif +} + +void ThreadUtility::postThreadDoNothing(){} + +void ThreadUtility::postThreadUnblockSignals(){ +#if LOG4CXX_HAS_PTHREAD_SIGMASK + sigset_t set; + sigemptyset(&set); + if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){ + LOGLOG_ERROR( "Unable to set thread sigmask" ); + } +#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ +} diff --git a/src/main/include/CMakeLists.txt b/src/main/include/CMakeLists.txt index 584941c97..575f68bab 100644 --- a/src/main/include/CMakeLists.txt +++ b/src/main/include/CMakeLists.txt @@ -85,8 +85,18 @@ 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) + set(CMAKE_REQUIRED_LIBRARIES "pthread") + CHECK_SYMBOL_EXISTS(pthread_setname_np "pthread.h" HAS_PTHREAD_SETNAME) +endif(UNIX) -foreach(varName HAS_STD_LOCALE HAS_ODBC HAS_MBSRTOWCS HAS_WCSTOMBS HAS_FWIDE HAS_LIBESMTP HAS_SYSLOG) +if(WIN32) + CHECK_SYMBOL_EXISTS(SetThreadDescription "windows.h;processthreadsapi.h" HAS_SETTHREADDESCRIPTION) +endif(WIN32) + +foreach(varName HAS_STD_LOCALE HAS_ODBC HAS_MBSRTOWCS HAS_WCSTOMBS HAS_FWIDE HAS_LIBESMTP HAS_SYSLOG HAS_PTHREAD_SIGMASK HAS_PTHREAD_SETNAME HAS_SETTHREADDESCRIPTION) if(${varName} EQUAL 0) continue() elseif(${varName} EQUAL 1) diff --git a/src/main/include/log4cxx/helpers/threadutility.h b/src/main/include/log4cxx/helpers/threadutility.h new file mode 100644 index 000000000..de4ffadc4 --- /dev/null +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -0,0 +1,114 @@ +#ifndef _LOG4CXX_THREADUTILITY_H +#define _LOG4CXX_THREADUTILITY_H + +#include +#include + +#include "log4cxx/logstring.h" + +namespace log4cxx { + +/** + * A function that will be called before a thread is started. This can + * be used to (for example) block all of the signals in the thread, so + * that when the thread is created it will have a correct signal mask. + */ +typedef std::function pre_thread_start; + +/** + * Called when a new thread has started. This can be used to set + * parameters for the thread in a platform-specific manner. + * + * @param threadName The name of the thread + * @param thread_id The ID of the thread as reported by std::thread::get_id + * @param native_handle The native handle of the thread, as reported by + * std::thread::native_handle + */ +typedef std::function thread_started; + +/** + * Called after a thread has started. This can be used to (for example) + * unblock the signals in the thread. + */ +typedef std::function post_thread_start; + +class LOG4CXX_EXPORT ThreadUtility { +public: + /** + * Configure the thread functions that log4cxx will use. + * Note that setting any of these parameters to nullptr will cause the + * default function to be used. + * + */ + static void configureThreadFunctions( pre_thread_start pre_start, + thread_started started, + post_thread_start post_start ); + + /** + * A pre-start thread function that does nothing + */ + static void preThreadDoNothing(); + + /** + * A pre-start thread function that blocks signals to the new thread + * (if the system has pthreads). If the system does not have pthreads, + * is equivalent to preThreadDoNothing(); + */ + static void preThreadBlockSignals(); + + /** + * A thread_started function that does nothing when the thread starts. + */ + static void threadStartedDoNothing(LogString threadName, + std::thread::id thread_id, + std::thread::native_handle_type native_handle); + + /** + * A thread_started function that names the thread using the appropriate + * system call. + */ + static void threadStartedNameThread(LogString threadName, + std::thread::id thread_id, + std::thread::native_handle_type native_handle); + + /** + * A post-start thread function that does nothing. + */ + static void postThreadDoNothing(); + + /** + * A post-start thread function that unblocks signals that preThreadBlockSignals + * blocked before starting the thread. If the system does not have pthreads, + * is equivalent to postThreadDoNothing(); + */ + static void postThreadUnblockSignals(); + + /** + * Start a thread + */ + template + static std::thread createThread(LogString name, + Function&& f, + Args&&... args){ + pre_start(); + std::thread t( f, args... ); + thread_start( name, + t.get_id(), + t.native_handle() ); + post_start(); + return t; + } + +private: + ThreadUtility(); + + static log4cxx::pre_thread_start pre_start; + static log4cxx::thread_started thread_start; + static log4cxx::post_thread_start post_start; +}; + +} + +#endif diff --git a/src/main/include/log4cxx/private/log4cxx_private.h.in b/src/main/include/log4cxx/private/log4cxx_private.h.in index 280f21f0e..bf965c937 100644 --- a/src/main/include/log4cxx/private/log4cxx_private.h.in +++ b/src/main/include/log4cxx/private/log4cxx_private.h.in @@ -53,6 +53,10 @@ #define LOG4CXX_WIN32_THREAD_FMTSPEC "0x%.8x" #define LOG4CXX_APR_THREAD_FMTSPEC "0x%pt" +#define LOG4CXX_HAS_PTHREAD_SIGMASK @HAS_PTHREAD_SIGMASK@ +#define LOG4CXX_HAS_PTHREAD_SETNAME @HAS_PTHREAD_SETNAME@ +#define LOG4CXX_HAS_SETTHREADDESCRIPTION @HAS_SETTHREADDESCRIPTION@ + #ifdef __BORLANDC__ /* * embarcadero/RAD Studio compilers don't support thread_local: diff --git a/src/site/markdown/1-usage.md b/src/site/markdown/1-usage.md index f8c6fe732..379c45b60 100644 --- a/src/site/markdown/1-usage.md +++ b/src/site/markdown/1-usage.md @@ -24,6 +24,7 @@ Usage {#usage-overview} See the following pages for usage information: * @subpage usage +* @subpage threading * @subpage extending-log4cxx * @subpage faq * @subpage configuration-samples diff --git a/src/site/markdown/threading.md b/src/site/markdown/threading.md new file mode 100644 index 000000000..045f31916 --- /dev/null +++ b/src/site/markdown/threading.md @@ -0,0 +1,82 @@ +Threading {#threading} +=== + + +# Threading Notes with Log4cxx + +Log4cxx is designed to be thread-safe under all circumstances. However, +this is not always the case. + +## Unexpected Exit + +In multithreaded applications, it is possible to call `exit()` from any +thread in the application. When this happens, other threads in the +application may continue to run and attempt to log information. As of +version 12 of Log4cxx, this should not cause the library to crash. + +We recommend that a graceful exit be performed whenever possible, and that +all threads be terminated properly before returning from `main()`. + +See [LOGCXX-322][3] for more information. + +## Threads Created by Log4cxx + +Under certain configurations, Log4cxx may create new threads in order to do +tasks(e.g. network comms, other async operations). On Linux systems, this +can lead to undesirable signal delivery, as signals can be delivered to +any thread in the process. + +To handle signals properly on Linux, you may wish to utilize the [signalfd][1] +API to handle signals correctly. Another way of handling signals is to +create a pipe internal to your application to notify the main loop that there +is a signal available - see the [Qt documentation][2] for more information. + +Log4cxx provides a mechanism for defining methods to be called at two main +points in the lifecycle of a thread: + +1. Just before the thread starts +2. Just after the thread starts + +These two points are intended to let client code determine how best to start +threads. Log4cxx provides a basic implementation of these for POSIX in order +to block signals to the new threads that it creates. + +Once a new thread is created, there is also a callback function that lets +client code do operations on the thread directly. A sample method in Log4cxx +has a callback to name the thread in order to make debugging nicer. + +In order to use these callback functions, use the ThreadUtility class. You +can use some sample functions(not no-ops) as follows: + +``` +ThreadUtility::configureThreadFunctions( ThreadUtility::preThreadBlockSignals, + ThreadUtility::threadStartedNameThread, + ThreadUtility::postThreadUnblockSignals ); +``` + +These sample functions will block all POSIX signals before starting a new thread, +and then unblock them once the thread has been created. You may provide your +own functions to handle this if you so choose. + + +[1]: https://man7.org/linux/man-pages/man2/signalfd.2.html +[2]: https://doc.qt.io/qt-5/unix-signals.html +[3]: https://issues.apache.org/jira/browse/LOGCXX-322 From 36e33e5ffdf44057f5cdb7cb3414c463635c2671 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Thu, 19 Aug 2021 21:28:47 -0400 Subject: [PATCH 02/17] Check for linux-specific pthread_setname_np --- src/cmake/pthread/log4cxx-pthread.cmake | 5 +++++ src/cmake/pthread/test-pthread.cpp | 6 ++++++ src/main/include/CMakeLists.txt | 10 ++++++++-- 3 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 src/cmake/pthread/log4cxx-pthread.cmake create mode 100644 src/cmake/pthread/test-pthread.cpp diff --git a/src/cmake/pthread/log4cxx-pthread.cmake b/src/cmake/pthread/log4cxx-pthread.cmake new file mode 100644 index 000000000..aa3647bf6 --- /dev/null +++ b/src/cmake/pthread/log4cxx-pthread.cmake @@ -0,0 +1,5 @@ +include(FindThreads) + +try_compile(PTHREAD_SETNAME_NP_FOUND "${CMAKE_BINARY_DIR}/pthread-compile-tests" + "${CMAKE_CURRENT_LIST_DIR}/test-pthread.cpp") + diff --git a/src/cmake/pthread/test-pthread.cpp b/src/cmake/pthread/test-pthread.cpp new file mode 100644 index 000000000..f30cd1ade --- /dev/null +++ b/src/cmake/pthread/test-pthread.cpp @@ -0,0 +1,6 @@ +#include + +int main(){ + pthread_t tid; + pthread_set_name_np(tid, "name"); +} diff --git a/src/main/include/CMakeLists.txt b/src/main/include/CMakeLists.txt index 575f68bab..558eaeb62 100644 --- a/src/main/include/CMakeLists.txt +++ b/src/main/include/CMakeLists.txt @@ -88,8 +88,14 @@ CHECK_FUNCTION_EXISTS(syslog HAS_SYSLOG) if(UNIX) set(CMAKE_REQUIRED_LIBRARIES "pthread") CHECK_SYMBOL_EXISTS(pthread_sigmask "signal.h" HAS_PTHREAD_SIGMASK) - set(CMAKE_REQUIRED_LIBRARIES "pthread") - CHECK_SYMBOL_EXISTS(pthread_setname_np "pthread.h" HAS_PTHREAD_SETNAME) + + # 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) + set(HAS_PTHREAD_SETNAME ${PTHREAD_SETNAME_NP_FOUND}) endif(UNIX) if(WIN32) From 0b08d4e0bd17e13fd800ca88dd63803c6e4c24a0 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 22:16:37 -0400 Subject: [PATCH 03/17] Create strings as LOGSTRING to compile on windows --- src/main/cpp/asyncappender.cpp | 2 +- src/main/cpp/filewatchdog.cpp | 2 +- src/main/cpp/socketappenderskeleton.cpp | 2 +- src/main/cpp/sockethubappender.cpp | 2 +- src/main/cpp/telnetappender.cpp | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 2a4c27f30..c9f87ad14 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -51,7 +51,7 @@ AsyncAppender::AsyncAppender() locationInfo(false), blocking(true) { - dispatcher = ThreadUtility::createThread( "AyncAppend", &AsyncAppender::dispatch, this ); + dispatcher = ThreadUtility::createThread( LOG4CXX_STR("AyncAppend"), &AsyncAppender::dispatch, this ); } AsyncAppender::~AsyncAppender() diff --git a/src/main/cpp/filewatchdog.cpp b/src/main/cpp/filewatchdog.cpp index fb49a090d..14942cdbf 100644 --- a/src/main/cpp/filewatchdog.cpp +++ b/src/main/cpp/filewatchdog.cpp @@ -93,7 +93,7 @@ void FileWatchdog::start() { checkAndConfigure(); - thread = ThreadUtility::createThread( "Filewatchdog", &FileWatchdog::run, this ); + thread = ThreadUtility::createThread( LOG4CXX_STR("Filewatchdog"), &FileWatchdog::run, this ); } bool FileWatchdog::is_interrupted() diff --git a/src/main/cpp/socketappenderskeleton.cpp b/src/main/cpp/socketappenderskeleton.cpp index 66bf9205e..a310cba8c 100644 --- a/src/main/cpp/socketappenderskeleton.cpp +++ b/src/main/cpp/socketappenderskeleton.cpp @@ -163,7 +163,7 @@ void SocketAppenderSkeleton::fireConnector() { LogLog::debug(LOG4CXX_STR("Connector thread not alive: starting monitor.")); - thread = ThreadUtility::createThread( "SocketAppend", &SocketAppenderSkeleton::monitor, this ); + thread = ThreadUtility::createThread( LOG4CXX_STR("SocketAppend"), &SocketAppenderSkeleton::monitor, this ); } } diff --git a/src/main/cpp/sockethubappender.cpp b/src/main/cpp/sockethubappender.cpp index dc6eb7cc2..73d821a0e 100644 --- a/src/main/cpp/sockethubappender.cpp +++ b/src/main/cpp/sockethubappender.cpp @@ -178,7 +178,7 @@ void SocketHubAppender::append(const spi::LoggingEventPtr& event, Pool& p) void SocketHubAppender::startServer() { - thread = ThreadUtility::createThread( "SocketHub", &SocketHubAppender::monitor, this ); + thread = ThreadUtility::createThread( LOG4CXX_STR("SocketHub"), &SocketHubAppender::monitor, this ); } void SocketHubAppender::monitor() diff --git a/src/main/cpp/telnetappender.cpp b/src/main/cpp/telnetappender.cpp index 222b982fb..9dc813a3f 100644 --- a/src/main/cpp/telnetappender.cpp +++ b/src/main/cpp/telnetappender.cpp @@ -62,7 +62,7 @@ void TelnetAppender::activateOptions(Pool& /* p */) serverSocket->setSoTimeout(1000); } - sh = ThreadUtility::createThread( "TelnetAppend", &TelnetAppender::acceptConnections, this ); + sh = ThreadUtility::createThread( LOG4CXX_STR("TelnetAppend"), &TelnetAppender::acceptConnections, this ); } void TelnetAppender::setOption(const LogString& option, From 60184d702279dd279cbd30e2e1596634cc47c979 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 22:26:38 -0400 Subject: [PATCH 04/17] More logstrings --- src/main/cpp/threadutility.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index bdd301bb2..c7990d9b0 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -46,7 +46,7 @@ void ThreadUtility::preThreadBlockSignals(){ sigset_t set; sigfillset(&set); if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){ - LOGLOG_ERROR( "Unable to set thread sigmask" ); + LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); } #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } @@ -60,12 +60,12 @@ void ThreadUtility::threadStartedNameThread(LogString threadName, std::thread::native_handle_type native_handle){ #if LOG4CXX_HAS_PTHREAD_SETNAME if( pthread_setname_np( static_cast( native_handle ), threadName.c_str() ) < 0 ){ - LOGLOG_ERROR( "unable to set thread name" ); + LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") ); } #elif LOG4CXX_HAS_SETTHREADDESCRIPTION HRESULT hr = SetThreadDescription(static_cast(native_handle), threadName.c_str()); if(FAILED(hr)){ - LOGLOG_ERROR( "unable to set thread name" ); + LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") ); } #endif } @@ -77,7 +77,7 @@ void ThreadUtility::postThreadUnblockSignals(){ sigset_t set; sigemptyset(&set); if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){ - LOGLOG_ERROR( "Unable to set thread sigmask" ); + LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); } #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } From 3687a8b253a196786b45b506b03399fc530fecd9 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 22:33:44 -0400 Subject: [PATCH 05/17] Fix pthread name test for linux --- src/cmake/pthread/log4cxx-pthread.cmake | 3 ++- src/cmake/pthread/test-pthread.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cmake/pthread/log4cxx-pthread.cmake b/src/cmake/pthread/log4cxx-pthread.cmake index aa3647bf6..d4a527225 100644 --- a/src/cmake/pthread/log4cxx-pthread.cmake +++ b/src/cmake/pthread/log4cxx-pthread.cmake @@ -1,5 +1,6 @@ include(FindThreads) try_compile(PTHREAD_SETNAME_NP_FOUND "${CMAKE_BINARY_DIR}/pthread-compile-tests" - "${CMAKE_CURRENT_LIST_DIR}/test-pthread.cpp") + "${CMAKE_CURRENT_LIST_DIR}/test-pthread.cpp" + LINK_LIBRARIES Threads::Threads ) diff --git a/src/cmake/pthread/test-pthread.cpp b/src/cmake/pthread/test-pthread.cpp index f30cd1ade..73f453cc7 100644 --- a/src/cmake/pthread/test-pthread.cpp +++ b/src/cmake/pthread/test-pthread.cpp @@ -2,5 +2,5 @@ int main(){ pthread_t tid; - pthread_set_name_np(tid, "name"); + pthread_setname_np(tid, "name"); } From 39bbf1b3bc17b60c7fc54c7f3e20ffa18ae2770d Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 23:19:01 -0400 Subject: [PATCH 06/17] Turned the thread utility into a singleton --- src/main/cpp/asyncappender.cpp | 2 +- src/main/cpp/filewatchdog.cpp | 2 +- src/main/cpp/socketappenderskeleton.cpp | 2 +- src/main/cpp/sockethubappender.cpp | 2 +- src/main/cpp/telnetappender.cpp | 2 +- src/main/cpp/threadutility.cpp | 66 ++++++++++-------- src/main/include/CMakeLists.txt | 2 + .../include/log4cxx/helpers/threadutility.h | 68 ++++++++++--------- 8 files changed, 80 insertions(+), 66 deletions(-) diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index c9f87ad14..7ab05b788 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -51,7 +51,7 @@ AsyncAppender::AsyncAppender() locationInfo(false), blocking(true) { - dispatcher = ThreadUtility::createThread( LOG4CXX_STR("AyncAppend"), &AsyncAppender::dispatch, this ); + dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AyncAppend"), &AsyncAppender::dispatch, this ); } AsyncAppender::~AsyncAppender() diff --git a/src/main/cpp/filewatchdog.cpp b/src/main/cpp/filewatchdog.cpp index 14942cdbf..3848ed391 100644 --- a/src/main/cpp/filewatchdog.cpp +++ b/src/main/cpp/filewatchdog.cpp @@ -93,7 +93,7 @@ void FileWatchdog::start() { checkAndConfigure(); - thread = ThreadUtility::createThread( LOG4CXX_STR("Filewatchdog"), &FileWatchdog::run, this ); + thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("Filewatchdog"), &FileWatchdog::run, this ); } bool FileWatchdog::is_interrupted() diff --git a/src/main/cpp/socketappenderskeleton.cpp b/src/main/cpp/socketappenderskeleton.cpp index a310cba8c..e3e137506 100644 --- a/src/main/cpp/socketappenderskeleton.cpp +++ b/src/main/cpp/socketappenderskeleton.cpp @@ -163,7 +163,7 @@ void SocketAppenderSkeleton::fireConnector() { LogLog::debug(LOG4CXX_STR("Connector thread not alive: starting monitor.")); - thread = ThreadUtility::createThread( LOG4CXX_STR("SocketAppend"), &SocketAppenderSkeleton::monitor, this ); + thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("SocketAppend"), &SocketAppenderSkeleton::monitor, this ); } } diff --git a/src/main/cpp/sockethubappender.cpp b/src/main/cpp/sockethubappender.cpp index 73d821a0e..7d83a23a4 100644 --- a/src/main/cpp/sockethubappender.cpp +++ b/src/main/cpp/sockethubappender.cpp @@ -178,7 +178,7 @@ void SocketHubAppender::append(const spi::LoggingEventPtr& event, Pool& p) void SocketHubAppender::startServer() { - thread = ThreadUtility::createThread( LOG4CXX_STR("SocketHub"), &SocketHubAppender::monitor, this ); + thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("SocketHub"), &SocketHubAppender::monitor, this ); } void SocketHubAppender::monitor() diff --git a/src/main/cpp/telnetappender.cpp b/src/main/cpp/telnetappender.cpp index 9dc813a3f..7e1d6f5d7 100644 --- a/src/main/cpp/telnetappender.cpp +++ b/src/main/cpp/telnetappender.cpp @@ -62,7 +62,7 @@ void TelnetAppender::activateOptions(Pool& /* p */) serverSocket->setSoTimeout(1000); } - sh = ThreadUtility::createThread( LOG4CXX_STR("TelnetAppend"), &TelnetAppender::acceptConnections, this ); + sh = ThreadUtility::instance()->createThread( LOG4CXX_STR("TelnetAppend"), &TelnetAppender::acceptConnections, this ); } void TelnetAppender::setOption(const LogString& option, diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index c7990d9b0..165803bc6 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -11,35 +11,36 @@ using log4cxx::ThreadUtility; -log4cxx::pre_thread_start ThreadUtility::pre_start = ThreadUtility::preThreadDoNothing; -log4cxx::thread_started ThreadUtility::thread_start = ThreadUtility::threadStartedDoNothing; -log4cxx::post_thread_start ThreadUtility::post_start = ThreadUtility::postThreadDoNothing; +struct ThreadUtility::priv_data{ + priv_data(){ + pre_start = nullptr; + thread_start = nullptr; + post_start = nullptr; + } -ThreadUtility::ThreadUtility(){} + log4cxx::pre_thread_start pre_start; + log4cxx::thread_started thread_start; + log4cxx::post_thread_start post_start; +}; -void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1, - thread_started started, - post_thread_start post_start1 ){ - if( pre_start1 == nullptr ){ - pre_start = ThreadUtility::preThreadDoNothing; - }else{ - pre_start = pre_start1; - } +ThreadUtility::ThreadUtility() : + m_priv( new priv_data() ) +{} - if( started == nullptr ){ - thread_start = ThreadUtility::threadStartedDoNothing; - }else{ - thread_start = started; - } +ThreadUtility::~ThreadUtility(){} - if( post_start1 == nullptr ){ - post_start = ThreadUtility::postThreadDoNothing; - }else{ - post_start = pre_start1; - } +std::shared_ptr ThreadUtility::instance(){ + static std::shared_ptr instance( new ThreadUtility() ); + return instance; } -void ThreadUtility::preThreadDoNothing(){} +void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1, + thread_started started, + post_thread_start post_start1 ){ + m_priv->pre_start = pre_start1; + m_priv->thread_start = started; + m_priv->post_start = post_start1; +} void ThreadUtility::preThreadBlockSignals(){ #if LOG4CXX_HAS_PTHREAD_SIGMASK @@ -51,10 +52,6 @@ void ThreadUtility::preThreadBlockSignals(){ #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } -void ThreadUtility::threadStartedDoNothing(LogString, - std::thread::id, - std::thread::native_handle_type){} - void ThreadUtility::threadStartedNameThread(LogString threadName, std::thread::id /*thread_id*/, std::thread::native_handle_type native_handle){ @@ -70,8 +67,6 @@ void ThreadUtility::threadStartedNameThread(LogString threadName, #endif } -void ThreadUtility::postThreadDoNothing(){} - void ThreadUtility::postThreadUnblockSignals(){ #if LOG4CXX_HAS_PTHREAD_SIGMASK sigset_t set; @@ -81,3 +76,16 @@ void ThreadUtility::postThreadUnblockSignals(){ } #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } + + +log4cxx::pre_thread_start ThreadUtility::preStartFunction(){ + return m_priv->pre_start; +} + +log4cxx::thread_started ThreadUtility::threadStartedFunction(){ + return m_priv->thread_start; +} + +log4cxx::post_thread_start ThreadUtility::postStartFunction(){ + return m_priv->post_start; +} diff --git a/src/main/include/CMakeLists.txt b/src/main/include/CMakeLists.txt index 558eaeb62..9905434f9 100644 --- a/src/main/include/CMakeLists.txt +++ b/src/main/include/CMakeLists.txt @@ -96,6 +96,8 @@ if(UNIX) # it anyway, we're only going to worry about linux. include(${LOG4CXX_SOURCE_DIR}/src/cmake/pthread/log4cxx-pthread.cmake) set(HAS_PTHREAD_SETNAME ${PTHREAD_SETNAME_NP_FOUND}) + + message("setname np: ${PTHREAD_SETNAME_NP_FOUND}") endif(UNIX) if(WIN32) diff --git a/src/main/include/log4cxx/helpers/threadutility.h b/src/main/include/log4cxx/helpers/threadutility.h index de4ffadc4..4111a0d1b 100644 --- a/src/main/include/log4cxx/helpers/threadutility.h +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -3,6 +3,7 @@ #include #include +#include #include "log4cxx/logstring.h" @@ -34,79 +35,82 @@ typedef std::function post_thread_start; +class ThreadUtility; +LOG4CXX_PTR_DEF(ThreadUtility); + class LOG4CXX_EXPORT ThreadUtility { +private: + ThreadUtility(); + + log4cxx::pre_thread_start preStartFunction(); + log4cxx::thread_started threadStartedFunction(); + log4cxx::post_thread_start postStartFunction(); + + struct priv_data; + std::unique_ptr m_priv; + public: + ~ThreadUtility(); + + static std::shared_ptr instance(); + /** * Configure the thread functions that log4cxx will use. * Note that setting any of these parameters to nullptr will cause the * default function to be used. * */ - static void configureThreadFunctions( pre_thread_start pre_start, + void configureThreadFunctions( pre_thread_start pre_start, thread_started started, post_thread_start post_start ); - /** - * A pre-start thread function that does nothing - */ - static void preThreadDoNothing(); - /** * A pre-start thread function that blocks signals to the new thread * (if the system has pthreads). If the system does not have pthreads, * is equivalent to preThreadDoNothing(); */ - static void preThreadBlockSignals(); - - /** - * A thread_started function that does nothing when the thread starts. - */ - static void threadStartedDoNothing(LogString threadName, - std::thread::id thread_id, - std::thread::native_handle_type native_handle); + void preThreadBlockSignals(); /** * A thread_started function that names the thread using the appropriate * system call. */ - static void threadStartedNameThread(LogString threadName, + void threadStartedNameThread(LogString threadName, std::thread::id thread_id, std::thread::native_handle_type native_handle); - /** - * A post-start thread function that does nothing. - */ - static void postThreadDoNothing(); - /** * A post-start thread function that unblocks signals that preThreadBlockSignals * blocked before starting the thread. If the system does not have pthreads, * is equivalent to postThreadDoNothing(); */ - static void postThreadUnblockSignals(); + void postThreadUnblockSignals(); /** * Start a thread */ template - static std::thread createThread(LogString name, + std::thread createThread(LogString name, Function&& f, Args&&... args){ - pre_start(); + log4cxx::pre_thread_start pre_start = preStartFunction(); + log4cxx::thread_started thread_start = threadStartedFunction(); + log4cxx::post_thread_start post_start = postStartFunction(); + + if( pre_start ){ + pre_start(); + } std::thread t( f, args... ); - thread_start( name, + if( thread_start ){ + thread_start( name, t.get_id(), t.native_handle() ); - post_start(); + } + if( post_start ){ + post_start(); + } return t; } - -private: - ThreadUtility(); - - static log4cxx::pre_thread_start pre_start; - static log4cxx::thread_started thread_start; - static log4cxx::post_thread_start post_start; }; } From 6e725a7883743e1b3b4c0130aa0906c36ad96b84 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 23:26:48 -0400 Subject: [PATCH 07/17] Made documentation on logging threaded clearer --- src/site/markdown/threading.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/site/markdown/threading.md b/src/site/markdown/threading.md index 045f31916..6bf373e55 100644 --- a/src/site/markdown/threading.md +++ b/src/site/markdown/threading.md @@ -22,8 +22,9 @@ Threading {#threading} --> # Threading Notes with Log4cxx -Log4cxx is designed to be thread-safe under all circumstances. However, -this is not always the case. +Log4cxx is designed to be thread-safe under under normal usage. This +means that logging itself is always thread-safe, however there are +certain circumstances that can cause threading issues with Log4cxx. ## Unexpected Exit From 46ad073499f147d0b3d46e8f8f0a4dc033b12738 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 23:42:18 -0400 Subject: [PATCH 08/17] setname actually tests properly --- src/main/include/CMakeLists.txt | 6 +++--- src/main/include/log4cxx/helpers/threadutility.h | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/include/CMakeLists.txt b/src/main/include/CMakeLists.txt index 9905434f9..ce3b4e867 100644 --- a/src/main/include/CMakeLists.txt +++ b/src/main/include/CMakeLists.txt @@ -95,9 +95,9 @@ if(UNIX) # 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) - set(HAS_PTHREAD_SETNAME ${PTHREAD_SETNAME_NP_FOUND}) - - message("setname np: ${PTHREAD_SETNAME_NP_FOUND}") + if(${PTHREAD_SETNAME_NP_FOUND}) + set(HAS_PTHREAD_SETNAME 1) + endif() endif(UNIX) if(WIN32) diff --git a/src/main/include/log4cxx/helpers/threadutility.h b/src/main/include/log4cxx/helpers/threadutility.h index 4111a0d1b..246e284a7 100644 --- a/src/main/include/log4cxx/helpers/threadutility.h +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -56,9 +56,8 @@ class LOG4CXX_EXPORT ThreadUtility { /** * Configure the thread functions that log4cxx will use. - * Note that setting any of these parameters to nullptr will cause the - * default function to be used. - * + * Note that setting any of these parameters to nullptr is valid, + * and simply results in the callback not being called. * */ void configureThreadFunctions( pre_thread_start pre_start, thread_started started, From a82c4d9584a713ab4ef51cb6ee6a804b6083b1d2 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Fri, 20 Aug 2021 23:43:31 -0400 Subject: [PATCH 09/17] Fixed documentation --- src/site/markdown/threading.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/site/markdown/threading.md b/src/site/markdown/threading.md index 6bf373e55..daed9e19a 100644 --- a/src/site/markdown/threading.md +++ b/src/site/markdown/threading.md @@ -68,7 +68,7 @@ In order to use these callback functions, use the ThreadUtility class. You can use some sample functions(not no-ops) as follows: ``` -ThreadUtility::configureThreadFunctions( ThreadUtility::preThreadBlockSignals, +ThreadUtility::instance()->configureThreadFunctions( ThreadUtility::preThreadBlockSignals, ThreadUtility::threadStartedNameThread, ThreadUtility::postThreadUnblockSignals ); ``` From 187c1812479266aadc7a010a572eccc7cdf523e9 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Sat, 21 Aug 2021 11:25:32 -0400 Subject: [PATCH 10/17] Added unit test for the thread utility --- src/test/cpp/helpers/CMakeLists.txt | 1 + .../cpp/helpers/threadutilitytestcase.cpp | 74 +++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100644 src/test/cpp/helpers/threadutilitytestcase.cpp diff --git a/src/test/cpp/helpers/CMakeLists.txt b/src/test/cpp/helpers/CMakeLists.txt index c40681cd5..fda8194bd 100644 --- a/src/test/cpp/helpers/CMakeLists.txt +++ b/src/test/cpp/helpers/CMakeLists.txt @@ -18,6 +18,7 @@ set(HELPER_TESTS syslogwritertest timezonetestcase transcodertestcase + threadutilitytestcase ) foreach(fileName IN LISTS HELPER_TESTS) add_executable(${fileName} "${fileName}.cpp") diff --git a/src/test/cpp/helpers/threadutilitytestcase.cpp b/src/test/cpp/helpers/threadutilitytestcase.cpp new file mode 100644 index 000000000..1aacb62c1 --- /dev/null +++ b/src/test/cpp/helpers/threadutilitytestcase.cpp @@ -0,0 +1,74 @@ +/* + * 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. + */ + +#include "../logunit.h" +#include + +using namespace log4cxx; +using namespace log4cxx::helpers; + + +LOGUNIT_CLASS(ThreadUtilityTest) +{ + LOGUNIT_TEST_SUITE(ThreadUtilityTest); + LOGUNIT_TEST(testNullFunctions); + LOGUNIT_TEST(testCustomFunctions); + LOGUNIT_TEST_SUITE_END(); + +public: + void testNullFunctions(){ + ThreadUtilityPtr thrUtil = ThreadUtility::instance(); + + thrUtil->configureThreadFunctions( nullptr, nullptr, nullptr ); + + std::thread t = thrUtil->createThread( "FooName", [](){} ); + + t.join(); + } + + void testCustomFunctions(){ + ThreadUtilityPtr thrUtil = ThreadUtility::instance(); + int num_pre = 0; + int num_started = 0; + int num_post = 0; + + thrUtil->configureThreadFunctions( [&num_pre](){ + num_pre++; + }, + [&num_started]( LogString, + std::thread::id, + std::thread::native_handle_type ){ + num_started++; + }, + [&num_post](){ + num_post++; + }); + + std::thread t = thrUtil->createThread( "FooName", [](){} ); + + t.join(); + + LOGUNIT_ASSERT_EQUAL( num_pre, 1 ); + LOGUNIT_ASSERT_EQUAL( num_started, 1 ); + LOGUNIT_ASSERT_EQUAL( num_post, 1 ); + } + +}; + + +LOGUNIT_TEST_SUITE_REGISTRATION(ThreadUtilityTest); + From a41756b4847c80c9e159bea7402d2af857102d9b Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Sat, 21 Aug 2021 11:40:27 -0400 Subject: [PATCH 11/17] Moved to helpers namespace and made test compile on windows --- src/main/cpp/threadutility.cpp | 14 +++++++------- src/main/include/log4cxx/helpers/threadutility.h | 16 +++++++++------- src/test/cpp/helpers/threadutilitytestcase.cpp | 4 ++-- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index 165803bc6..229d2d0a7 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -9,7 +9,7 @@ #include #endif -using log4cxx::ThreadUtility; +using log4cxx::helpers::ThreadUtility; struct ThreadUtility::priv_data{ priv_data(){ @@ -18,9 +18,9 @@ struct ThreadUtility::priv_data{ post_start = nullptr; } - log4cxx::pre_thread_start pre_start; - log4cxx::thread_started thread_start; - log4cxx::post_thread_start post_start; + log4cxx::helpers::pre_thread_start pre_start; + log4cxx::helpers::thread_started thread_start; + log4cxx::helpers::post_thread_start post_start; }; ThreadUtility::ThreadUtility() : @@ -78,14 +78,14 @@ void ThreadUtility::postThreadUnblockSignals(){ } -log4cxx::pre_thread_start ThreadUtility::preStartFunction(){ +log4cxx::helpers::pre_thread_start ThreadUtility::preStartFunction(){ return m_priv->pre_start; } -log4cxx::thread_started ThreadUtility::threadStartedFunction(){ +log4cxx::helpers::thread_started ThreadUtility::threadStartedFunction(){ return m_priv->thread_start; } -log4cxx::post_thread_start ThreadUtility::postStartFunction(){ +log4cxx::helpers::post_thread_start ThreadUtility::postStartFunction(){ return m_priv->post_start; } diff --git a/src/main/include/log4cxx/helpers/threadutility.h b/src/main/include/log4cxx/helpers/threadutility.h index 246e284a7..94af5bd1b 100644 --- a/src/main/include/log4cxx/helpers/threadutility.h +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -8,6 +8,7 @@ #include "log4cxx/logstring.h" namespace log4cxx { +namespace helpers { /** * A function that will be called before a thread is started. This can @@ -42,9 +43,9 @@ class LOG4CXX_EXPORT ThreadUtility { private: ThreadUtility(); - log4cxx::pre_thread_start preStartFunction(); - log4cxx::thread_started threadStartedFunction(); - log4cxx::post_thread_start postStartFunction(); + log4cxx::helpers::pre_thread_start preStartFunction(); + log4cxx::helpers::thread_started threadStartedFunction(); + log4cxx::helpers::post_thread_start postStartFunction(); struct priv_data; std::unique_ptr m_priv; @@ -92,9 +93,9 @@ class LOG4CXX_EXPORT ThreadUtility { std::thread createThread(LogString name, Function&& f, Args&&... args){ - log4cxx::pre_thread_start pre_start = preStartFunction(); - log4cxx::thread_started thread_start = threadStartedFunction(); - log4cxx::post_thread_start post_start = postStartFunction(); + log4cxx::helpers::pre_thread_start pre_start = preStartFunction(); + log4cxx::helpers::thread_started thread_start = threadStartedFunction(); + log4cxx::helpers::post_thread_start post_start = postStartFunction(); if( pre_start ){ pre_start(); @@ -112,6 +113,7 @@ class LOG4CXX_EXPORT ThreadUtility { } }; -} +} /* namespace helpers */ +} /* namespace log4cxx */ #endif diff --git a/src/test/cpp/helpers/threadutilitytestcase.cpp b/src/test/cpp/helpers/threadutilitytestcase.cpp index 1aacb62c1..7c7ec3655 100644 --- a/src/test/cpp/helpers/threadutilitytestcase.cpp +++ b/src/test/cpp/helpers/threadutilitytestcase.cpp @@ -35,7 +35,7 @@ LOGUNIT_CLASS(ThreadUtilityTest) thrUtil->configureThreadFunctions( nullptr, nullptr, nullptr ); - std::thread t = thrUtil->createThread( "FooName", [](){} ); + std::thread t = thrUtil->createThread( LOG4CXX_STR("FooName"), [](){} ); t.join(); } @@ -58,7 +58,7 @@ LOGUNIT_CLASS(ThreadUtilityTest) num_post++; }); - std::thread t = thrUtil->createThread( "FooName", [](){} ); + std::thread t = thrUtil->createThread( LOG4CXX_STR("FooName"), [](){} ); t.join(); From f728135fe4a8141241a145a138513b35f460f1d5 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Thu, 26 Aug 2021 20:22:52 -0400 Subject: [PATCH 12/17] Made some thread names more descriptive --- src/main/cpp/asyncappender.cpp | 2 +- src/main/cpp/filewatchdog.cpp | 2 +- src/main/cpp/telnetappender.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/cpp/asyncappender.cpp b/src/main/cpp/asyncappender.cpp index 7ab05b788..d357ff0ff 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -51,7 +51,7 @@ AsyncAppender::AsyncAppender() locationInfo(false), blocking(true) { - dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AyncAppend"), &AsyncAppender::dispatch, this ); + dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AsyncAppender"), &AsyncAppender::dispatch, this ); } AsyncAppender::~AsyncAppender() diff --git a/src/main/cpp/filewatchdog.cpp b/src/main/cpp/filewatchdog.cpp index 3848ed391..f9d0d7f03 100644 --- a/src/main/cpp/filewatchdog.cpp +++ b/src/main/cpp/filewatchdog.cpp @@ -93,7 +93,7 @@ void FileWatchdog::start() { checkAndConfigure(); - thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("Filewatchdog"), &FileWatchdog::run, this ); + thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("FileWatchdog"), &FileWatchdog::run, this ); } bool FileWatchdog::is_interrupted() diff --git a/src/main/cpp/telnetappender.cpp b/src/main/cpp/telnetappender.cpp index 7e1d6f5d7..90872423f 100644 --- a/src/main/cpp/telnetappender.cpp +++ b/src/main/cpp/telnetappender.cpp @@ -62,7 +62,7 @@ void TelnetAppender::activateOptions(Pool& /* p */) serverSocket->setSoTimeout(1000); } - sh = ThreadUtility::instance()->createThread( LOG4CXX_STR("TelnetAppend"), &TelnetAppender::acceptConnections, this ); + sh = ThreadUtility::instance()->createThread( LOG4CXX_STR("TelnetAppender"), &TelnetAppender::acceptConnections, this ); } void TelnetAppender::setOption(const LogString& option, From 7d67642f1de15761deb48f323e2ae7f63eb61b2f Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Thu, 26 Aug 2021 20:44:23 -0400 Subject: [PATCH 13/17] Fixed issues seen in review --- src/main/cpp/threadutility.cpp | 36 +++++++++---------- .../include/log4cxx/helpers/threadutility.h | 36 +++++++++---------- .../cpp/helpers/threadutilitytestcase.cpp | 26 +++++++------- 3 files changed, 50 insertions(+), 48 deletions(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index 229d2d0a7..63127b7a8 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -13,14 +13,14 @@ using log4cxx::helpers::ThreadUtility; struct ThreadUtility::priv_data{ priv_data(){ - pre_start = nullptr; - thread_start = nullptr; - post_start = nullptr; + start_pre = nullptr; + started = nullptr; + start_post = nullptr; } - log4cxx::helpers::pre_thread_start pre_start; - log4cxx::helpers::thread_started thread_start; - log4cxx::helpers::post_thread_start post_start; + log4cxx::helpers::ThreadStartPre start_pre; + log4cxx::helpers::ThreadStarted started; + log4cxx::helpers::ThreadStartPost start_post; }; ThreadUtility::ThreadUtility() : @@ -34,12 +34,12 @@ std::shared_ptr ThreadUtility::instance(){ return instance; } -void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1, - thread_started started, - post_thread_start post_start1 ){ - m_priv->pre_start = pre_start1; - m_priv->thread_start = started; - m_priv->post_start = post_start1; +void ThreadUtility::configureFuncs( ThreadStartPre pre_start, + ThreadStarted started, + ThreadStartPost post_start ){ + m_priv->start_pre = pre_start; + m_priv->started = started; + m_priv->start_post = post_start; } void ThreadUtility::preThreadBlockSignals(){ @@ -78,14 +78,14 @@ void ThreadUtility::postThreadUnblockSignals(){ } -log4cxx::helpers::pre_thread_start ThreadUtility::preStartFunction(){ - return m_priv->pre_start; +log4cxx::helpers::ThreadStartPre ThreadUtility::preStartFunction(){ + return m_priv->start_pre; } -log4cxx::helpers::thread_started ThreadUtility::threadStartedFunction(){ - return m_priv->thread_start; +log4cxx::helpers::ThreadStarted ThreadUtility::threadStartedFunction(){ + return m_priv->started; } -log4cxx::helpers::post_thread_start ThreadUtility::postStartFunction(){ - return m_priv->post_start; +log4cxx::helpers::ThreadStartPost ThreadUtility::postStartFunction(){ + return m_priv->start_post; } diff --git a/src/main/include/log4cxx/helpers/threadutility.h b/src/main/include/log4cxx/helpers/threadutility.h index 94af5bd1b..a5a43c99a 100644 --- a/src/main/include/log4cxx/helpers/threadutility.h +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -15,26 +15,26 @@ namespace helpers { * be used to (for example) block all of the signals in the thread, so * that when the thread is created it will have a correct signal mask. */ -typedef std::function pre_thread_start; +typedef std::function ThreadStartPre; /** * Called when a new thread has started. This can be used to set * parameters for the thread in a platform-specific manner. * * @param threadName The name of the thread - * @param thread_id The ID of the thread as reported by std::thread::get_id - * @param native_handle The native handle of the thread, as reported by + * @param threadId The ID of the thread as reported by std::thread::get_id + * @param nativeHandle The native handle of the thread, as reported by * std::thread::native_handle */ typedef std::function thread_started; + std::thread::id threadId, + std::thread::native_handle_type nativeHandle )> ThreadStarted; /** * Called after a thread has started. This can be used to (for example) * unblock the signals in the thread. */ -typedef std::function post_thread_start; +typedef std::function ThreadStartPost; class ThreadUtility; LOG4CXX_PTR_DEF(ThreadUtility); @@ -43,9 +43,9 @@ class LOG4CXX_EXPORT ThreadUtility { private: ThreadUtility(); - log4cxx::helpers::pre_thread_start preStartFunction(); - log4cxx::helpers::thread_started threadStartedFunction(); - log4cxx::helpers::post_thread_start postStartFunction(); + log4cxx::helpers::ThreadStartPre preStartFunction(); + log4cxx::helpers::ThreadStarted threadStartedFunction(); + log4cxx::helpers::ThreadStartPost postStartFunction(); struct priv_data; std::unique_ptr m_priv; @@ -58,16 +58,16 @@ class LOG4CXX_EXPORT ThreadUtility { /** * Configure the thread functions that log4cxx will use. * Note that setting any of these parameters to nullptr is valid, - * and simply results in the callback not being called. * + * and simply results in the callback not being called. */ - void configureThreadFunctions( pre_thread_start pre_start, - thread_started started, - post_thread_start post_start ); + void configureFuncs( ThreadStartPre pre_start, + ThreadStarted started, + ThreadStartPost post_start ); /** * A pre-start thread function that blocks signals to the new thread * (if the system has pthreads). If the system does not have pthreads, - * is equivalent to preThreadDoNothing(); + * does nothing. */ void preThreadBlockSignals(); @@ -82,7 +82,7 @@ class LOG4CXX_EXPORT ThreadUtility { /** * A post-start thread function that unblocks signals that preThreadBlockSignals * blocked before starting the thread. If the system does not have pthreads, - * is equivalent to postThreadDoNothing(); + * does nothing. */ void postThreadUnblockSignals(); @@ -93,9 +93,9 @@ class LOG4CXX_EXPORT ThreadUtility { std::thread createThread(LogString name, Function&& f, Args&&... args){ - log4cxx::helpers::pre_thread_start pre_start = preStartFunction(); - log4cxx::helpers::thread_started thread_start = threadStartedFunction(); - log4cxx::helpers::post_thread_start post_start = postStartFunction(); + log4cxx::helpers::ThreadStartPre pre_start = preStartFunction(); + log4cxx::helpers::ThreadStarted thread_start = threadStartedFunction(); + log4cxx::helpers::ThreadStartPost post_start = postStartFunction(); if( pre_start ){ pre_start(); diff --git a/src/test/cpp/helpers/threadutilitytestcase.cpp b/src/test/cpp/helpers/threadutilitytestcase.cpp index 7c7ec3655..33abf9cd0 100644 --- a/src/test/cpp/helpers/threadutilitytestcase.cpp +++ b/src/test/cpp/helpers/threadutilitytestcase.cpp @@ -33,7 +33,7 @@ LOGUNIT_CLASS(ThreadUtilityTest) void testNullFunctions(){ ThreadUtilityPtr thrUtil = ThreadUtility::instance(); - thrUtil->configureThreadFunctions( nullptr, nullptr, nullptr ); + thrUtil->configureFuncs( nullptr, nullptr, nullptr ); std::thread t = thrUtil->createThread( LOG4CXX_STR("FooName"), [](){} ); @@ -46,17 +46,19 @@ LOGUNIT_CLASS(ThreadUtilityTest) int num_started = 0; int num_post = 0; - thrUtil->configureThreadFunctions( [&num_pre](){ - num_pre++; - }, - [&num_started]( LogString, - std::thread::id, - std::thread::native_handle_type ){ - num_started++; - }, - [&num_post](){ - num_post++; - }); + thrUtil->configureFuncs( + [&num_pre](){ + num_pre++; + }, + [&num_started]( LogString, + std::thread::id, + std::thread::native_handle_type ){ + num_started++; + }, + [&num_post](){ + num_post++; + } + ); std::thread t = thrUtil->createThread( LOG4CXX_STR("FooName"), [](){} ); From 1006b6f086435535c5f6df387f7b691d359b3ce4 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Thu, 26 Aug 2021 20:58:01 -0400 Subject: [PATCH 14/17] Properly set and restore the thread mask --- src/main/cpp/threadutility.cpp | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index 63127b7a8..22006543b 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -3,6 +3,7 @@ #include "log4cxx/helpers/loglog.h" #include +#include #if LOG4CXX_HAS_SETTHREADDESCRIPTION #include @@ -21,6 +22,11 @@ struct ThreadUtility::priv_data{ log4cxx::helpers::ThreadStartPre start_pre; log4cxx::helpers::ThreadStarted started; log4cxx::helpers::ThreadStartPost start_post; +#if LOG4CXX_HAS_PTHREAD_SIGMASK + std::mutex creation_mutex; + sigset_t old_mask; + bool sigmask_valid; +#endif }; ThreadUtility::ThreadUtility() : @@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start, void ThreadUtility::preThreadBlockSignals(){ #if LOG4CXX_HAS_PTHREAD_SIGMASK + m_priv->creation_mutex.lock(); sigset_t set; sigfillset(&set); - if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){ + if( pthread_sigmask(SIG_SETMASK, &set, &m_priv->old_mask) < 0 ){ LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); + m_priv->sigmask_valid = false; + }else{ + m_priv->sigmask_valid = true; } #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } void ThreadUtility::threadStartedNameThread(LogString threadName, - std::thread::id /*thread_id*/, - std::thread::native_handle_type native_handle){ + std::thread::id /*threadId*/, + std::thread::native_handle_type nativeHandle){ #if LOG4CXX_HAS_PTHREAD_SETNAME - if( pthread_setname_np( static_cast( native_handle ), threadName.c_str() ) < 0 ){ + if( pthread_setname_np( static_cast( nativeHandle ), threadName.c_str() ) < 0 ){ LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") ); } #elif LOG4CXX_HAS_SETTHREADDESCRIPTION @@ -69,11 +79,13 @@ void ThreadUtility::threadStartedNameThread(LogString threadName, void ThreadUtility::postThreadUnblockSignals(){ #if LOG4CXX_HAS_PTHREAD_SIGMASK - sigset_t set; - sigemptyset(&set); - if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){ - LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); + // Only restore the signal mask if we were able to set it in the first place. + if( m_priv->sigmask_valid ){ + if( pthread_sigmask(SIG_SETMASK, &m_priv->old_mask, nullptr) < 0 ){ + LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); + } } + m_priv->creation_mutex.unlock(); #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } From c37d12700ae446d72ff2dd102676ca29c5c60bea Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Thu, 26 Aug 2021 21:56:52 -0400 Subject: [PATCH 15/17] Updated test case to try default methods. Added utility method to configure the thread utility --- src/main/cpp/threadutility.cpp | 26 +++++++++ .../include/log4cxx/helpers/threadutility.h | 13 +++++ src/site/markdown/threading.md | 55 +++++++++++-------- .../cpp/helpers/threadutilitytestcase.cpp | 11 ++++ 4 files changed, 83 insertions(+), 22 deletions(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index 22006543b..c2821f7f9 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -40,6 +40,32 @@ std::shared_ptr ThreadUtility::instance(){ return instance; } +void ThreadUtility::configure( ThreadConfigurationType type ){ + std::shared_ptr utility = instance(); + + if( type == ThreadConfigurationType::NoConfiguration ){ + utility->configureFuncs( nullptr, nullptr, nullptr ); + }else if( type == ThreadConfigurationType::NameThreadOnly ){ + utility->configureFuncs( nullptr, + std::bind( &ThreadUtility::threadStartedNameThread, utility, + std::placeholders::_1, + std::placeholders::_2, + std::placeholders::_3 ), + nullptr ); + }else if( type == ThreadConfigurationType::BlockSignalsOnly ){ + utility->configureFuncs( std::bind( &ThreadUtility::preThreadBlockSignals, utility ), + nullptr, + std::bind( &ThreadUtility::postThreadUnblockSignals, utility ) ); + }else if( type == ThreadConfigurationType::BlockSignalsAndNameThread ){ + utility->configureFuncs( std::bind( &ThreadUtility::preThreadBlockSignals, utility ), + std::bind( &ThreadUtility::threadStartedNameThread, utility, + std::placeholders::_1, + std::placeholders::_2, + std::placeholders::_3 ), + std::bind( &ThreadUtility::postThreadUnblockSignals, utility ) ); + } +} + void ThreadUtility::configureFuncs( ThreadStartPre pre_start, ThreadStarted started, ThreadStartPost post_start ){ diff --git a/src/main/include/log4cxx/helpers/threadutility.h b/src/main/include/log4cxx/helpers/threadutility.h index a5a43c99a..84dd25fc6 100644 --- a/src/main/include/log4cxx/helpers/threadutility.h +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -36,6 +36,13 @@ typedef std::function ThreadStartPost; +enum class ThreadConfigurationType { + NoConfiguration, + BlockSignalsOnly, + NameThreadOnly, + BlockSignalsAndNameThread, +}; + class ThreadUtility; LOG4CXX_PTR_DEF(ThreadUtility); @@ -55,6 +62,12 @@ class LOG4CXX_EXPORT ThreadUtility { static std::shared_ptr instance(); + /** + * Utility method for configuring the ThreadUtility in a standard + * configuration. + */ + static void configure( ThreadConfigurationType type ); + /** * Configure the thread functions that log4cxx will use. * Note that setting any of these parameters to nullptr is valid, diff --git a/src/site/markdown/threading.md b/src/site/markdown/threading.md index daed9e19a..41faaa5d9 100644 --- a/src/site/markdown/threading.md +++ b/src/site/markdown/threading.md @@ -38,18 +38,28 @@ all threads be terminated properly before returning from `main()`. See [LOGCXX-322][3] for more information. -## Threads Created by Log4cxx +## Signal Handling with Log4cxx Under certain configurations, Log4cxx may create new threads in order to do -tasks(e.g. network comms, other async operations). On Linux systems, this -can lead to undesirable signal delivery, as signals can be delivered to -any thread in the process. - -To handle signals properly on Linux, you may wish to utilize the [signalfd][1] -API to handle signals correctly. Another way of handling signals is to -create a pipe internal to your application to notify the main loop that there -is a signal available - see the [Qt documentation][2] for more information. - +tasks(e.g. network comms, other async operations). On Linux/POSIX systems, +this can lead to undesirable signal delivery, as signals can be delivered to +any thread in the process. This can be most clearly seen if your application +uses the [sigwait(3)][4] system call, as the thread that calls sigwait may +not be the thread that actually gets the signal. + +There are three main ways to handle signals coming to your process. All +of these ways of handling signals are supported by Log4cxx in order to +provide flexibility to users of the library. These three ways are: + +1. Write to a file descriptor in a signal handler, notifying your main event +loop of a signal. If you use Qt, [their documentation][2] has information on +this method of handling a signal. +2. (Linux-only) Use [signalfd(2)][1] to create a file descriptor that notifies +you of a signal. This is a special case of (1). +3. Block signals in newly created threads, ensuring that signals can only be +sent to threads of your choosing. + +If you need to use option #3(for example, because you are using sigwait), Log4cxx provides a mechanism for defining methods to be called at two main points in the lifecycle of a thread: @@ -64,20 +74,21 @@ Once a new thread is created, there is also a callback function that lets client code do operations on the thread directly. A sample method in Log4cxx has a callback to name the thread in order to make debugging nicer. -In order to use these callback functions, use the ThreadUtility class. You -can use some sample functions(not no-ops) as follows: - -``` -ThreadUtility::instance()->configureThreadFunctions( ThreadUtility::preThreadBlockSignals, - ThreadUtility::threadStartedNameThread, - ThreadUtility::postThreadUnblockSignals ); -``` - -These sample functions will block all POSIX signals before starting a new thread, -and then unblock them once the thread has been created. You may provide your -own functions to handle this if you so choose. +In order to use these callback functions, use the [ThreadUtility](@ref log4cxx.helpers.ThreadUtility) +class. You can configure the ThreadUtility class in several different ways by using the +[ThreadUtility::configure](@ref log4cxx.helpers.ThreadUtility.configure) +method with several pre-defined configurations. +In the event that you need special signal handling, you can implement your own +functions, and use the ThreadUtility::configureFuncs method in order to +customize exactly what happens. +**NOTE:** It is very important that if you use the `ThreadUtility::preThreadBlockSignals` +method, it must be paired with the equivalent `ThreadUtility::postThreadUnblockSignals` +call, as there is an internal mutex that is locked and unlocked in order to ensure that +only one thread can be started at a time. Failure to do this may lead to deadlock. +The ThreadUtility::configure method handles this automatically. [1]: https://man7.org/linux/man-pages/man2/signalfd.2.html [2]: https://doc.qt.io/qt-5/unix-signals.html [3]: https://issues.apache.org/jira/browse/LOGCXX-322 +[4]: https://man7.org/linux/man-pages/man3/sigwait.3.html diff --git a/src/test/cpp/helpers/threadutilitytestcase.cpp b/src/test/cpp/helpers/threadutilitytestcase.cpp index 33abf9cd0..05b7cfd08 100644 --- a/src/test/cpp/helpers/threadutilitytestcase.cpp +++ b/src/test/cpp/helpers/threadutilitytestcase.cpp @@ -27,6 +27,7 @@ LOGUNIT_CLASS(ThreadUtilityTest) LOGUNIT_TEST_SUITE(ThreadUtilityTest); LOGUNIT_TEST(testNullFunctions); LOGUNIT_TEST(testCustomFunctions); + LOGUNIT_TEST(testDefaultFunctions); LOGUNIT_TEST_SUITE_END(); public: @@ -69,6 +70,16 @@ LOGUNIT_CLASS(ThreadUtilityTest) LOGUNIT_ASSERT_EQUAL( num_post, 1 ); } + void testDefaultFunctions(){ + ThreadUtility::configure( ThreadConfigurationType::BlockSignalsAndNameThread ); + + ThreadUtilityPtr thrUtil = ThreadUtility::instance(); + + std::thread t = thrUtil->createThread( LOG4CXX_STR("FooName"), [](){} ); + + t.join(); + } + }; From 890c72f5e8ddbfe7902b44b47a94759f111ecfda Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Sun, 29 Aug 2021 20:24:49 -0400 Subject: [PATCH 16/17] use thread_local on *nix --- src/main/cpp/threadutility.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index c2821f7f9..60010a088 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -22,12 +22,12 @@ struct ThreadUtility::priv_data{ log4cxx::helpers::ThreadStartPre start_pre; log4cxx::helpers::ThreadStarted started; log4cxx::helpers::ThreadStartPost start_post; +}; + #if LOG4CXX_HAS_PTHREAD_SIGMASK - std::mutex creation_mutex; - sigset_t old_mask; - bool sigmask_valid; +static thread_local sigset_t old_mask; +static thread_local bool sigmask_valid; #endif -}; ThreadUtility::ThreadUtility() : m_priv( new priv_data() ) @@ -76,14 +76,13 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start, void ThreadUtility::preThreadBlockSignals(){ #if LOG4CXX_HAS_PTHREAD_SIGMASK - m_priv->creation_mutex.lock(); sigset_t set; sigfillset(&set); - if( pthread_sigmask(SIG_SETMASK, &set, &m_priv->old_mask) < 0 ){ + if( pthread_sigmask(SIG_SETMASK, &set, &old_mask) < 0 ){ LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); - m_priv->sigmask_valid = false; + sigmask_valid = false; }else{ - m_priv->sigmask_valid = true; + sigmask_valid = true; } #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } @@ -106,12 +105,11 @@ void ThreadUtility::threadStartedNameThread(LogString threadName, void ThreadUtility::postThreadUnblockSignals(){ #if LOG4CXX_HAS_PTHREAD_SIGMASK // Only restore the signal mask if we were able to set it in the first place. - if( m_priv->sigmask_valid ){ - if( pthread_sigmask(SIG_SETMASK, &m_priv->old_mask, nullptr) < 0 ){ + if( sigmask_valid ){ + if( pthread_sigmask(SIG_SETMASK, &old_mask, nullptr) < 0 ){ LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") ); } } - m_priv->creation_mutex.unlock(); #endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */ } From 2b232f64b6dd79261cb8777dd0a2c92236e1db03 Mon Sep 17 00:00:00 2001 From: Robert Middleton Date: Sun, 29 Aug 2021 20:48:37 -0400 Subject: [PATCH 17/17] typo when changing code style --- src/main/cpp/threadutility.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/cpp/threadutility.cpp b/src/main/cpp/threadutility.cpp index 60010a088..a6c7ca2d5 100644 --- a/src/main/cpp/threadutility.cpp +++ b/src/main/cpp/threadutility.cpp @@ -95,7 +95,7 @@ void ThreadUtility::threadStartedNameThread(LogString threadName, LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") ); } #elif LOG4CXX_HAS_SETTHREADDESCRIPTION - HRESULT hr = SetThreadDescription(static_cast(native_handle), threadName.c_str()); + HRESULT hr = SetThreadDescription(static_cast(nativeHandle), threadName.c_str()); if(FAILED(hr)){ LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") ); }