signal: new signal handling backend based on signalfd(2)#1342
signal: new signal handling backend based on signalfd(2)#1342azat merged 1 commit intolibevent:masterfrom dmantipov:signal-use-signalfd-if-available
Conversation
|
Could I run CI jobs myself? |
Sadly, but first-time contributors requires an approve for CI. |
I will mark it as draft, please press |
azat
left a comment
There was a problem hiding this comment.
In general this is a good thing, thank you!
Will take a look once you will finish it.
I will merge you #1339, and then CI will started automatically for you. |
|
Any chance to consider this for upstream? |
Yes, this is a great patch! Just let me take a closer look. |
azat
left a comment
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.
| * 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.
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>
|
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.