You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/08/24 09:04:27 UTC
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
ams-tschoening commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r694560188
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -0,0 +1,91 @@
+#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::helpers::ThreadUtility;
+
+struct ThreadUtility::priv_data{
+ priv_data(){
+ pre_start = nullptr;
+ thread_start = nullptr;
+ post_start = nullptr;
+ }
+
+ log4cxx::helpers::pre_thread_start pre_start;
+ log4cxx::helpers::thread_started thread_start;
+ log4cxx::helpers::post_thread_start post_start;
+};
Review comment:
Opinion: I prefer same prefixes for easier readability/alignment, so `start*`, `end*`, `during*` etc. That would make the `helpers`-lines better readable in my opinion as well, as all of those would become `thread`-related in the first place, then `callbacks` etc.
Besides that, `thread_started` vs. `thread_start` looks inconsistent and like a spelling error instead of as intended. You seem to use `started` more often.
```c++
start[ed]_pre = nullptr;
started = nullptr;
start[ed]_post = nullptr;
```
```c++
log4cxx::helpers::thread_start[ed]_pre start[ed]_pre;
log4cxx::helpers::thread_started started;
log4cxx::helpers::thread_start[ed]_post start[ed]_post;
```
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -0,0 +1,91 @@
+#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::helpers::ThreadUtility;
+
+struct ThreadUtility::priv_data{
+ priv_data(){
+ pre_start = nullptr;
+ thread_start = nullptr;
+ post_start = nullptr;
+ }
+
+ log4cxx::helpers::pre_thread_start pre_start;
+ log4cxx::helpers::thread_started thread_start;
+ log4cxx::helpers::post_thread_start post_start;
+};
+
+ThreadUtility::ThreadUtility() :
+ m_priv( new priv_data() )
+{}
+
+ThreadUtility::~ThreadUtility(){}
+
+std::shared_ptr<ThreadUtility> ThreadUtility::instance(){
+ static std::shared_ptr<ThreadUtility> instance( new ThreadUtility() );
+ return instance;
+}
+
+void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1,
Review comment:
Opinion: The phrase `thread` in your function names reads redundant and unnecessary noisy to me, as that phrase is already part of the class name.
Opinion: `function[s]` used at multiple places reads unnecessary long to me. A simple `func[s]` would be sufficient in my opinion.
##########
File path: src/main/cpp/filewatchdog.cpp
##########
@@ -92,7 +93,7 @@ void FileWatchdog::start()
{
checkAndConfigure();
- thread = std::thread( &FileWatchdog::run, this );
+ thread = ThreadUtility::instance()->createThread( LOG4CXX_STR("Filewatchdog"), &FileWatchdog::run, this );
Review comment:
`Filewatchdog` vs. `FileWatchdog` vs. `AsyncAppender` vs. `SocketAppend` ... Lower case looks like a mistake.
##########
File path: src/site/markdown/threading.md
##########
@@ -0,0 +1,83 @@
+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 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
+
+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::instance()->configureThreadFunctions( ThreadUtility::preThreadBlockSignals,
Review comment:
Shouldn't that be called by default by log4cxx as well and apps would need to apply different defaults as necessary? That's how I understood the associated [JIRA](https://issues.apache.org/jira/browse/LOGCXX-431).
##########
File path: src/main/cpp/asyncappender.cpp
##########
@@ -50,7 +51,7 @@ AsyncAppender::AsyncAppender()
locationInfo(false),
blocking(true)
{
- dispatcher = std::thread( &AsyncAppender::dispatch, this );
+ dispatcher = ThreadUtility::instance()->createThread( LOG4CXX_STR("AyncAppend"), &AsyncAppender::dispatch, this );
Review comment:
`AyncAppend` vs. `AsyncAppend` vs. `AsyncAppender` vs. `SocketAppend` ... There's at least an `s` missing.
In general: Did you choose to not use the class name or should that simply be reused 1:1?
##########
File path: src/main/include/log4cxx/helpers/threadutility.h
##########
@@ -0,0 +1,119 @@
+#ifndef _LOG4CXX_THREADUTILITY_H
+#define _LOG4CXX_THREADUTILITY_H
+
+#include <thread>
+#include <functional>
+#include <memory>
+
+#include "log4cxx/logstring.h"
+
+namespace log4cxx {
+namespace helpers {
+
+/**
+ * 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 ThreadUtility;
+LOG4CXX_PTR_DEF(ThreadUtility);
+
+class LOG4CXX_EXPORT ThreadUtility {
+private:
+ ThreadUtility();
+
+ log4cxx::helpers::pre_thread_start preStartFunction();
+ log4cxx::helpers::thread_started threadStartedFunction();
+ log4cxx::helpers::post_thread_start postStartFunction();
+
+ struct priv_data;
+ std::unique_ptr<priv_data> m_priv;
+
+public:
+ ~ThreadUtility();
+
+ static std::shared_ptr<ThreadUtility> instance();
+
+ /**
+ * 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. *
+ */
+ void configureThreadFunctions( pre_thread_start pre_start,
+ thread_started started,
+ post_thread_start 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();
+ */
+ void preThreadBlockSignals();
+
+ /**
+ * A thread_started function that names the thread using the appropriate
+ * system call.
+ */
+ void threadStartedNameThread(LogString threadName,
+ std::thread::id thread_id,
+ std::thread::native_handle_type native_handle);
+
+ /**
+ * 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();
Review comment:
I can't find an implementation of `postThreadDoNothing`. Did I miss something or is the function missing or the comment outdated?
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -0,0 +1,91 @@
+#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::helpers::ThreadUtility;
+
+struct ThreadUtility::priv_data{
Review comment:
Opinion: `priv_data` sound so meaningless and exposing an implementation detail about visibility. How about something like `[thread_]func[tion]s`, `[thread_]hooks`, `[thread_]callbacks` or alike instead? If you prefer, with a `priv_`-prefix as well. But `data` reads pretty meaningless currently. You use the phrase `function[s]` anyway already at some places.
##########
File path: 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 <log4cxx/helpers/threadutility.h>
+
+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( LOG4CXX_STR("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++;
+ });
Review comment:
Opinion: There's a level of indentation missing in the args to `configureFunctions` which make them difficult to read.
```cpp
thrUtil->configureThreadFunctions(
[&num_pre](){
num_pre++;
},
[&num_started]( LogString, std::thread::id, std::thread::native_handle_type ){
num_started++;
},
[&num_post](){
num_post++;
}
);
```
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -0,0 +1,91 @@
+#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::helpers::ThreadUtility;
+
+struct ThreadUtility::priv_data{
+ priv_data(){
+ pre_start = nullptr;
+ thread_start = nullptr;
+ post_start = nullptr;
+ }
+
+ log4cxx::helpers::pre_thread_start pre_start;
+ log4cxx::helpers::thread_started thread_start;
+ log4cxx::helpers::post_thread_start post_start;
+};
+
+ThreadUtility::ThreadUtility() :
+ m_priv( new priv_data() )
+{}
+
+ThreadUtility::~ThreadUtility(){}
+
+std::shared_ptr<ThreadUtility> ThreadUtility::instance(){
+ static std::shared_ptr<ThreadUtility> instance( new ThreadUtility() );
+ 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::preThreadBlockSignals(){
+#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ sigset_t set;
+ sigfillset(&set);
+ if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){
+ LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );
+ }
+#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */
+}
+
+void ThreadUtility::threadStartedNameThread(LogString threadName,
+ std::thread::id /*thread_id*/,
+ std::thread::native_handle_type native_handle){
Review comment:
Opinion: `threadName` vs. `thread_name` vs. `native_handle` vs. `nativeHandle`
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -0,0 +1,91 @@
+#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::helpers::ThreadUtility;
+
+struct ThreadUtility::priv_data{
+ priv_data(){
+ pre_start = nullptr;
+ thread_start = nullptr;
+ post_start = nullptr;
+ }
+
+ log4cxx::helpers::pre_thread_start pre_start;
+ log4cxx::helpers::thread_started thread_start;
+ log4cxx::helpers::post_thread_start post_start;
+};
+
+ThreadUtility::ThreadUtility() :
+ m_priv( new priv_data() )
+{}
+
+ThreadUtility::~ThreadUtility(){}
+
+std::shared_ptr<ThreadUtility> ThreadUtility::instance(){
+ static std::shared_ptr<ThreadUtility> instance( new ThreadUtility() );
+ return instance;
+}
+
+void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1,
+ thread_started started,
+ post_thread_start post_start1 ){
Review comment:
What's the reason for `1`? Makes me expect at least a `2`. :-) If to avoid name clashes, how about prefixing with `_` instead? I have the feeling that's a widely adopted convention, though, more likely used for the instance variables instead of the arguments. But is a different name necessary at all?
##########
File path: src/main/include/log4cxx/helpers/threadutility.h
##########
@@ -0,0 +1,119 @@
+#ifndef _LOG4CXX_THREADUTILITY_H
+#define _LOG4CXX_THREADUTILITY_H
+
+#include <thread>
+#include <functional>
+#include <memory>
+
+#include "log4cxx/logstring.h"
+
+namespace log4cxx {
+namespace helpers {
+
+/**
+ * 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 ThreadUtility;
+LOG4CXX_PTR_DEF(ThreadUtility);
+
+class LOG4CXX_EXPORT ThreadUtility {
+private:
+ ThreadUtility();
+
+ log4cxx::helpers::pre_thread_start preStartFunction();
+ log4cxx::helpers::thread_started threadStartedFunction();
+ log4cxx::helpers::post_thread_start postStartFunction();
+
+ struct priv_data;
+ std::unique_ptr<priv_data> m_priv;
+
+public:
+ ~ThreadUtility();
+
+ static std::shared_ptr<ThreadUtility> instance();
+
+ /**
+ * 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. *
+ */
+ void configureThreadFunctions( pre_thread_start pre_start,
+ thread_started started,
+ post_thread_start 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();
+ */
+ void preThreadBlockSignals();
+
+ /**
+ * A thread_started function that names the thread using the appropriate
+ * system call.
+ */
+ void threadStartedNameThread(LogString threadName,
+ std::thread::id thread_id,
+ std::thread::native_handle_type native_handle);
+
+ /**
+ * 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();
+ */
+ void postThreadUnblockSignals();
+
+ /**
+ * Start a thread
+ */
+ template<class Function, class... Args>
+ 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();
+
+ if( pre_start ){
+ pre_start();
+ }
+ std::thread t( f, args... );
+ if( thread_start ){
+ thread_start( name,
+ t.get_id(),
+ t.native_handle() );
+ }
+ if( post_start ){
Review comment:
Just for my understanding: `thread_start` is simply designed to reconfigure/interact with the thread, while `post_start` is designed to reconfigure the "thread creating" aka "current" environment? Because from a lifecycle point of view both functions seems like they could be merged into one. But you want to logically separate the different purposes?
So, `pre` and `post` are somewhat [POSIX-specific](https://man7.org/linux/man-pages/man3/pthread_sigmask.3.html) in the end, because some aspects are inherited by new threads?
```c
/* Block SIGQUIT and SIGUSR1; other threads created by main()
will inherit a copy of the signal mask. */
```
Or is that how C++ is documented to work on its own already? I wonder if we would need to consider if some callbacks would need to be or at least make sense to be executed in different threads, e.g. the current vs. the created one or alike. OTOH, that could be implemented by each thread function on its own anyway, correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org