Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
nhooyr opened this issue Sep 18, 2017 · 5 comments
Closed

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

nhooyr opened this issue Sep 18, 2017 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.10

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Sep 18, 2017

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.
@ianlancetaylor ianlancetaylor changed the title exec: CommandContext should also cause the stdout/stderr pipes to be closed os/exec: CommandContext should also cause the stdout/stderr pipes to be closed Sep 18, 2017
@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 18, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Sep 18, 2017
@nhooyr
Copy link
Contributor Author

nhooyr commented Sep 19, 2017

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
Copy link
Contributor

rsc commented Oct 23, 2017

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.

@rsc rsc closed this as completed Oct 23, 2017
@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 7, 2017

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
Copy link
Contributor

@nhooyr Doc patches are always welcome.

@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 7, 2017 •
edited

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

@ianlancetaylor ianlancetaylor mentioned this issue Dec 6, 2017
Closed
@golang golang locked and limited conversation to collaborators Nov 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
4 participants
@rsc @ianlancetaylor @gopherbot @nhooyr