Re: [log4cxx] Block signals by default?
On Wed, 2021-08-25 at 12:35 -0400, Robert Middleton wrote: > > (Disclaimer: I only briefly skimmed the PR, so I might got things wrong.) > > > > Some thoughts: > > - I would not impose on applications should handle signals. This could > > be a mayor annoyance, especially if applications needs being changed of > > that > > (this refering to the docs about signalfd etc.) > > My objective is to not define how applications should handle signals, > the documentation is really in terms of "here's a way you can handle > signals better." That should be more clear though, so I can update > that. Thanks for the clarifiaction; I indeed understood it that users of the library needs to use the mentioned mechanism instead. (A documentation how to use signals in general felt for me a bit out of scope for a library about logging... Maybe that contributed to my confusion.) > > - A question: does log4cxx use signals of its own? > > If not, I would use the approach to block all signals before pthread_create() > > and then restore them afterwards. > > > > Log4cxx does not use any signals on its own, so the question is should > we do this automatically or not? If you're using signalfd or > something similar, there's no need to block signals before a thread > starts, as it bypasses the issue at that point. The problem is if you don't. There a quite a few ways to handle signals, and liblog4cxx should not get in they way of those... You can even combine those methods, to make it even more complicated... For example, my mentioned §DAYJOB app uses signal handler for "fatal" signals like SIGSEV, SIGABORT and so on where I want to get an backtrace when it happens, but for users I use a dedicated signal handler thread (eg. SIGUSR1), using sig_wait() in my case... So what do to? Its actually not so easy to answer... My instict would say: get the spawned threads out of the way, in terms of signals. That means before starting the threads blocking all signals using pthread_sigmask(), spawn the thread, and then restore the previous signals set. This way, the signal handling should be "unchanged" for the rest of the application, regardless which method the application use to tackle signals. Anhother approach could be to document the issue and let the user do the stuff themselves, by providing them a hook for before and after pthread_create() or some callbacks (I think this is the approach the PR takes), with possibly being the default implmentatino the pthread_sigmask* dance from above (a default would help to keep ABI changes minimal)... (However, at the moment I'd not see really why it would be necessary to let the user have this complexity.) (* I've seen in the PR the pthread_Sigmask thing, however IIRC it fails to restore the previous signal set. ) Cheers, -- tobi
Re: [log4cxx] Block signals by default?
> (Disclaimer: I only briefly skimmed the PR, so I might got things wrong.) > > Some thoughts: > - I would not impose on applications should handle signals. This could > be a mayor annoyance, especially if applications needs being changed of that > (this refering to the docs about signalfd etc.) > My objective is to not define how applications should handle signals, the documentation is really in terms of "here's a way you can handle signals better." That should be more clear though, so I can update that. > - A question: does log4cxx use signals of its own? > If not, I would use the approach to block all signals before > pthread_create() > and then restore them afterwards. > Log4cxx does not use any signals on its own, so the question is should we do this automatically or not? If you're using signalfd or something similar, there's no need to block signals before a thread starts, as it bypasses the issue at that point. -Robert Middleton
Re: [log4cxx] Block signals by default?
(Disclaimer: I only briefly skimmed the PR, so I might got things wrong.) Some thoughts: - I would not impose on applications should handle signals. This could be a mayor annoyance, especially if applications needs being changed of that (this refering to the docs about signalfd etc.) - A question: does log4cxx use signals of its own? If not, I would use the approach to block all signals before pthread_create() and then restore them afterwards. In one of my $DAYJOB projects I'm doing something along that to archieve that (typed out of memory) sigset_t allset, oldset; sigfillset(); pthread_sigmask(SIG_BLOCK, , ); pthread_create(...); pthread_sigmask(SIG_SET,); (- Another maybe unrelated thought (people were biten by this in log4cpp -- https://github.com/log4cplus/log4cplus/issues/466): After threads have been created, it is not safe anymore to fork() without exec(). This might be a mayjor problem it threads spawn automatically, but I guess in log4cpp they are only spawned when using the AsyncAppender, so it should be fine.) -- tobi
[log4cxx] Block signals by default?
Hi everyone, there's an open issue about log4cxx not blocking signals and therefore behaving somewhat badly. https://issues.apache.org/jira/browse/LOGCXX-431 This is issue is being worked on currently, but a question came up about if to block signals by default, leave that decision to devs in code or provide some config to switch blocking vs. not blocking by default. https://github.com/apache/logging-log4cxx/pull/68#discussion_r695277961 Additional configs might not be necessary at all compared to only one line of code otherwise necessary. Keeping the current behaviour is backwards compatible and how things have always been, so won't break too much most likely. OTOH, if the current default behaviour is PITA for a lot of users, it might be worth changing it. So what's user's opinion about this? Thanks! Mit freundlichen Grüßen Thorsten Schöning -- AM-SoFT IT-Service - Bitstore Hameln GmbH Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK E-Mail: thorsten.schoen...@am-soft.de Web:http://www.AM-SoFT.de/ Tel: 05151- 9468- 0 Tel: 05151- 9468-55 Fax: 05151- 9468-88 Mobil: 0178-8 9468-04 AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
