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

When SIGINT is detected by devbox run send SIGTERM to the pid of the script that is running #1825

Closed
wants to merge 4 commits into from

Conversation

kadaan
Copy link
Contributor

@kadaan kadaan commented Feb 21, 2024

Summary

When SIGINT is detected by devbox run send SIGTERM to the pid of the script that is running. Fixes #1815

How was it tested?

I ran a script before this change and CTRL-C could not cancel it. With this change it can be canceled.

@savil savil requested review from mikeland73 and gcurtis and removed request for mikeland73 February 21, 2024 00:54
@kadaan
Copy link
Contributor Author

kadaan commented Feb 21, 2024

Is there a good way to add test for these cases into the repository? If there is, I will gladly do it.

Comment on lines 37 to 39
cmd.Cancel = func() error {
return syscall.Kill(cmd.Process.Pid, syscall.SIGTERM)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using the context to cancel, you can pass along kill signals. It's a bit more involved because you need to kill subprocesses as well.

You could do something like:

// This sets up a process group id.
// It is needed in order to kill a process group (will kill any subprocesses).
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}

c := make(chan os.Signal, 1)
// Catch signals we may want to propagate. Not 100% sure about sigquit.
signal.Notify(c, syscall.SIGINT, syscall.SIGTERM, syscall.SIGQUIT)
defer func() {
	signal.Stop(c)
}()
go func() {
	select {
	case s := <-c:
		// note the negative pid. This means kill the group (process and children)
                // not sure what to do with error.
		_ = syscall.Kill(-cmd.Process.Pid, s.(syscall.Signal))
	case <-ctx.Done():
	}
}()

@@ -26,12 +28,15 @@ func RunScript(projectDir, cmdWithArgs string, env map[string]string) error {

// Try to find sh in the PATH, if not, default to a well known absolute path.
shPath := cmdutil.GetPathOrDefault("sh", "/bin/sh")
cmd := exec.Command(shPath, "-c", cmdWithArgs)
cmd := exec.CommandContext(ctx, shPath, "-c", cmdWithArgs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the background context (which this is) is not canceled when sigint/sigterm are received. So I'm not sure this works. (in fact I think the background context is never canceled)

That said, passing context here is still an improvement (in case we do want to support cancel)

When devbox runs scripts then can create a arbitrarily deep
process tree.  If you try to cancel the run script only the
top level process will receive the signal.  Depending on how
that process is constructed it may not propagate those signals
to its children.  In practice this can mean that canceling
the devbox run script will leave orphan processes running.

To solve this, when spawning the devbox run script process,
create a process group and forward all signals from the run
devbox process to the process group.
@kadaan
Copy link
Contributor Author

kadaan commented Apr 2, 2024

I made an initial stab of doing it this way and cancelling didn't work. I will try to get back to this in a bit.

@loreto loreto closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support passing SIGINT to scripts that are running via devbox run
3 participants