GNOME Bugzilla – Bug 731228
worker: don't block SIGUSR1
Last modified: 2014-09-02 19:14:29 UTC
When the slaves were folded into the main manager process, we had to add support for detecting multiple simultaneous X servers starting up. We did that by creating a detected thread to listen for SIGUSR1 and then examining the siginfo to find out which X server is ready. In order to ensure only the dedicated thread received the SIGUSR1 signals from the X server we blocked SIGUSR1 from the main thread. That blocked signal is inherited to the worker processes and the session. Some programs depend on SIGUSR1 but don't explicitly unblock SIGUSR1 at start up, since the signal is presumed to be unblocked out the box. This commit makes sure SIGUSR1 is unblocked before starting the session to keep these programs functioning correctly.
Created attachment 277891 [details] [review] worker: don't block SIGUSR1
Review of attachment 277891 [details] [review]: Sure. At this point I'd just rewrite the code to use -displayfd, but socket activation is the future, so perhaps it isn't worth it.
-displayfd is fine except it only recently gained the ability to be run with a prespecified display. btw, there's a race in this code I didn't think about before. if you have 3 or more displays starting at the same time, we'll potentially drop the ready notification for some of them since SIGUSR1 doesn't queue more than one pending handler. So if we did want to keep using SIGUSR1, a better approach would be to set up an SA_DEFERRED handler that sendmsg's out a pipe the pid of the ready X server (handling EINTR when the SIGUSR1 recurses for a flurry of activity)
Attachment 277891 [details] pushed as ef69548 - worker: don't block SIGUSR1
(In reply to comment #3) > btw, there's a race in this code I didn't think about before. if you have 3 or > more displays starting at the same time, we'll potentially drop the ready > notification for some of them since SIGUSR1 doesn't queue more than one pending > handler. Are you sure? From http://lwn.net/Articles/414618/ """ In more recent years the collection of signal types has been extended to include "realtime" signals. These signals are user-defined signals (like SIGUSR1 and SIGUSR2) which are only delivered if explicitly requested in some way. They have two particular properties. Firstly, realtime signals are queued so the handler in the target process is called exactly as many times as the signal was sent. This contrasts with regular signals which simply set a flag on delivery. If a process has a given (regular) signal blocked and the signal is sent several times, then, when the process unblocks the signal, it will still only see a single delivery event. With realtime signals it will see several. """
that text is confusingly written, but SIGUSR1 and SIGUSR2 aren't real-time signals. What it's saying is real-time signals have no predetermined purpose (much like SIGUSR1 and SIGUSR2 have no predetermined purpose), and can be used freely from applications without worry of the kernel delivering it for some other reason. In addition to being user-defined, they're also queued (sadly unlike SIGUSR1 and SIGUSR2)
Created attachment 284967 [details] [review] Reset signal mask to really unblock SIGUSR1 I'm afraid the current fix here isn't sufficient, as singal(SIGUSR1, SIG_DFL) won't unset the signal mask set with sigprocmask(), which is inherited by the children as well. Attached patch properly resets the signal mask so the session (and its children) don't get a bogus default signal mask (which, as stated above, breaks any application relying on SIGUSR1, like dd or redshift). As this is done in the child process after the fork(), it won't affect the parent. I don't know however if we still need signal(SIGUSR1, SIG_DFL) in this code path, but as I'm not sure I didn't remove it -- it should be at worse harmless anyway. Anyway, with this patch applied my apps start receiving SIGUSR1 properly again.
Looks right to me.