os/exec: CommandContext documentation doesn't say that context is ignored until Wait

[glasser]

The new CommandContext function in os/exec is documented as:

// CommandContext is like Command but includes a context.
//
// The provided context is used to kill the process (by calling
// os.Process.Kill) if the context becomes done before the command
// completes on its own.

I was surprised to learn that the provided context is entirely ignored until Wait is called (perhaps implicitly via Run). I think that's a reasonable implementation choice (without calling c.Process.Wait, it's hard to know if the process is still around to send a signal to), but it is not supported by the documentation for CommandContext or Wait.

(I could also imagine that Start should check the context for Done-ness before starting the process, like net.DialContext does, but the documentation is consistent with the implementation here.)

[bradfitz]

I agree that it'd be nice if CommandContext + cmd.Start worked as expected and the process got killed if needed.

And I agree we should probably check the Done-ness before starting the process.

@ianlancetaylor, you worked on this while I was gone. Thoughts? Maybe if there's a context, then Start immediately starts a goroutine to do wait, and the exported Wait just waits for that goroutine?

[glasser]

Actually, given that the "idiomatic usage" of StdoutPipe/StderrPipe is to not call Wait until after you're doing reading from the pipe, I'd say that the current behavior is just buggy. Hopefully this isn't too late to adjust before 1.7...

It does look to me like a goroutine calling c.Process.Wait() in Start and passing the value to c.Wait() via would work.

[ianlancetaylor]

@bradfitz I didn't do anything in the os/exec package. What I did was fix up the os package so that there is no race between Process.Signal and Process.Wait on GNU/Linux, and then mikioh extended that to Darwin and FreeBSD.

Maybe I'm missing something but I don't see why the goroutine started by Start should call wait. Seems to me it should just be like the one currently started by Wait:

        go func() {
            select {
            case <-c.ctx.Done():
                c.Process.Kill()
            case <-c.waitDone:
            }
        }()

and then Wait closes c.waitDone after c.Process.Wait returns.

[bradfitz]

@ianlancetaylor, the problem with the new context support in os/exec is that it doesn't do anything if you only use Start instead of Start+Wait or Run. And as @glasser pointed out, when reading the output of a program via a pipe, you only call Start & do blocking reads and only call Wait later. All that time, your context finishing does nothing.

I think Start needs to start a goroutine monitoring the context and killing the process if needed.

[ianlancetaylor]

@bradfitz I think you are saying the same thing that I am saying.

[ianlancetaylor]

https://golang.org/cl/24650

[gopherbot]

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