timeout: adding all signal handlers, passing on signals to child, adding ignored signal handling #10254
Conversation
ChrisDryden
force-pushed
the
timeout-fixes-squashed
branch
from
93ea128 to
228824e
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Another note is that we have a bunch of over-rides in the build-gnu script that override the yes and timeout command that we can remove once this is removed. |
CodSpeed Performance ReportMerging this PR will improve performance by 4.01%Comparing Summary
Performance Changes
Footnotes
|
| #[uucore::main] | ||
| pub fn uumain(args: impl uucore::Args) -> UResult<()> { | ||
| #[cfg(unix)] | ||
| if !uucore::signals::sigpipe_was_ignored() { |
There was a problem hiding this comment.
like I wrote in #10265 I think we're going to need this almost for all utilities. We can merge this for now, but the call to enable_pipe_errors() will probably need to be removed from yes.rs and moved to a central place.
| return; | ||
| } | ||
| let _ = process.send_signal_group(signal); | ||
| let kill_sig = signal_by_name_or_value("KILL").unwrap(); |
There was a problem hiding this comment.
nit: The variable renames (kill_signal -> kill_sig, continued_signal -> cont_sig) add diff noise without
functional benefit. Consider keeping original names to make the actual changes clearer. Not blocking though.
| let kill_sig = signal_by_name_or_value("KILL").unwrap(); | ||
| let cont_sig = signal_by_name_or_value("CONT").unwrap(); | ||
| if signal != kill_sig && signal != cont_sig { | ||
| let _ = process.send_signal(cont_sig); |
There was a problem hiding this comment.
for reference, because it took me a while to understand: this extra call to send_signal(cont_sig) makes sense to handle the corner-case where the child process has created its own process-group using setpgid().
When that's not the case, the child process gets two SIGCONT, and the second one is harmless.
| static RECEIVED_SIGNAL: std::sync::atomic::AtomicI32 = std::sync::atomic::AtomicI32::new(0); | ||
|
|
||
| /// Install signal handlers for termination signals. | ||
| fn install_signal_handlers(term_signal: usize) { |
There was a problem hiding this comment.
Note: this replaces catch_sigterm() which was too limited and wasn't handling e.g. SIGINT
| Signal::SIGUSR1, | ||
| Signal::SIGUSR2, | ||
| ] { | ||
| if sig != Signal::SIGPIPE || !sigpipe_ignored { |
There was a problem hiding this comment.
suggestion to avoid the double negative and improve readability:
if sig == Signal::SIGPIPE && sigpipe_ignored {
continue; // Skip SIGPIPE if it was ignored by parent
}
let _ = unsafe { nix::sys::signal::signal(sig, handler) };
| Signal::SIGTERM, | ||
| Signal::SIGPIPE, | ||
| Signal::SIGUSR1, | ||
| Signal::SIGUSR2, |
There was a problem hiding this comment.
why only those signals? why not SIGILL, SIGTRAP, SIGABRT, SIGBUS,SIGSEGV, etc.? and why not check if those signals are set to ignored by the parent process?
Is it because this logic will be extended later?
2 participants
There was quite a bunch of different features missing in timeout, to start off, now that we have a mechanism to read the SIGPIPE handlers before they are overwritten by the rust runtime, it means that we can not propagate this signal down to the child processes if the signal is set to ignore.
This also includes all of the latest changes since 9.9 where the specific signal sent to timeout will be propagated instead of just defaulting to a TERM signal. For the timeout GNU integration tests to pass I also had to enable to pipe signal handlers to the yes utility since the test relied on that utility outputting the correct error code when ignoring the the pipe signal.