signal: new signal handling backend based on signalfd(2) #1342
Conversation
3834d93 to
45f0232
Compare
|
Could I run CI jobs myself? |
45f0232 to
d7f8284
Compare
Sadly, but first-time contributors requires an approve for CI. |
d7f8284 to
b38c6b2
Compare
I will mark it as draft, please press |
I will merge you #1339, and then CI will started automatically for you. |
b38c6b2 to
7b5cca7
Compare
7b5cca7 to
2db8115
Compare
2db8115 to
7913a0c
Compare
|
Any chance to consider this for upstream? |
Yes, this is a great patch! Just let me take a closer look. |
There was a problem hiding this comment.
LGTM, please look at the comments.
Also what bothers me most is that it is not possible to use old signal API, and with signalfd yo cannot catch SIGSEGV/SIGFPU (though it is not an issue for almost 99% code, but I'm worrying that there can be the code that relies on this).
So that said, I think we need a way to get back to old signal-based API (and I'm fine to use signalfd by default, since it is cleaner code), and let's also add a test run with this old signal-based API.
| @@ -1260,6 +1260,9 @@ test_signal_pipeloss(void) | |||
| * make two bases to catch signals, use both of them. this only works | |||
| * for event mechanisms that use our signal pipe trick. kqueue handles | |||
| * signals internally, and all interested kqueues get all the signals. | |||
| * This is not expected to work with signalfd - having more than one | |||
| * descriptor in attempt to accept the same signal (or intersecting sets | |||
| * of signals) is not the thing signalfd() was designed for. | |||
There was a problem hiding this comment.
The problem is not signalfd, but SIG_UNBLOCK
There was a problem hiding this comment.
Why SIG_UNBLOCK? The limitation is clearly stated in signalfd() manual page as:
If a signal appears in the mask of more than one of the file descriptors, then occurrences of that signal can be read (once) from any one of the file descriptors.
IOW if you poll() more than one descriptor in attempt to accept the same signal, all of them will be marked active (with POLLIN in revents after poll() return) on signal arrival, but you can read struct signalfd_siginfo from the only one of them, and attempt to read another active descriptor(s) always returns -1.
There was a problem hiding this comment.
Here I'm talking about the test, look:
$ strace -e/signal,/proc regress --no-fork signal/signal_switchbase
signal/signal_switchbase: rt_sigprocmask(SIG_BLOCK, [USR1], NULL, 8) = 0
signalfd4(-1, [USR1], 8, SFD_CLOEXEC|SFD_NONBLOCK) = 11
rt_sigprocmask(SIG_BLOCK, [USR1], NULL, 8) = 0
signalfd4(-1, [USR1], 8, SFD_CLOEXEC|SFD_NONBLOCK) = 12
rt_sigprocmask(SIG_UNBLOCK, [USR1], NULL, 8) = 0
--- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_USER, si_pid=32667, si_uid=1000} ---
+++ killed by SIGUSR1 +++
User defined signal 1There was a problem hiding this comment.
So because there is no way to tell that this signal had been used in another base, this does not work.
But this is OK.
7913a0c to
bdb2df2
Compare
bdb2df2 to
5057027
Compare
Linux-specific signal handling backend based on signalfd(2) system call, and public function event_base_get_signal_method() to obtain an underlying kernel signal handling mechanism. Signed-off-by: Dmitry Antipov <dantipov@cloudlinux.com>
5057027 to
1af745d
Compare
|
Applied, thank you! For this the following places should be modified:
|
signalfd may behave differently to sigaction/signal, so to avoid breaking libevent users (like [1], [2]) disable it by default. [1]: tmux/tmux#3621 [1]: tmux/tmux#3626 Also signalfd is not that perfect: - you need to SIG_BLOCK the signal before - blocked signals are not reset on exec - blocked signals are allowed to coalesce - so in case of multiple signals sent you may get the signal only once (ok for most of the signals, but may be a problem for SIGCHLD, though you may call waitpid() in a loop or use pidfd) - and also one implementation problem - sigprocmask is unspecified in a multithreaded process Refs: - https://lwn.net/Articles/415684/ - https://ldpreload.com/blog/signalfd-is-useless Refs: libevent#1460 Refs: libevent#1342 (cc @dmantipov)
signalfd may behave differently to sigaction/signal, so to avoid breaking libevent users (like [1], [2]) disable it by default. [1]: tmux/tmux#3621 [2]: tmux/tmux#3626 Also signalfd is not that perfect: - you need to SIG_BLOCK the signal before - blocked signals are not reset on exec - blocked signals are allowed to coalesce - so in case of multiple signals sent you may get the signal only once (ok for most of the signals, but may be a problem for SIGCHLD, though you may call waitpid() in a loop or use pidfd) - and also one implementation problem - sigprocmask is unspecified in a multithreaded process Refs: - https://lwn.net/Articles/415684/ - https://ldpreload.com/blog/signalfd-is-useless Refs: libevent#1460 Refs: libevent#1342 (cc @dmantipov)
signalfd may behave differently to sigaction/signal, so to avoid breaking libevent users (like [1], [2]) disable it by default. [1]: tmux/tmux#3621 [2]: tmux/tmux#3626 Also signalfd is not that perfect: - you need to SIG_BLOCK the signal before - blocked signals are not reset on exec - blocked signals are allowed to coalesce - so in case of multiple signals sent you may get the signal only once (ok for most of the signals, but may be a problem for SIGCHLD, though you may call waitpid() in a loop or use pidfd) - and also one implementation problem - sigprocmask is unspecified in a multithreaded process Refs: - https://lwn.net/Articles/415684/ - https://ldpreload.com/blog/signalfd-is-useless Refs: libevent#1460 Refs: libevent#1342 (cc @dmantipov)
Linux-specific signal handling backend based on
signalfd(2)system call, and public functionevent_base_get_signal_method()to obtain an underlying kernel signal handling mechanism.