os/exec: stop stdin copying goroutine after process exits

[gopherbot]

by robryk:

When Cmd.Stdin is not a file, Cmd.Wait waits until a read on it unblocks. When Cmd.Stdin
is a file, Cmd.Wait returns immediately after the command being executed completes. I
find it confusing that semantics of Wait depend on the kind of Reader passed as standard
input.

What does 'go version' print?
go version devel +96cbc91bc15a Sat Apr 26 22:31:32 2014 -0700 + linux/amd64

What steps reproduce the problem?
If possible, include a link to a program on play.golang.org.

1. Run http://play.golang.org/p/eADuSYGZ1C. Notice that it exits
   immediately.
2. Run http://play.golang.org/p/hh1eXk9S0E. Notice that it waits
   (in Cmd.Wait) for EOF on stdin.

What happened?

These two programs differ in behaviour.

What should have happened instead?

These two programs should behave identically, because the Reader passed
to Cmd.Stdin behaves identically in both cases.

Please provide any additional information below.

I find the behaviour for non-files troublesome to work with, although it seems to
conform better to the documentation. In my usecase, I need to close the stdin Reader
after the process has exited (the Reader can have a user on the other end, so I can't
predict that the input has ended). Alas, I can't use Wait() for that purpose, because
Wait() waits until my reader unblocks, which (assuming no input is forthcoming) will
happen only after it is closed.

[ianlancetaylor]

Comment 1:

The issue here is that when c.Stdin is set to an io.Reader that is not an os.File, a
goroutine is created to copy data from the io.Reader to a freshly create os.Pipe.  When
the process exits, and we've waited for it, Cmd.Wait waits for that goroutine to
complete.  But there is really no reason to do that.  The process is done; it's not
going to read any more data.  We shouldn't sit around waiting for the goroutine to copy
data to a process that isn't going to read it.  We should instead just tell the
goroutine to shut down.

Labels changed: added repo-main, release-go1.4.

[gopherbot]

Comment 2 by robryk:

There is a problem with making that change: If we were to change tis, the following
example would become a data race: http://play.golang.org/p/bODUysTV1l (the copying
goroutine and main() would access mr concurrently).
At the same time I find it weird that this example returns the broken pipe error from
Run(). There are two weird things I see here:
This is another difference in behaviour between *os.File and other io.Readers
This makes it largely impossible to use Cmd.Stdin directly (ie. not by calling
StdinPipe) if we don't want to treat an early exit from the child process as an error.

[ianlancetaylor]

Comment 3:

I agree that it would be incorrect to permit Run to return while the copying goroutine
is still running.  I wasn't suggesting that.
I'm sorry, I don't understand the comment about the broken pipe error.  It's clearly
possible to use Cmd.Stdin directly if you set it to a file or an os.Pipe.  Perhaps it is
difficult to use it for a general io.Reader, I'm not sure.

[gopherbot]

Comment 4 by robryk:

> I'm sorry, I don't understand the comment about the broken pipe error.  It's clearly
possible to use Cmd.Stdin directly if you set it to a file or an os.Pipe.  Perhaps it is
difficult to use it for a general io.Reader, I'm not sure.
The difficulty arises if you set it to another io.Reader and you don't expect the child
process to read all its input. In that case, Run (or Wait) will return a broken pipe
error if the child didn't read all its input (http://play.golang.org/p/bODUysTV1l
demonstrates that). It's difficult to distinguish between that error and any other error
(eg. the process having died due to a signal).
It seems to me now that one should always use StdinPipe in those situations. If that's
true, I will send a doc CL that makes it explicit after Go 1.3 is out.

[adg]

Comment 5:

I guess using StdinPipe in those situations allows you to separate the pipe error from
any Wait error.
Another approach would be to have Wait return a specific error type in this case that
the user may test for.

[robpike]

Comment 6:

Owner changed to @bradfitz.

Status changed to Accepted.

[rsc]

Comment 7:

I think the current behavior is as good as can be done, given the abstractions and the
constraints. I will send a CL making the docs clear.

[gopherbot]

Comment 8:

CL https://golang.org/cl/156220043 mentions this issue.

[rsc]

Comment 9:

Re #1 there is no way to "tell the goroutine to shut down". It might be blocked in a
Read indefinitely.

[rsc]

Comment 10:

This issue was closed by revision 05c4b69.

Status changed to Fixed.