Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LOGCXX-431 #68

Merged
merged 17 commits into from
Aug 30, 2021
Merged

LOGCXX-431 #68

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
091a9ce
LOGCXX-431 Update thread creation
rm5248 Aug 20, 2021
36e33e5
Check for linux-specific pthread_setname_np
rm5248 Aug 20, 2021
0b08d4e
Create strings as LOGSTRING to compile on windows
rm5248 Aug 21, 2021
60184d7
More logstrings
rm5248 Aug 21, 2021
3687a8b
Fix pthread name test for linux
rm5248 Aug 21, 2021
39bbf1b
Turned the thread utility into a singleton
rm5248 Aug 21, 2021
6e725a7
Made documentation on logging threaded clearer
rm5248 Aug 21, 2021
46ad073
setname actually tests properly
rm5248 Aug 21, 2021
a82c4d9
Fixed documentation
rm5248 Aug 21, 2021
187c181
Added unit test for the thread utility
rm5248 Aug 21, 2021
a41756b
Moved to helpers namespace and made test compile on windows
rm5248 Aug 21, 2021
f728135
Made some thread names more descriptive
rm5248 Aug 27, 2021
7d67642
Fixed issues seen in review
rm5248 Aug 27, 2021
1006b6f
Properly set and restore the thread mask
rm5248 Aug 27, 2021
c37d127
Updated test case to try default methods. Added utility method to co…
rm5248 Aug 27, 2021
890c72f
use thread_local on *nix
rm5248 Aug 30, 2021
2b232f6
typo when changing code style
rm5248 Aug 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 20 additions & 8 deletions src/main/cpp/threadutility.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "log4cxx/helpers/loglog.h"

#include <signal.h>
#include <mutex>

#if LOG4CXX_HAS_SETTHREADDESCRIPTION
#include <windows.h>
Expand All @@ -21,6 +22,11 @@ struct ThreadUtility::priv_data{
log4cxx::helpers::ThreadStartPre start_pre;
log4cxx::helpers::ThreadStarted started;
log4cxx::helpers::ThreadStartPost start_post;
#if LOG4CXX_HAS_PTHREAD_SIGMASK
std::mutex creation_mutex;
sigset_t old_mask;
bool sigmask_valid;
#endif
};
ams-tschoening marked this conversation as resolved.
Show resolved Hide resolved

ThreadUtility::ThreadUtility() :
Expand All @@ -44,19 +50,23 @@ void ThreadUtility::configureFuncs( ThreadStartPre pre_start,

void ThreadUtility::preThreadBlockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
m_priv->creation_mutex.lock();
Copy link
Contributor

@coldtobi coldtobi Aug 27, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, but that's also on Windows and this is for *nix systems, so maybe we can do it anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)..)

Copy link
Contributor Author

@rm5248 rm5248 Aug 28, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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, BSD may be pthread_set_name_np, and OSX uses pthread_setname_np but only in the current thread.

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.

Copy link
Contributor

@coldtobi coldtobi Aug 28, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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())

sigset_t set;
sigfillset(&set);
if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){
if( pthread_sigmask(SIG_SETMASK, &set, &m_priv->old_mask) < 0 ){
LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );
m_priv->sigmask_valid = false;
}else{
m_priv->sigmask_valid = true;
}
#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */
}

void ThreadUtility::threadStartedNameThread(LogString threadName,
std::thread::id /*thread_id*/,
std::thread::native_handle_type native_handle){
std::thread::id /*threadId*/,
std::thread::native_handle_type nativeHandle){
#if LOG4CXX_HAS_PTHREAD_SETNAME
if( pthread_setname_np( static_cast<pthread_t>( native_handle ), threadName.c_str() ) < 0 ){
if( pthread_setname_np( static_cast<pthread_t>( nativeHandle ), threadName.c_str() ) < 0 ){
LOGLOG_ERROR( LOG4CXX_STR("unable to set thread name") );
}
#elif LOG4CXX_HAS_SETTHREADDESCRIPTION
Expand All @@ -69,11 +79,13 @@ void ThreadUtility::threadStartedNameThread(LogString threadName,

void ThreadUtility::postThreadUnblockSignals(){
#if LOG4CXX_HAS_PTHREAD_SIGMASK
sigset_t set;
sigemptyset(&set);
if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){
LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );
// Only restore the signal mask if we were able to set it in the first place.
if( m_priv->sigmask_valid ){
if( pthread_sigmask(SIG_SETMASK, &m_priv->old_mask, nullptr) < 0 ){
LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );
}
}
m_priv->creation_mutex.unlock();
#endif /* LOG4CXX_HAS_PTHREAD_SIGMASK */
}

Expand Down