How to correctly use exec.Command and properly clean up all pipes

[tomas...@showmax.com]

Hello,

I need to spawn a subprocess and I'm trying to use os/exec. I need to capture
stdout/err so I'm using Cmd.StdoutPipe and Cmd.StderrPipe . The problem I'm
having is how to correctly clean up the pipes after I am done. If Cmd.Start
works fine, all I need to do is read everything and then call Cmd.Wait, which
states that it will clean up ( https://golang.org/pkg/os/exec/#Cmd.Wait ):

> Wait releases any resources associated with the Cmd.

However, what I am not sure about is how to handle cases when Cmd.Start returns
an error. Nowhere in its documentation it's said that it cleans up the
resources, so originally I thought I have to do the cleanup myself. However,
after checking the source code it does seem to close the pipes when it fails:

https://golang.org/src/os/exec/exec.go?s=11462:11489#L374

And now I'm not sure what to do. io.Closer states that double-close is no-no:

> The behavior of Close after the first call is undefined.

so I cannot even close them myself (since they will be already closed). It seems
I have to rely on undocumented behaviour, but those have tendency to change and
I don't want to start leaking file descriptors after patch version update... I
assume there must be a way to solve this using just documented behaviour. Can
someone point me in a right direction?



Maybe to better show my problem, here it's in pseudo code (error handling
omitted):

        cmd := exec.Command(name, args...)
        stdout, _ := cmd.StdoutPipe()
        stderr, _ := cmd.StderrPipe()
        if err := cmd.Start(); err != nil {
                // --> How should I close stdout/stderr here to future-proof for
                //     changes in cmd.Start behavior?
                return err
        }
        consumeAll(stdout)
        consumeAll(stderr)
        cmd.Wait()
        return nil



Thank you,
Tomas Volf

[Ian Lance Taylor]

If Start returns an error, the read and write ends of the pipes will
be closed just as though Wait returned. You don't have to close
anything yourself if Start fails. I agree that this should be
documented.

Ian

[Lucas Christian]

Apologies for reviving an old thread, but thought this might be the most efficient way to keep context.

I'm looking at a similar issue Thomas encountered, but in this case I'm concerned about how to handle errors returned from StdoutPipe()/StderrPipe() and properly clean up after that. e.g.:

    cmd := exec.Command(myCommand, myArgs...)

    stdout, err := cmd.StdoutPipe()
    if err != nil {
        // log warning or bail out
    }

    stderr, err := cmd.StderrPipe()
    if err != nil {
        // log warning, if we bail out we will leak the os.Pipe allocated internally in cmd.StdoutPipe()

    }

    err = cmd.Start() // if this fails, it *will* clean up both pipes

Looking at the source, both StdoutPipe() and StderrPipe() allocate os.Pipe()s internally. Since these methods return the reader I can technically close that one myself but the writer side of the pipe will presumably leak if cmd.Start() is never called. In my usecase it's probably an acceptable compromise to just complain to the log and move along with starting the process, but if my understanding is correct it feels like an omission in the language spec that there is seemingly no way to clean up a the pipes associated with a command that is never started.

Thanks,

Lucas

[Ian Lance Taylor]

On Fri, Feb 3, 2023 at 12:38 PM 'Lucas Christian' via golang-nuts
<golan...@googlegroups.com> wrote:
>
> Apologies for reviving an old thread, but thought this might be the most efficient way to keep context.
>
> I'm looking at a similar issue Thomas encountered, but in this case I'm concerned about how to handle errors returned from StdoutPipe()/StderrPipe() and properly clean up after that. e.g.:
>
> cmd := exec.Command(myCommand, myArgs...)
>
> stdout, err := cmd.StdoutPipe()
> if err != nil {
> // log warning or bail out
> }
>
> stderr, err := cmd.StderrPipe()
> if err != nil {
> // log warning, if we bail out we will leak the os.Pipe allocated internally in cmd.StdoutPipe()
> }
>
> err = cmd.Start() // if this fails, it *will* clean up both pipes
>
> Looking at the source, both StdoutPipe() and StderrPipe() allocate os.Pipe()s internally. Since these methods return the reader I can technically close that one myself but the writer side of the pipe will presumably leak if cmd.Start() is never called. In my usecase it's probably an acceptable compromise to just complain to the log and move along with starting the process, but if my understanding is correct it feels like an omission in the language spec that there is seemingly no way to clean up a the pipes associated with a command that is never started.

As a practical matter I wouldn't worry about it, as StdoutPipe and
StderrPipe should approximately never fail. I think that the only way
they could fail would be if you hit the limit on the total number of
open file descriptors.

If they do manage to fail, it's a fair point that there is no
supported way to close any earlier pipes. Again it's unlikely to be a
practical problem as any lingering file descriptors will get closed by
the finalizer associated with each file descriptor. But, yes, it
looks like a bit of a hole in the API.

Ian

[Lucas Christian]

Thanks for the reply and confirmation—theoretical but we have had systems fail this way before (including the system this codebase will eventually be deployed on). Of course practically once that happens, the go process leaking one more file descriptor is probably the least of our worries.

For completeness is there an issue tracker it would be worth filing this off to?

--

Lucas Christian  

Staff Software Engineer, Voice and SIP Connectivity

[Ian Lance Taylor]

On Fri, Feb 3, 2023 at 1:48 PM Lucas Christian <lchri...@twilio.com> wrote:
>
> Thanks for the reply and confirmation—theoretical but we have had systems fail this way before (including the system this codebase will eventually be deployed on). Of course practically once that happens, the go process leaking one more file descriptor is probably the least of our worries.
>
> For completeness is there an issue tracker it would be worth filing this off to?

https://go.dev/issue (which redirects to GitHub).

Ian

[Lucas Christian]

Filed under https://github.com/golang/go/issues/58369.

--

Lucas Christian  

Staff Software Engineer, Voice and SIP Connectivity

On Fri, Feb 3, 2023 at 2:14 PM Ian Lance Taylor <ia...@golang.org> wrote:

On Fri, Feb 3, 2023 at 1:48 PM Lucas Christian <lchri...@twilio.com> wrote:
>
> Thanks for the reply and confirmation—theoretical but we have had systems fail this way before (including the system this codebase will eventually be deployed on). Of course practically once that happens, the go process leaking one more file descriptor is probably the least of our worries.
>
> For completeness is there an issue tracker it would be worth filing this off to?

https://urldefense.com/v3/__https://go.dev/issue__;!!NCc8flgU!aclVyNiB3jXq50nLgI3WT74VmhVQaslq5Q_idqxqQ3QGe2HY-8Bx2A1dkZvNyGTYBD6wvTQF3tG0ng$  (which redirects to GitHub).

[Rich]

Not sure if this helps at all, but I've stopped using Go's exec and use bitfield's script command. You can do so much more with it than you can with the standard exec and it's shorter to implement than Go's exec.

https://github.com/bitfield/script