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

Closed
bcmills opened this issue Apr 26, 2022 · 8 comments
Closed

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

bcmills opened this issue Apr 26, 2022 · 8 comments
Assignees
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

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 Feedback is required from experts, contributors, and/or the community before a change can be made. 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
Contributor Author

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
Contributor Author

bcmills commented Apr 26, 2022

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

@ianlancetaylor
Copy link
Contributor

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 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 The path to resolution is known, but the work has not been done. Documentation labels Apr 30, 2022
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 30, 2022
@bcmills bcmills added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label May 31, 2022
@gopherbot gopherbot removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 10, 2022
@gopherbot
Copy link

Change https://go.dev/cl/414056 mentions this issue: os/exec: clarify that Wait must be called

jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
Fixes golang#52580

Change-Id: Ib2dd8a793b9c6fcb083abb3f7c346f6279adefc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/414056
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/436655 mentions this issue: os/exec: add a GODEBUG setting to diagnose leaked processes

@gopherbot
Copy link

Change https://go.dev/cl/436656 mentions this issue: os/exec: avoid leaking a process in TestDoubleStartLeavesPipesOpen

gopherbot pushed a commit that referenced this issue Sep 29, 2022
Updates #52580.
For #50436.

Change-Id: I0929055ffca1ca429f6ebec7d877f4268bd1fda2
Reviewed-on: https://go-review.googlesource.com/c/go/+/436656
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
gopherbot pushed a commit that referenced this issue Oct 4, 2022
Updates #52580.
For #50436.

Change-Id: I669f13863f1f85d576c3c94500b118e6989000eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/436655
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/460998 mentions this issue: os/exec: avoid leaking an exec.Cmd in TestWaitInterrupt

gopherbot pushed a commit that referenced this issue Jan 10, 2023
In CL 436655 I added a GODEBUG setting to this test process to verify
that Wait is eventually called for every exec.Cmd before it becomes
unreachable. However, the cmdHang test helpers in
TestWaitInterrupt/Exit-hang and TestWaitInterrupt/SIGKILL-hang
intentially leak a subprocess in order to simulate a leaky third-party
program, as Go users might encounter in practical use.

To avoid tripping over the leak check, we call Wait on the leaked
subprocess in a background goroutine. Since we expect the process
running cmdHang to exit before its subprocess does, the call to Wait
should have no effect beyond suppressing the leak check.

Fixes #57596.
Updates #52580.
Updates #50436.

Change-Id: Ia4b88ea47fc6b605c27ca6d9d7669c874867a900
Reviewed-on: https://go-review.googlesource.com/c/go/+/460998
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

4 participants