interp: dup pipe fds in pipelines so EOF and SIGPIPE propagate#1334
Closed
qiangli wants to merge 1 commit into
Closed
interp: dup pipe fds in pipelines so EOF and SIGPIPE propagate#1334qiangli wants to merge 1 commit into
qiangli wants to merge 1 commit into
Conversation
Owner
|
Can we write a regression test for this that fails without the fix? Otherwise, the fix is going to be rather fragile. |
When the parent Go process holds extra references to a pipeline's read or write end, EOF does not propagate to readers and SIGPIPE is not delivered to writers. Concretely, `yes | head -1` hangs forever instead of terminating after head closes its stdin. Fix this by duplicating both pipe ends with syscall.Dup before handing them to the goroutines that run the left and right sides, then immediately closing the originals in the parent. The parent no longer holds any reference to the pipe; closing the duplicates in the goroutines is now sufficient to drive EOF/SIGPIPE through the pipeline. The duplicate fds are marked CloseOnExec so external commands spawned from inside the goroutines do not inherit them. Restoring r.stdin from oldStdin after wg.Wait() prevents the right-hand side's stdin replacement from leaking into subsequent statements in the enclosing block. A new helper, dupPipeFd, lives in interp/os_unix.go on unix platforms and as a no-op fallback in interp/os_notunix.go. On non-unix the previous behaviour is preserved (pipelines run but EOF/SIGPIPE propagation is best-effort). Fixes mvdan#1142
qiangli
force-pushed
the
interp-pipe-fd-eof
branch
from
15dca86 to
12f5191
Compare
Owner
|
@qiangli did you see my comment above? |
Author
|
Sorry, I did not see. I may create another pull request, closing this one for now. |
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
2 participants
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.
Summary
When the parent Go process holds extra references to a pipeline's read or write end, EOF does not propagate to readers and SIGPIPE is not delivered to writers. Concretely,
yes | head -1hangs forever instead of terminating afterheadcloses its stdin.Approach
Duplicate both pipe ends with
syscall.Dupbefore handing them to the goroutines that run the left and right sides, then immediately close the originals in the parent. The parent no longer holds any reference to the pipe; closing the duplicates in the goroutines is now sufficient to drive EOF/SIGPIPE through the pipeline.CloseOnExecso external commands spawned from inside the goroutines do not inherit them.r.stdinfromoldStdinafterwg.Wait()prevents the right-hand side's stdin replacement from leaking into subsequent statements in the enclosing block.dupPipeFdlives ininterp/os_unix.goon unix platforms and as a no-op fallback ininterp/os_notunix.go. On non-unix the previous behaviour is preserved (pipelines run but EOF/SIGPIPE propagation is best-effort).Fixes #1142.
Test plan
go test -short ./interp/passes.yes | head -1exits promptly under the patched runner; previously hung indefinitely.