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

mage: cancel context on SIGINT #313

Merged
merged 6 commits into from
Nov 29, 2022

Conversation

pmcatominey
Copy link
Contributor

On receiving an interrupt signal, mage cancels the context allowing the magefile
to perform any cleanup before exiting.

A second interrupt signal will kill the magefile process without delay.

The behaviour for a timeout remains unchanged (context is canclled and the magefile
exits).

Fixes #269, #266.

On receiving an interrupt signal, mage cancels the context allowing the magefile
to perform any cleanup before exiting.

A second interrupt signal will kill the magefile process without delay.

The behaviour for a timeout remains unchanged (context is canclled and the magefile
exits).
@pmcatominey
Copy link
Contributor Author

Updated to switch test from SIGINFO to SIGHUP as the former does not exist on linux/amd64.

@flowchartsman
Copy link

You may want to consider holding off on this until signal.NotifyContext lands: golang/go#37255

@natefinch
Copy link
Member

Sorry for the late review.

re: signal.NotifyContext .... I don't want mage to require a new version of Go, because I know the pain of being stuck on an older version. I think we can do fine with the knobs we have.

This PR looks pretty good, but I think we're missing a synchronization effort... we should wait for the targets to clean up, and right now there's no mechanism for that. So, like, if you put a defer in a target that takes a little time to run, it may not always have time to run before mage exits out. I think we need a waitgroup and a timeout... like, first time we get sigint, cancel the context and start waiting for people to clean up. Maybe wait up to 5 seconds. If the user hits ctrl-c again, skip the timeout and just exit.

so something like this:

$ mage runlongthing
^C cancelling mage targets, waiting up to 5 seconds for cleanup...
^C exiting mage
$ 

@pmcatominey
Copy link
Contributor Author

@natefinch thanks for the review.

I've updated the code to apply a timeout after a SIGINT and tests to cover this, aswell as testing that a deferred function call within a target will be ran during during cleanup.

I'm not sure where a WaitGroup would be useful?

@faiq
Copy link

faiq commented Nov 28, 2022

Hi can we get some movement on this PR @natefinch? This feature would be killer for us. Please let me know how I can help make it happen. Thank you

@natefinch
Copy link
Member

Thanks for pinging me on this, @faiq ... I'll have to re-review the code and work on the merge conflicts, but it seems like it should be able to be merged soon. I'll try to poke at this some tonight.

natefinch
natefinch previously approved these changes Nov 29, 2022
Copy link
Member

@natefinch natefinch left a comment

Choose a reason for hiding this comment

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

LGTM after a slight tweak.

@natefinch natefinch merged commit a920604 into magefile:master Nov 29, 2022
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 this pull request may close these issues.

Signal can't be caught by the underlying binary
4 participants