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

Add ability to kill subprocess when parent process exits #10

Closed
josegonzalez opened this issue Apr 16, 2022 · 5 comments · Fixed by #14
Closed

Add ability to kill subprocess when parent process exits #10

josegonzalez opened this issue Apr 16, 2022 · 5 comments · Fixed by #14

Comments

@josegonzalez
Copy link

josegonzalez commented Apr 16, 2022

The cmd object is not exposed in any way, so there is no way to call cmd.Process.Kill() out of band if the parent process exits early.

Expected Behaviour

I expect to be able to kill the process, or at least have it automatically exit on it's own with a non-zero exit code.

Current Behaviour

The process is currently orphaned and continues running to completion.

Possible Solution

Not sure, maybe an option like ListenToSignals (default false) that will listen to process signals and pass them along to the child. This would allow ctrl+c and the like to work properly (not sure about SIGKILL though).

Steps to Reproduce (for bugs)

Use the following in a bin and issue a ctrl+c on the executing terminal. docker ps should still show the container.

package main

import (
	"fmt"

	execute "github.com/alexellis/go-execute/pkg/v1"
)

func main() {
	cmd := execute.ExecTask{
		Args: []string{
			"container",
			"run",
			"--rm",
			"ubuntu",
			"/bin/bash",
			"-c",
			"sleep infinity",
		},
		Command: "docker",
	}

	res, err := cmd.Execute()
	if err != nil {
		fmt.Printf("error executing command: %s", err.Error())
	}

	if res.ExitCode != 0 {
		fmt.Printf("error executing command: %s", res.Stderr)
	}
}

Why do you need this?

I'm writing a tool that wraps docker calls - well, maybe now using the docker api if it seems straightforward enough - and was waiting a long time for a command to execute anything (because the underlying tool was slow). I ran ctrl+c and then noticed the container was still up.

Your Environment

  • go version : go version go1.17.9 darwin/amd64

  • Operating System and version (e.g. Linux, Windows, MacOS): macOS Monterey 12.3.1

  • Link to your project or a code example to reproduce issue: The above code reproduces my issue (the project I'm working on isn't published yet).

@derek derek bot added the invalid This doesn't seem right label Apr 16, 2022
@derek
Copy link

derek bot commented Apr 16, 2022

Please complete the whole issue template, without deleting any headings.

@josegonzalez
Copy link
Author

Yeah this bot is wrong, I definitely filled out the entire ticket. I've updated Context to say Why do you need this? based on https://raw.githubusercontent.com/openfaas/faas/master/.DEREK.yml

@josegonzalez
Copy link
Author

May be worth looking into Github Forms. Example here (click any of the Get Started links).

@alexellis alexellis removed the invalid This doesn't seem right label Apr 17, 2022
@alexellis
Copy link
Owner

alexellis commented Apr 17, 2022

Thanks for the interest @josegonzalez and you're right, Derek is misconfigured on this repo and I've removed that label, however the so called "legacy" templates are required for reasons.

Are you handling signals when the process quits? Like here

Killing your container's PID1 should also kill any other child processes.

Adding a context to the command's execution may allow for it be cancelled later.

Alex

alexellis added a commit that referenced this issue Apr 17, 2022
ExecuteWithContext allows a user to pass in a context with
a timeout, or a cancellation that can be used to terminate
the process.

Fixes: #10 #9

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis added a commit that referenced this issue Apr 17, 2022
ExecuteWithContext allows a user to pass in a context with
a timeout, or a cancellation that can be used to terminate
the process.

Fixes: #10 #9

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis added a commit that referenced this issue Apr 17, 2022
ExecuteWithContext allows a user to pass in a context with
a timeout, or a cancellation that can be used to terminate
the process.

Fixes: #10 #9

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis added a commit that referenced this issue Apr 17, 2022
ExecuteWithContext allows a user to pass in a context with
a timeout, or a cancellation that can be used to terminate
the process.

Fixes: #10 #9

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis added a commit that referenced this issue Apr 17, 2022
Setting Context on ExecTask allows a user to control when
the process completes through a cancellation or a timeout.

Tested through additional unit tests, examples added to
README.

Fixes: #10 #9

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
rgee0 added a commit to rgee0/derek that referenced this issue Apr 17, 2022
A recent issue (alexellis/go-execute#10)
indirectly highlighted that the merging of local and remote configs
was producing a superset which included duplicate headings.  The
intended mode of operation for the Issue template headings was
for the local to over-ride the remote.  This is diffent to the other
fields, such as maintainers.

This change adds the test to highlight the issue and addresses the
issue in MergeDerekRepoConfigs.

Signed-off-by: Richard Gee <richard@technologee.co.uk>
@josegonzalez
Copy link
Author

Not sure how I'd handle signals when we don't have direct access to the underlying cmd, but your MR might work for my use case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants