os/exec: CommandContext should also cause the stdout/stderr pipes to be closed

[nhooyr]

I'm trying to run a process and kill it if it takes too long. So I create a command with a timeout context. Then, when the context expires and the process is not done, it will be killed. This approach works fine unless my Go process is reading from the process's stdout/stderr and the process forks and gives its stdout to its child. Then, cmd.Wait() will not exit until the child is done writing to stdout. I think that is unexpected because once the context expires, cmd.Wait() should unblock.

Here is a minimal example that reproduces my issue:

package main

import (
	"bytes"
	"context"
	"log"
	"os/exec"
	"time"
)

func main() {
	log.SetFlags(0)

	ctx, cancel := context.WithTimeout(context.Background(), time.Second)

	cmd := exec.CommandContext(ctx, "bash", "-c", "echo -n foo; sleep 5")
	stdout := &bytes.Buffer{}
	cmd.Stdout = stdout

	start := time.Now()

	err := cmd.Start()
	must(err)

	err = cmd.Wait()
	cancel()

	log.Println("took:", time.Since(start))
	log.Println("expected it to take:", time.Second)
	log.Println("stdout:", stdout)
	log.Println("wait error:", err)
}

func must(err error) {
	if err != nil {
		panic(err)
	}
}

So what I'm proposing is that the stdout/stderr pipes should be closed once the process dies and the context has expired. This will allow the copying goroutines to exit and cmd.Wait() to unblock after the context expires as expected.

[nhooyr]

There is a similar problem with setting the stdin of the command as then Wait() will block until one of the started goroutines is done copying from the stdin reader. This isn't something that can be fixed without breaking the Go API promise though. But I'm not sure how many instances there are of a stdin reader that also cannot be closed concurrently.

[rsc]

In general, cancelling a context is a "please" request: please stop what you're doing, because I don't care. You don't know that the operation is done until the usual operation returns, either with a "I stopped because you said please stop" error or some other result, and the calling code can react accordingly. It's not at all clear that forcibly ending stdout/stderr, making reporting that result impossible, is the right answer here.

exec.CommandContext makes things a bit unclear already because it sends SIGKILL, but it might be best not to overload it further.

There's also the more general problem that the child can exit and the wait system call can return but the copying of stdout/stderr hangs because of grandchildren holding the pipe open (can't find in issue tracker right now but I thought it existed).

Note that if you are having this problem you can change your exec.Command to hand it an os.Pipe directly and then choose to stop reading when cmd.Wait returns. In fact, Go 1.10 will support SetReadDeadline on os.Pipes, so you can SetReadDeadline(0) on the read end once cmd.Wait returns.

But further special semantics in CommandContext seems not right.

[nhooyr]

I would rather the stdout/stderr be forcefully closed instead of hanging a goroutine in my program. I cannot see any circumstances where this would not be the expected behaviour, especially considering CommandContext sends a SIGKILL.

If this is not something you want to change in CommandContext, I propose we at least add docs explaining and warning of this behaviour. If that is fine with you, I can whip up the patch.

[ianlancetaylor]

@nhooyr Doc patches are always welcome.

[nhooyr]

Sounds good. Just waiting to see what @rsc wants to do.