Repository navigation
Don't let the wrapper shell's job notification block kill() - #88
Merged
Merged
Conversation
When a job it waits for is killed by a signal, the wrapper shell reports it on its stderr
("Killed: 9" from bash, which is /bin/sh on macOS), and its stderr is the stderr pipe of
the process. If the process filled that pipe with output nobody read, the shell blocked
writing the notice and never exited, and the blocking waitpid() of reapShell() in kill()
(and in PosixHandle::__destruct() once the exit code was read, since amphp#86) waited for it
forever, holding the event loop that would have drained the pipe.
Discard the stderr of the wait builtin: the process's own stderr is redirected by the
command itself, the notice is the only thing the shell writes there.
Member
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mechanism
When a job it waits for dies from a signal, the wrapper shell reports it on its stderr: bash, which is
/bin/shon macOS, printsKilled: 9. That stderr is the process's stderr pipe. If the process had filled the pipe with output nobody read, the shell blocks writing the notice and never exits.Since #86,
kill()callsreapShell(), a blockingpcntl_waitpid()on the shell, right after the SIGKILL (and so doesPosixHandle::__destruct()once the exit code has been read). That wait holds the event loop, which is what would drain the pipe. Nothing can move, andkill()never returns.I hit it with amphp/parallel:
ContextWorkerPoolkills its workers at shutdown. A worker that printed more than a pipe's worth (64 KiB) at startup while the parent wasn't running the loop — here, PHP 8.5 deprecation notices from dev dependencies, 85 KB on every start — hangs the parent forever, stuck inpcntl_waitpid().Fix
The wrapper discards the stderr of the
waitbuiltin (wait $pid 2>/dev/null). The process's own stderr isn't affected: the notice is the only thing the shell itself writes there.Test
testKillWithUnreadOutputkills a process that wrote 1 MiB to stderr that nobody read. The kill runs in a child PHP script, because akill()that never returns blocks the test's own event loop, and the test joins it with a timeout. It fails without the fix (macOS, PHP 8.5) and passes with it.testSignalfails locally with or without this change: see the separate PR for its race.