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.

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

os/exec: documentation unclear on whether (*Cmd).Wait must be called #52580

Open
bcmills opened this issue Apr 26, 2022 · 4 comments
Open

os/exec: documentation unclear on whether (*Cmd).Wait must be called #52580

bcmills opened this issue Apr 26, 2022 · 4 comments
Assignees
Labels
Documentation NeedsFix
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 26, 2022

On POSIX platforms, a C program that uses fork to spawn a child process must subsequently call wait or similar to reap any resulting zombies. However, the call to wait is not needed if the process explicitly sets the handler for SIGCHLD to SIG_IGN or sets the SA_NOCLDWAIT flag on that handler.

On those same platforms, Go's os/exec package uses fork under the hood (via os.StartProcess, syscall.StartProcess, and ultimately syscall.forkExec). However, (*exec.Cmd).Start doesn't document whether Wait must actually be called. The documentation today says:

The Wait method will return the exit code and release associated resources once the command exits.

That seems to imply that Wait must be called in order to release those resources, but that isn't entirely obvious to me — given the finalizers that the standard library already uses for os.File, one might assume that the child process does not need to be explicitly reaped if the caller doesn't care about the exit code and hasn't allocated explicit resources that would need to be released (for example, because they set Stdin, Stdout, and/or Stderr to either nil or instances of *os.File).

I think that (*exec.Cmd).Start should either clearly state that Wait must always be called (and perhaps diagnose violations with a finalizer), or avoid unbounded resource leaks if Wait is not called.

I think the latter is fairly easy to implement at the cost of one extra goroutine per Cmd (by moving the Wait call into an eager goroutine), and that would also enable a clean fix for #28461 (#15103 notwithstanding).

@ianlancetaylor, @bradfitz: does that seem like a reasonable resolution?

@bcmills bcmills added NeedsDecision and removed Documentation labels Apr 26, 2022
@bcmills bcmills added this to the Go1.19 milestone Apr 26, 2022
@bcmills bcmills self-assigned this Apr 26, 2022
@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 26, 2022

I will note that at the moment, (*exec.Cmd) instances allocated via exec.Command do not leak goroutines by default, while those allocated via exec.CommandContext do.

(Compare https://go.dev/play/p/mNn6W6J9iDZ vs. https://go.dev/play/p/OQc3BPJfSBQ.)

@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 26, 2022

(This was noticed while prototyping an in-tree implementation of #50436.)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 26, 2022

The docs for the Wait method also say "Wait releases any resources associated with the Cmd." I think it would be fine to explicitly require callers to call Wait themselves, especially since that is how it already works.

My main concern with a simple approach to automatically waiting is that it uses up more than a goroutine, it uses up a thread. I don't think we want to burn a thread per child process automatically and unnecessarily. We could in principle use a SIGCHLD handler to minimize threads, but I'm not sure it's worth the potential collisions with C code--upon receiving a SIGCHLD we would have to check the status of only subprocesses started by os/exec, to avoid confusion with C code waiting for subprocesses started by C code.

@magical
Copy link
Contributor

@magical magical commented Apr 29, 2022

Just chiming into say that i had this exact question recently. More guidance from the docs would certainly be appreciated 😄

In my case, i was reading output from a command. When i was done with the portion i was interested in, i didn't care what happened to the process, just that it went away. Ended up deferring Process.Kill followed by Wait. I don't know if the Wait was really necessary but i figured it couldn't hurt.

cmd := exec.Command("ndisasm", "...")
pipe, err := cmd.StdoutPipe()
if err != nil {
        t.Fatal(err)
}
defer func() { cmd.Process.Kill(); cmd.Wait() }()
defer pipe.Close()
cmd.Start()

@bcmills bcmills added NeedsFix Documentation labels Apr 30, 2022
@gopherbot gopherbot removed the NeedsDecision label Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix
Projects
None yet
Development

No branches or pull requests

4 participants