LOGCXX-431 #68

rm5248 · 2021-08-21T15:58:09Z

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.

In order to make it possible to block signals when a thread is created, a new API to let users configure what to do when a thread is started has been created. Documentation on how Log4cxx handles threads, and information about good ways to handle signals has been added as well.

ams-tschoening

I'm unable to test right new because VS is broken in my version, but I do have some notes.

ams-tschoening · 2021-08-24T06:51:08Z

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


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?

On Linux at least, the thread name is limited to 16 characters, so I wanted to keep the names as short as possible. At least in this case, there's enough space to do 'AsyncAppender'.

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

ams-tschoening · 2021-08-24T06:52:19Z

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


Filewatchdog vs. FileWatchdog vs. AsyncAppender vs. SocketAppend ... Lower case looks like a mistake.

src/main/cpp/threadutility.cpp

ams-tschoening · 2021-08-24T07:07:55Z

src/main/cpp/threadutility.cpp

+
+void ThreadUtility::configureThreadFunctions( pre_thread_start pre_start1,
+							   thread_started started,
+							   post_thread_start post_start1 ){


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?

I think that I had named things the same before; I can probably fix that easily enough.

src/main/cpp/threadutility.cpp

ams-tschoening · 2021-08-24T08:05:05Z

src/main/include/log4cxx/helpers/threadutility.h

+	/**
+	 * 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();


I can't find an implementation of postThreadDoNothing. Did I miss something or is the function missing or the comment outdated?

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.

ams-tschoening · 2021-08-24T08:06:33Z

src/main/cpp/threadutility.cpp

+
+void ThreadUtility::threadStartedNameThread(LogString threadName,
+							 std::thread::id /*thread_id*/,
+							 std::thread::native_handle_type native_handle){


Opinion: threadName vs. thread_name vs. native_handle vs. nativeHandle

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.

src/main/include/log4cxx/helpers/threadutility.h

ams-tschoening · 2021-08-24T08:31:34Z

src/site/markdown/threading.md

+can use some sample functions(not no-ops) as follows:
+
+```
+ThreadUtility::instance()->configureThreadFunctions( ThreadUtility::preThreadBlockSignals,


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.

The reasoning behind this is that:

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.

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.

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.

ams-tschoening · 2021-08-24T08:41:18Z

src/test/cpp/helpers/threadutilitytestcase.cpp

+		},
+		[&num_post](){
+			num_post++;
+		});


Opinion: There's a level of indentation missing in the args to configureFunctions which make them difficult to read.

thrUtil->configureThreadFunctions( [&num_pre](){ num_pre++; }, [&num_started]( LogString, std::thread::id, std::thread::native_handle_type ){ num_started++; }, [&num_post](){ num_post++; } );

coldtobi · 2021-08-25T18:38:14Z

src/main/cpp/threadutility.cpp

+#if LOG4CXX_HAS_PTHREAD_SIGMASK
+	sigset_t set;
+	sigfillset(&set);
+	if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){


I guess you want SIG_BLOCK (to be more explicit) here and store the old set somewhere....

coldtobi · 2021-08-25T18:38:21Z

src/main/cpp/threadutility.cpp

+	sigset_t set;
+	sigemptyset(&set);
+	if( pthread_sigmask(SIG_SETMASK, &set, nullptr) < 0 ){
+		LOGLOG_ERROR( LOG4CXX_STR("Unable to set thread sigmask") );


... so that you can restore the original sigmask afterwards....

…figure the thread utility

ams-tschoening

Still can't test, but things look fine enough for me.

coldtobi · 2021-08-27T07:11:45Z

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


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

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?

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

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

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

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.

👍

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

rm5248 added 11 commits August 19, 2021 21:08

Check for linux-specific pthread_setname_np

36e33e5

Create strings as LOGSTRING to compile on windows

0b08d4e

More logstrings

60184d7

Fix pthread name test for linux

3687a8b

Turned the thread utility into a singleton

39bbf1b

Made documentation on logging threaded clearer

6e725a7

setname actually tests properly

46ad073

Fixed documentation

a82c4d9

Added unit test for the thread utility

187c181

Moved to helpers namespace and made test compile on windows

a41756b

rm5248 requested a review from ams-tschoening August 21, 2021 15:58

ams-tschoening requested changes Aug 24, 2021

View reviewed changes

coldtobi reviewed Aug 25, 2021

View reviewed changes

rm5248 added 4 commits August 26, 2021 20:22

Made some thread names more descriptive

f728135

Fixed issues seen in review

7d67642

Properly set and restore the thread mask

1006b6f

Updated test case to try default methods. Added utility method to con…

c37d127

…figure the thread utility

ams-tschoening approved these changes Aug 27, 2021

View reviewed changes

coldtobi reviewed Aug 27, 2021

View reviewed changes

rm5248 added 2 commits August 29, 2021 20:24

use thread_local on *nix

890c72f

typo when changing code style

2b232f6

rm5248 merged commit 4dd4398 into master Aug 30, 2021

rm5248 deleted the LOGCXX-431 branch August 30, 2021 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOGCXX-431 #68

LOGCXX-431 #68

rm5248 commented Aug 21, 2021 •

edited by ams-tschoening

ams-tschoening left a comment

ams-tschoening Aug 24, 2021

rm5248 Aug 24, 2021

ams-tschoening Aug 25, 2021

ams-tschoening Aug 24, 2021

ams-tschoening Aug 24, 2021

rm5248 Aug 24, 2021

ams-tschoening Aug 24, 2021

rm5248 Aug 24, 2021

ams-tschoening Aug 24, 2021

rm5248 Aug 27, 2021

ams-tschoening Aug 24, 2021

rm5248 Aug 24, 2021

ams-tschoening Aug 25, 2021 •

edited

ams-tschoening Aug 24, 2021

coldtobi Aug 25, 2021

coldtobi Aug 25, 2021

ams-tschoening left a comment

coldtobi Aug 27, 2021 •

edited

rm5248 Aug 27, 2021

coldtobi Aug 28, 2021

rm5248 Aug 28, 2021 •

edited

coldtobi Aug 28, 2021 •

edited

rm5248 Aug 30, 2021

coldtobi Aug 31, 2021

LOGCXX-431 #68

LOGCXX-431 #68

Conversation

rm5248 commented Aug 21, 2021 • edited by ams-tschoening

ams-tschoening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ams-tschoening Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ams-tschoening left a comment

Choose a reason for hiding this comment

coldtobi Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rm5248 Aug 28, 2021 • edited

Choose a reason for hiding this comment

coldtobi Aug 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rm5248 commented Aug 21, 2021 •

edited by ams-tschoening

ams-tschoening Aug 25, 2021 •

edited

coldtobi Aug 27, 2021 •

edited

rm5248 Aug 28, 2021 •

edited

coldtobi Aug 28, 2021 •

edited