Skip to content

os/exec: add example showing use of pipe #4290

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.

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

Closed
rogpeppe opened this issue Oct 26, 2012 · 12 comments
Closed

os/exec: add example showing use of pipe #4290

rogpeppe opened this issue Oct 26, 2012 · 12 comments
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@rogpeppe
Copy link
Contributor

Cmd.StdoutPipe, Cmd.StdinPipe, Cmd.StderrPipe all close the client end of the pipe when
Wait returns. This is problematic when there's an independent goroutine that uses the
client end of the pipe. For example, this code can
fail to copy all the data from a command.

    c := exec.Command("some-command")
    r, err := c.StdoutPipe()
    if err != nil {...}
    go io.Copy(dest, r)
    c.Run()

Since all these methods return io.*Closer types, it seems reasonable to me that the
caller should have the responsibility for closing them.
@bradfitz
Copy link
Contributor

Comment 1:

Why did we make StderrPipe and StdoutPipe return io.ReadCloser instead of io.Reader?  I
think we at least need a doc clarification.
Our docs say "The pipe will be closed automatically after Wait sees the command exit.",
but that I assume is referring to the server's side, so the client reads an EOF.

@rogpeppe
Copy link
Contributor Author

Comment 2:

> Our docs say "The pipe will be closed automatically after Wait sees the command 
> exit.", but that I assume is referring to the server's side, so the client reads an 
> EOF.
I had assumed this too, but it actually can't close the server side of the pipe, because
the process has a handle to it and there's no guarantee that the process
hasn't started another process in the background that holds it open.
I think it's correct that StderrPipe and StdoutPipe return io.ReadCloser, but I think
it's wrong that Wait closes the client side of the pipe.
FWIW I'm submitting this issue because of an actual test failure we've seen. For the
time being I've stoppped using StdoutPipe, but I'd like to fix it properly.
I'm wondering if it might be a good idea to remove closeAfterWait entirely. If we're not
going to close the client side of the pipe, then ISTM that all the other current use
cases can be catered for by doing the close inside the started goroutine.

@dsymonds
Copy link
Contributor

Comment 3:

Labels changed: added priority-soon, packagebug, removed priority-triage.

Status changed to Started.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 4:

Can someone summarize the current status of this issue?
dsymonds marked it as started a month ago, but it's owned by rogpeppe, and there are no
linked CLs.
Thanks.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 5:

Labels changed: added size-m.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 6:

Labels changed: added suggested.

@rogpeppe
Copy link
Contributor Author

Comment 7:

My fault. I made a CL to fix this (http://golang.org/cl/6789043/) but the
consensus was that we couldn't fix the issue because of Go 1 issues. I said I'd update
the docs, but I have not done so yet.

@rsc
Copy link
Contributor

rsc commented Jan 31, 2013

Comment 8:

Labels changed: added priority-later, removed priority-soon.

@rsc
Copy link
Contributor

rsc commented Mar 11, 2013

Comment 9:

The example should be:
call StdoutPipe
call Start
Read until EOF
call Wait
without any goroutines.

Status changed to Accepted.

@adg
Copy link
Contributor

adg commented Mar 12, 2013

Comment 10:

There is already a ExampleCmd_StdoutPipe that does exactly that.

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 11:

Labels changed: added go1.1maybe, removed go1.1.

@rsc
Copy link
Contributor

rsc commented Mar 13, 2013

Comment 12:

Status changed to Fixed.

@rogpeppe rogpeppe added fixed Suggested Issues that may be good for new contributors looking for work to do. labels Mar 13, 2013
@rsc rsc added this to the Go1.1 milestone Apr 14, 2015
@rsc rsc removed the go1.1maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

6 participants