Repository navigation
os/exec: add fields for managing termination signals and pipes #50436
Description
Background
#23019 (accepted but not yet implemented; CC @ianlancetaylor @bradfitz) proposed to change exec.Cmd.Wait to stop the goroutines that are copying I/O to and from a completed exec.Cmd; see that proposal for further background on the problem it aims to address. However, as noted in #23019 (comment) and #23019 (comment), any feasible implementation of the proposal requires the use of an arbitrary timeout, and the proposal does not include a mechanism to adjust that timeout. (Given our history with the Go project's builders, I am extremely skeptical that any particular hard-coded timeout can strike an appropriate balance between robustness and latency.)
#31774, #22757, and #21135 proposed to allow users of exec.CommandContext to customize the signal sent to the command when the context is canceled. They were all declined due to lack of concrete demand for the feature (#21135 (comment), #22757 (comment), #31774 (comment)). We have since accrued a number of copies of functions that work around the feature's absence. In the Go project alone, we have:
- https://cs.opensource.google/go/go/+/master:src/cmd/go/script_test.go;l=1233-1292;drc=af3b8cf5026a9d74a53ac5d3fec2bba35a96ad91
- https://cs.opensource.google/go/go/+/master:src/internal/testenv/testenv.go;l=312-366;drc=cc795a01dcec7c97044b31571af88ac98310f2b3 (added for runtime: hangs in TestGdbBacktrace on linux #37405)
- https://cs.opensource.google/go/x/build/+/master:internal/internal.go;l=28-86;drc=fa5c6e87e680b621066712172cd92426fb206416
- https://cs.opensource.google/go/x/playground/+/master:internal/internal.go;l=14-70;drc=ccdf1e5d22bc9653f3a3b58016d2c9914ebaad48
I'm attempting to add yet another variation (in CL 373005) in order to help diagnose #50014. However, for this variation (prompted by discussions with @aclements and @prattmic) I have tried to make this variation a minimally-invasive change on top of the exec.Cmd API.
I believe I have achieved that goal: the API requires the addition of only 2–3 new fields and no new methods or top-level functions. You can view (and try out) a prototype as github.com/bcmills/more/os/moreexec, which provides a drop-in replacement for a subset of the exec.Cmd API.
Proposal
I propose the addition of the following fields to the exec.Cmd struct, along with their corresponding implementation:
// Context is the context that controls the lifetime of the command
// (typically the one passed to CommandContext).
Context context.Context
// If Interrupt is non-nil, Context must also be non-nil and Interrupt will be
// sent to the child process when Context is done.
//
// If the command exits with a success code after the Interrupt signal has
// been sent, Wait and similar methods will return Context.Err()
// instead of nil.
//
// If the Interrupt signal is not supported on the current platform
// (for example, if it is os.Interrupt on Windows), Start may fail
// (and return a non-nil error).
Interrupt os.Signal
// If WaitDelay is non-zero, the command's I/O pipes will be closed after
// WaitDelay has elapsed after either the command's process has exited or
// (if Context is non-nil) Context is done, whichever occurs first.
// If the command's process is still running after WaitDelay has elapsed,
// it will be terminated with os.Kill before the pipes are closed.
//
// If the command exits with a success code after pipes are closed due to
// WaitDelay and no Interrupt signal has been sent, Wait and similar methods
// will return ErrWaitDelay instead of nil.
//
// If WaitDelay is zero (the default), I/O pipes will be read until EOF,
// which might not occur until orphaned subprocesses of the command have
// also closed their descriptors for the pipes.
WaitDelay time.DurationThe new Context field is exported only in order to simplify the documentation for the Interrupt and WaitDelay fields. (It was requested and rejected in #46699, but the objection there was my own — due to concerns about the interactions with the API in this proposal. It could be excised from this proposal without damaging anything but documentation clarity.)
The new Interrupt field sets the signal to be sent when the Context is done. exec.CommandContext explicitly sets it to os.Kill in order to maintain the existing behavior of exec.CommandContext, but I expect many users on Unix platforms will want to set it to os.Interrupt or syscall.SIGQUIT instead.
The new WaitDelay field sets the interval to wait for input and output after process termination or an interrupt signal. That interval turns out to be important for many testing applications (such as the Go Playground implementation and the cmd/go test suite). It also generalizes nicely to the use-cases in #23019: setting WaitDelay without Context provides bounded I/O wait times without sending a preceding signal.
Compatibility
I believe that this proposal is entirely backward-compatible (in contrast with #23019). The zero-values for the new fields provide exactly the same behavior as a Cmd returned by exec.Command or exec.CommandContext today.
Caveats
This proposal does not address graceful shutdown on Windows (#22757 (comment); CC @mvdan). However, it may be possible to extend it to do so by providing special-case Windows behavior when the Interrupt field is set to os.Interrupt, or by adding an InterruptFunc func(*Cmd) callback that would also be invoked when Context is done.
The proposed API also does not provide a mechanism to send an Interrupt signal followed by os.Kill after a delay but still wait for subprocesses to close all I/O pipes. I believe the use-cases for that scenario are sufficiently niche to be provided only by third-party libraries: sending SIGKILL to the parent process makes it likely that subprocesses will not know to shut down, so in the vast majority of cases users should either not send SIGKILL at all (WaitDelay == 0), forcibly terminate the pipes to try to kill the subprocesses with SIGPIPE (WaitDelay > 0), or do something platform-specific to try to forcibly shut down an entire process group (outside the scope of this proposal).
Alternatives considered
In #31774 (comment), @bradfitz suggested a field Kill func(*os.Process), which would presumably be added instead of the Interrupt field in this proposal. However, I believe that such a field would be simultaneously too complex and not powerful enough:
-
The
Killfield would be too complex for most Unix applications, which overwhelmingly only need to send one ofSIGTERM,SIGINT,SIGQUIT, orSIGKILL— why pass a whole callback when you really just want to say which signal you need? -
A
*os.Processcallback would still not be powerful enough for Windows applications. If I understand the discussion in os: Interrupt is not sendable on Windows #6720 correctly (CC @alexbrainman),CTRL_BREAK_EVENTis sent to an entire process group, not a single*os.Process, so Windows users would also need a mechanism for creating (or determining) such a group, or some completely separate out-of-band way to request that the process terminate (such as by sending it a particular input or IPC message).
Given the above, the Interrupt field seems more ergonomic: it gives the right behavior for Unix users, and if Windows users want to do something more complex they can set Interrupt to nil and start a separate goroutine in between the calls to (*Cmd).Start and (*Cmd).Wait to implement whatever custom logic they want.
Activity
gopherbot commentedon Jan 4, 2022
Change https://golang.org/cl/373005 mentions this issue:
internal/moreexec: add a utility package for manipulating os/exec.Cmdseebs commentedon Jan 4, 2022
I think this may be innately race-prone in that theoretically, the process can terminate and be replaced by a new process with that PID between the check and the kill. I have vague recollections of someone having told me there's a fancy new feature in some Linuxes that could bypass this, and it's admittedly a very small window for a very large amount of stuff to happen.
seebs commentedon Jan 4, 2022
I'm a bit unclear on the meaning of the exported fields. Are they required to be set prior to launching a command, or can they be changed during the lifespan of the command? What happens if I change c.Interrupt after starting a process? What happens if I write a new value into c.Context while the process is running, and then after doing that close the previous context? etc.
I'm not sure what the answers should be, I'm pretty sure that any reasonably plausible answer will be fine, but I think it should be explicit. I think I'd favor "Context is a read-only field once the command is started, Interrupt and WaitDelay are both writeable".
bcmills commentedon Jan 4, 2022
That is #13987, which seems orthogonal.
bcmills commentedon Jan 4, 2022
They are like the other (existing) exported fields of
exec.Cmd: they must be set prior toStart(or equilavent) and then not modified untilWait(or equivalent) returns.seebs commentedon Jan 4, 2022
It is the same issue as 13987, but it's a new circumstance in which we hit that which might be non-obvious to a user, who is assuming that if we are killing "the" process, we'll pick the right one.
bcmills commentedon Jan 4, 2022
That's true, but applies equally to all of the (numerous) workarounds that build the same behavior on top of
os/exec.Cmdtoday. (I don't think this proposal makes that race any worse for anybody than it already is.)ianlancetaylor commentedon Jan 4, 2022
The Go standard library already uses that feature (the
waitidorwait6system call with an option ofWNOWAIT) on Linux, DragonFly, and FreeBSD. (For a while we usedwaitidon macOS but stopped doing so due to #19314.)mvdan commentedon Jan 4, 2022
I share this concern enthusiastically. Depending on what program you're executing, and in what environment and context, you may need very different timeouts. I also agree that we should let the user decide. As much as I would love for Go to just "do the right thing" without having to extend the API, I don't think we can do that without introducing footguns.
I had originally given #23019 a thumbs up, but I've moved my thumbs up here now instead :) I even think #23019 might qualify as a breaking change, given how it may break some unlucky programs where the timeout may lead to losing some output.
bcmills commentedon Jan 4, 2022
Thinking about this some more, there is one other interesting caveat: error behavior.
If we stop reading the program's output before EOF, it may be useful for the caller to know that. Stopping output before EOF seems orthogonal to the exit status of the program, which is what the
Waitand similar methods currently report.Should setting
WaitDelaycauseWaitand similar methods to return a different error if end up forcibly closing I/O pipes?henvic commentedon Jan 4, 2022
The Interrupt field seems super useful to me.
However, IMHO adding the Context field to simplify the documentation might end up hurting more than helping as now I've to understand why there are two ways to pass context. Especially after reading this:
henvic commentedon Jan 4, 2022
Someone might ask themselves: is it only if I pass Context as a field or also when using CommandContext?
bcmills commentedon Jan 5, 2022
Note that without the Context field, we would still have to explain that
Interruptcan only be used (or only has an effect) withCmdinstances returned byCommandContext. So I'm not sure that removing it would help with that confusion much. 😅Probably we could update the
CommandContextdoc comment to explicitly state that it sets theContextandInterruptfields?96 remaining items
doc/go1.20: add a release note for os/exec API changes
gopherbot commentedon Nov 17, 2022
Change https://go.dev/cl/377835 mentions this issue:
internal/testenv: add a Command function that replaces exec.Commandinternal/testenv: add a Command function that replaces exec.Command
gopherbot commentedon Dec 7, 2022
Change https://go.dev/cl/456116 mentions this issue:
cmd/go: use Cancel and WaitDelay to terminate test subprocessesgopherbot commentedon Jan 10, 2023
Change https://go.dev/cl/460997 mentions this issue:
cmd/go/internal/vcweb: simplify hgHandler cancellationgopherbot commentedon Jan 10, 2023
Change https://go.dev/cl/460998 mentions this issue:
os/exec: avoid leaking an exec.Cmd in TestWaitInterruptos/exec: avoid leaking an exec.Cmd in TestWaitInterrupt
cmd/go: use Cancel and WaitDelay to terminate test subprocesses
cmd/go/internal/vcweb: simplify hgHandler cancellation
Use Command.WaitDelay to avoid Command.Wait blocking on stdin/out/err
fix(servstate): use WaitDelay to avoid Command.Wait blocking on stdin…
os/exec: add the Cancel and WaitDelay fields