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/21 15:58:21 UTC
[GitHub] [logging-log4cxx] rm5248 opened a new pull request #68: LOGCXX-431
rm5248 opened a new pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68
In order to provide a way to block signals to threads created by log4cxx, a new API has been created to set functions that will be called before a thread has started, information about the thread that has started, and after the thread has started. The idea is to give the end-user control over how they want pre/post functions to do. Sample functions (to fix LOGCXX-431) are provided, but can always be overridden.
Documentation on how Log4cxx deals with threads has also been added.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697927015
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
Well, pthreads is POSIX, so it should be quite portable.
(Obviously - the name bit not, but I seldom saw anyone set the name of a thread)
Yes, I suggest that blocking the signals should be the default -- this will fix signal-bugs in existing apps without the need to add patches to them
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697068021
##########
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:
Looking at it again, I also realized that I had done cpp_style instead of JavaStyle for the function definitions as well. Since the rest of the library is all JavaStyle, I've made those updates as well.
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695277961
##########
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:
The reasoning behind this is that:
1. Masking the signals works, but you should probably use a better API(such as signalfd) or do it the Qt way, where you catch the signal and write to a file descriptor to notify your process that the signal has come in.
2. I don't want to break things in unexpected ways - if people want to mask signals, there should be effectively only one line of code to add. I don't think this would break anything, but I don't like to silently try to help people.
For issue 2 though, I am perfectly fine with adding a new configuration parameter(called something like `blockThreadSignals`) that would then do this automatically.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r699005019
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
:+1:
> There are a few things that I want to take a look at in terms of how/when that gets configured.
Yes, that would be interesting informatin. One thing that would explcitly interest me is "when are the threads started". (To assess when liblog4cxx spawns it threads and therefore fork() becoming unsafe without exec())
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r698859515
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
I'm going to merge this for now, and do a separate PR in a little bit that configures the blocking of signals to be set by default. There are a few things that I want to take a look at in terms of how/when that gets configured.
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r698859515
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
I'm going to merge this for now, and do a separate PR in a little bit that configures the blocking of signals to be set by default. There are a few things that I want to take a look at in terms of how/when that gets configured.
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697925078
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
> (OTOH .. .I'm still not sure if this configurabiity/complixty is really needed; Possibly it would be enough to block the signals unconditionally when log4cxx starts a thread? I currently cant think of an usecase where a user would not want that log4cxx just blocks the signals. (Please explain; I'm obviously missing why you think it necessary)..)
My general philosophy is that sane defaults should be acceptable >90% of the time, but there are some instances where you might want to customize this for whatever reason. Hence the single-line call to ThreadUtility::configure to set various default configurations, while still allowing the end-user to customize. It's unlikely, but users could run on an OS that uses some sort of threading implementation that doesn't use pthreads, but still needs to do something before the thread starts.
Part of the 'naming threads' as well is that there are some systems where you can name threads and some systems where you can't - Linux is [pthread_setname_np](https://man7.org/linux/man-pages/man3/pthread_setname_np.3.html), BSD may be [pthread_set_name_np](https://www.freebsd.org/cgi/man.cgi?query=pthread_set_name_np), and OSX uses pthread_setname_np but only [in the current thread](https://opensource.apple.com/source/libpthread/libpthread-301.30.1/pthread/pthread.h.auto.html).
I don't particularly care if the blocking of signals is off or on by default, as long as there is a way to change it.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697206398
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
(Maybe I miss something but) I dont think a mutex is required, at least not for the sigmask dance...
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695269347
##########
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:
This is related to [LOGCXX-516](https://issues.apache.org/jira/browse/LOGCXX-516), to make things ABI-stable. The CPP Core Guidelines suggest using `impl` as the class name, Qt uses `d_ptr`, I like using `priv_data`. Effectively all this is doing is putting all of the instance variables into a hidden struct so that any users of the code don't need to be recompiled incase we add/remove members into the class.
I don't have a strong opinion as to what we name it.
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697747208
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
I'm not a huge fan of it either, but I couldn't think of a better way to handle it. If you have a better idea I'd love to hear it.
The problem with using a thread local is that Thorsten's compiler [doesn't support it](https://docwiki.embarcadero.com/RADStudio/Sydney/en/Thread_local), but that's also on Windows and this is for *nix systems, so maybe we can do it anyway?
--
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
[GitHub] [logging-log4cxx] rm5248 merged pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 merged pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697873836
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
> Thorsten's compiler doesn't support it,
(Thread local is required to be supported by compiler claiming C++11 compliance; and https://logging.apache.org/log4cxx/latest_stable/dependencies.html says log4cxx requires C++11 or later....)
I agree, as Signals is a Unix thing, ignoring it might be it as well.
Another (Unix) approach could be Thread-Specific-Data (pthread_key_create(), pthread_setspecific() and friends)
Disclaimer: Never used those functions myself until now (as thread local storage is far easier to use and has less overhead).
Thinking... Another idea would be around avoiding that singleton, or at least using the singleton in a different way...
mmm....
Maybe a factory instead of the singleton? It could returns individual IThreadFunctions-derived objects and thus having its own members variables
The factory would be configured by the Configure() function;
Rather than having individual functions to be set with configureFuncs() I'd encapsulate the 3 functions in in the class/struct IThreadFunctions. IThreadFunctions could have a (maybe static) clone() function to help the factory.
(OTOH .. .I'm still not sure if this configurabiity/complixty is really needed; Possibly it would be enough to block the signals unconditionally when log4cxx starts a thread? I currently cant think of an usecase where a user would not want that log4cxx just blocks the signals. (Please explain; I'm obviously missing why you think it necessary)..)
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697925078
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
> (OTOH .. .I'm still not sure if this configurabiity/complixty is really needed; Possibly it would be enough to block the signals unconditionally when log4cxx starts a thread? I currently cant think of an usecase where a user would not want that log4cxx just blocks the signals. (Please explain; I'm obviously missing why you think it necessary)..)
My general philosophy is that sane defaults should be acceptable >90% of the time, but there are some instances where you might want to customize this for whatever reason. Hence the single-line call to ThreadUtility::configure to set various default configurations, while still allowing the end-user to customize. It's unlikely, but users could run on an OS that uses some sort of threading implementation that doesn't use pthreads, but still needs to do something before the thread starts.
Part of the 'naming threads' as well is that there are some systems where you can name threads and some systems where you can't - Linux is [pthread_setname_np](https://man7.org/linux/man-pages/man3/pthread_setname_np.3.html), BSD may be [pthread_set_name_np](https://www.freebsd.org/cgi/man.cgi?query=pthread_set_name_np), and OSX uses pthread_setname_np but only [in the current thread](https://opensource.apple.com/source/libpthread/libpthread-301.30.1/pthread/pthread.h.auto.html).
I don't particularly care if the blocking of signals is off or on by default, as long as there is a way to change it. On by default probably makes the most sense.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697927015
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
Well, pthreads is POSIX, so it should be quite portable.
(Obviously - the name bit not, but that seldom see anyone set the name of a thread)
Yes, I suggest that blocking the signals should be the default
--
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
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695517215
##########
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:
I see your point and introducing additional configs means additional necessary docs again etc. So I thought it might be a good idea to simply ask users in the ticket and [on the list](https://markmail.org/search/list:org.apache.logging.log4cxx-user#query:list%3Aorg.apache.logging.log4cxx-user+page:1+mid:ergcc3woyjmpqh2w+state:results).
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697927015
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
Well, pthreads is POSIX, so it should be quite portable.
(Obviously - the name, but that seldom see anyone set the name of a thread)
Yes, I suggest that blocking the signals should be the default
--
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
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695473112
##########
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:
I wasn't aware of the length limit, so your shorter names make perfectly sense. You are using them in most cases anyway already, no need to make it different here:
* AsyncAppend vs. AsyncAppender
* FileWatch vs. FileWatchdog
* SocketAppend vs. SocketAppenderSkeleton
* SocketHub vs. SocketHubAppender
* TelnetAppend vs. TelnetAppender
--
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
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695476624
##########
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:
All of those phrases are named pretty meaningless in the end. But your `priv_data` seems more descriptive than `impl`, so let's keep that.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697927015
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
Well, pthreads is POSIX, so it should be quite portable.
(Obviously - the name bit not, but I seldom saw anyone set the name of a thread)
Yes, I suggest that blocking the signals should be the default
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697206398
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
(Maybe I miss something but) I dont think a mutex is required, at least not for the sigmask dance...
Oh, Update... now I got it... ThreadUtil is a singleton.. Maybe it shouldn't? or the old sigmask should be stored as thread-local-data?
(my very own bikeshed color is that holding a mutex over function boundaries is fragile long-term)
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r697206398
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
(Maybe I miss something but) I dont think a mutex is required, at least not for the sigmask dance...
Oh, Update... now I got it... ThreadUtil is a singleton.. Maybe it shouldn't? or the old sigmask should be stored as thread-local-data?
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695269717
##########
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:
Outdated comment; I originally had a method that did nothing, but then I realized that just testing the `std::function` to see if it is valid does the same thing.
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695263225
##########
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:
On Linux at least, the thread name is limited to [16 characters](https://man7.org/linux/man-pages/man3/pthread_setname_np.3.html), so I wanted to keep the names as short as possible. At least in this case, there's enough space to do 'AsyncAppender'.
--
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
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695517215
##########
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:
I see your point and introducing additional configs means additional necessary docs again etc. So I thought it might be a good idea to simply ask users in the ticket and on the list.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r699005019
##########
File path: src/main/cpp/threadutility.cpp
##########
@@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,
void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
+ m_priv->creation_mutex.lock();
Review comment:
:+1:
> There are a few things that I want to take a look at in terms of how/when that gets configured.
Yes, that would be interesting informatin. One thing that would explcitly interest me is "when are the threads started". (To assess when liblog4cxx spawns it threads and therefore fork() becoming unsafe without exec())
--
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
[GitHub] [logging-log4cxx] rm5248 merged pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 merged pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695263774
##########
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:
I think that I had named things the same before; I can probably fix that easily enough.
--
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
[GitHub] [logging-log4cxx] coldtobi commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
coldtobi commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r696018350
##########
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 ){
Review comment:
I guess you want SIG_BLOCK (to be more explicit) here and store the old set somewhere....
##########
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){
+#if LOG4CXX_HAS_PTHREAD_SETNAME
+ if( pthread_setname_np( static_cast<pthread_t>( native_handle ), threadName.c_str() ) < 0 ){
+ LOGLOG_ERROR( LOG4CXX_STR("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( LOG4CXX_STR("unable to set thread name") );
+ }
+#endif
+}
+
+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") );
Review comment:
... so that you can restore the original sigmask afterwards....
--
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
[GitHub] [logging-log4cxx] rm5248 commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695274586
##########
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?
I agree they could be merged into one; I wasn't sure if it made sense to split them apart. They do technically get called at effectively the same time. I think that would make the most sense.
> So, pre and post are somewhat POSIX-specific in the end, because some aspects are inherited by new threads?
Correct.
> Or is that how C++ is documented to work on its own already?
The std::thread on Linux is effectively just a wrapper around pthreads. As far as I am aware there is nothing in the standard about signals, or how threads work at a low level.
> 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?
I was thinking about that, but I couldn't really think of a reason to call a method at the start of a thread in that thread. Doing that properly would be somewhat odd, due to how you start threads in C++(variable arguments).
--
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
[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #68: LOGCXX-431
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on a change in pull request #68:
URL: https://github.com/apache/logging-log4cxx/pull/68#discussion_r695481684
##########
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:
> I agree they could be merged into one; I wasn't sure if it made sense to split them apart. They do technically get called at effectively the same time.[...]
Their purpose/focus/... might still be different: `pre` and `post` focus on preparing/resetting the environment for/after new threads, the other callback focuses on configuring/... the created thread itself. Doesn't sound too wrong to me, I'm fine with keeping things that way.
--
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