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

x/playground: child processes of the "present" tool not killed on terminate #8854

Closed
karalabe opened this issue Oct 2, 2014 · 5 comments
Closed

Comments

@karalabe
Copy link
Contributor

karalabe commented Oct 2, 2014

In the online presentations (through the "present" tool), if the executing
code snippet starts up a child process (see end of report for a trivial example), the
child process will remain running in the background even if the parent (original
process) is killed or terminates gracefully.

This is especially an issue if a user kills a running snippet, which will leave
stale/dangling processes running on the host machine.

---
E.g.

package main

import "os/exec"

func main() {
    cmd := exec.Command("sleep", "100")
    cmd.Start()
}

If you run this inside present, and kill the process, the sleep will happily run in the
background until it finishes.
@karalabe
Copy link
Contributor Author

karalabe commented Oct 2, 2014

Comment 1:

After thinking about it a bit and digging around, I can see that this is a more deeply
rooted issue with process termination is general.
However, imho even a hackish solution would be better than the current state of dangling
processes in the case of demo snippets, since they will probably not contain
sophisticated parent/child teardown logic (which would be the de facto solution to this).

@karalabe
Copy link
Contributor Author

karalabe commented Oct 2, 2014

Comment 2:

On Linux it's solvable by a small hack:
Instead of starting a snippet via [1]:
    cmd := exec.Command(args[0], args[1:]...)
a new process group / session group can be created for it:
    cmd := exec.Command("setsid", args...)
And then in the kill, instead of [2]:
    p.run.Process.Kill()
the whole process group can be killed:
    syscall.Kill(-p.run.Process.Pid, syscall.SIGKILL)
But I've got no clue how to do these in Windows or if it works in OSX. Windows has
setsid WIN32 API calls, but as far as I know not command, so maybe it would require a
slightly more involved fix of creating an extra command for present to run with setsid.
But I guess this report is good as a starting point.
Refs:
  [1] https://code.google.com/p/go/source/browse/playground/socket/socket.go?repo=tools#325
  [2] https://code.google.com/p/go/source/browse/playground/socket/socket.go?repo=tools#201

@karalabe
Copy link
Contributor Author

karalabe commented Oct 2, 2014

Comment 3:

Yeah, and to make it complete, the same syscall.Kill is needed in process.end too in
case the snippet terminates gracefully and not killed by force.

@ianlancetaylor
Copy link
Contributor

Comment 4:

Labels changed: added repo-tools, release-none.

@bradfitz bradfitz removed the new label Dec 18, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed the release-none label Apr 10, 2015
@rsc rsc changed the title go.tools/playground: Child processes not killed on terminate x/tools/playground: Child processes not killed on terminate Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@ysmolski ysmolski changed the title x/tools/playground: Child processes not killed on terminate x/playground: child processes of the "present" tool not killed on terminate May 12, 2018
@ysmolski
Copy link
Member

I am confused about this bug. This does not have to be solved in playground code because playground already kills processes that timed out. Anyway, I'm closing it unless somebody can show me the problem to solve.

@golang golang locked and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants