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

Signal can't be caught by the underlying binary #269

Closed
alon7 opened this issue Oct 20, 2019 · 6 comments · Fixed by #313
Closed

Signal can't be caught by the underlying binary #269

alon7 opened this issue Oct 20, 2019 · 6 comments · Fixed by #313

Comments

@alon7
Copy link

alon7 commented Oct 20, 2019

Hi everyone

Moved my code from makefile to mage and I'm very pleased so thank you for the great work!

I'm running another go binary which has a loop running and this code to catch signals

stopChan := make(chan os.Signal)
	signal.Notify(stopChan, syscall.SIGTERM, syscall.SIGINT, os.Interrupt, syscall.SIGQUIT, syscall.SIGHUP)

then this code a bit later:

<-stopChan
log.Info("Shutting down gracefully...")

but when i ctrl + c my mage running process the underlying process doesn't receive any signal.
I've seen it with other executables that do cleanup on terminal as well such as terraform.

Let me know if you have any idea on how to approach a fix here or maybe if I'm missing something.

Thanks!

@natefinch
Copy link
Member

Yeah, thanks for bringing this up, that's something we should make work correctly - passing a signal down from mage to the commands you're executing. It will take some work, but I think it's doable.

@alon7
Copy link
Author

alon7 commented Oct 21, 2019

Thanks for the quick response.
If you can direct me a bit I might be able to fix it over a weekend

@pmcatominey
Copy link
Contributor

Just wanted to check if anyone is working on this before opening a PR.

@flowchartsman
Copy link

flowchartsman commented Oct 23, 2020

I don't believe the linked PR addresses this issue. If I'm reading this right, it will issue a SigKill to the underlying process which doesn't seem like what this issue is talking about. For example, let's say I wanted a mage command that spins up a service with docker compose and lets it run until I'm done with it. If I issue a sigint to spin it down, it looks like #313 would send it a kill. To actually pass through the sigint, I believe you'd have to do something like the following (lifted from here):

if err := cmd.Start(); err != nil {
    log.Fatal(err) // Command not found on PATH, not executable, &c.
}
// wait for the command to finish
waitCh := make(chan error, 1)
go func() {
    waitCh <- cmd.Wait()
    close(waitCh)
}()
sigChan := make(chan os.Signal, 1)
signal.Notify(sigChan)

// You need a for loop to handle multiple signals
for {
    select {
    case sig := <-sigChan:
        if err := cmd.Process.Signal(sig); err != nil {
            // Not clear how we can hit this, but probably not
            // worth terminating the child.
            log.Print("error sending signal", sig, err)
        }
    case err := <-waitCh:
        // Subprocess exited. Get the return code, if we can
        var waitStatus syscall.WaitStatus
        if exitError, ok := err.(*exec.ExitError); ok {
            waitStatus = exitError.Sys().(syscall.WaitStatus)
            os.Exit(waitStatus.ExitStatus())
        }
        if err != nil {
            log.Fatal(err)
        }
        return
    }
}

@pmcatominey
Copy link
Contributor

Hi @flowchartsman,

My PR will allow the magefile to catch a sigint, this is covered with this test, sigkill is issued after a second sigint is received.

@Dragomir-Ivanov
Copy link

@pmcatominey Thanks for working on this. I am new to Mage and was wondering if this could help with having mage start running 2(or more) depended targets at once(run with mg.Deps), and canceling(terminate) them when issuing Ctrl-C on mage process?

My use case is having a Go project consisting of multiple micro-services in cmd/ and want to start the whole thing at once with Mage.
Thanks!

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

Successfully merging a pull request may close this issue.

5 participants