This is an automated email from the ASF dual-hosted git repository. rmiddleton pushed a commit to branch LOGCXX-431 in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 091a9ce1e9961fa8d81473e1a8b72167a9a3f912 Author: Robert Middleton <[email protected]> AuthorDate: Thu Aug 19 21:08:10 2021 -0400 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 ++- src/main/include/log4cxx/helpers/threadutility.h | 114 +++++++++++++++++++++ .../include/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(-) diff --git a/src/main/cpp/CMakeLists.txt b/src/main/cpp/CMakeLists.txt index ff5375d..0f8f40d 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 6da23b1..2a4c27f 100644 --- a/src/main/cpp/asyncappender.cpp +++ b/src/main/cpp/asyncappender.cpp @@ -30,6 +30,7 @@ #include <log4cxx/helpers/stringhelper.h> #include <apr_atomic.h> #include <log4cxx/helpers/optionconverter.h> +#include <log4cxx/helpers/threadutility.h> 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 e3dceb5..fb49a09 100644 --- a/src/main/cpp/filewatchdog.cpp +++ b/src/main/cpp/filewatchdog.cpp @@ -23,6 +23,7 @@ #include <apr_atomic.h> #include <log4cxx/helpers/transcoder.h> #include <log4cxx/helpers/exception.h> +#include <log4cxx/helpers/threadutility.h> #include <functional> 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 122de9d..66bf920 100644 --- a/src/main/cpp/socketappenderskeleton.cpp +++ b/src/main/cpp/socketappenderskeleton.cpp @@ -26,6 +26,7 @@ #include <log4cxx/spi/loggingevent.h> #include <log4cxx/helpers/transcoder.h> #include <log4cxx/helpers/bytearrayoutputstream.h> +#include <log4cxx/helpers/threadutility.h> #include <functional> 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 a88eb42..dc6eb7c 100644 --- a/src/main/cpp/sockethubappender.cpp +++ b/src/main/cpp/sockethubappender.cpp @@ -30,6 +30,7 @@ #include <log4cxx/helpers/objectoutputstream.h> #include <log4cxx/helpers/socketoutputstream.h> #include <log4cxx/helpers/exception.h> +#include <log4cxx/helpers/threadutility.h> #include <mutex> 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 418aac0..222b982 100644 --- a/src/main/cpp/telnetappender.cpp +++ b/src/main/cpp/telnetappender.cpp @@ -24,6 +24,7 @@ #include <apr_strings.h> #include <log4cxx/helpers/charsetencoder.h> #include <log4cxx/helpers/bytebuffer.h> +#include <log4cxx/helpers/threadutility.h> #include <mutex> 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 0000000..bdd301b --- /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 <signal.h> + +#if LOG4CXX_HAS_SETTHREADDESCRIPTION +#include <windows.h> +#include <processthreadsapi.h> +#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<pthread_t>( native_handle ), threadName.c_str() ) < 0 ){ + LOGLOG_ERROR( "unable to set thread name" ); + } +#elif LOG4CXX_HAS_SETTHREADDESCRIPTION + HRESULT hr = SetThreadDescription(static_cast<HANDLE>(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 584941c..575f68b 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 0000000..de4ffad --- /dev/null +++ b/src/main/include/log4cxx/helpers/threadutility.h @@ -0,0 +1,114 @@ +#ifndef _LOG4CXX_THREADUTILITY_H +#define _LOG4CXX_THREADUTILITY_H + +#include <thread> +#include <functional> + +#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<void()> 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<void( LogString threadName, + std::thread::id thread_id, + std::thread::native_handle_type native_handle )> thread_started; + +/** + * Called after a thread has started. This can be used to (for example) + * unblock the signals in the thread. + */ +typedef std::function<void()> 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<class Function, class... Args> + 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 280f21f..bf965c9 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 f8c6fe7..379c45b 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 0000000..045f319 --- /dev/null +++ b/src/site/markdown/threading.md @@ -0,0 +1,82 @@ +Threading {#threading} +=== +<!-- + Note: License header cannot be first, as doxygen does not generate + cleanly if it before the '===' +--> +<!-- + 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. +--> +# 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
