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 shutdown hook to destroy the process #19

Closed

Conversation

Heliosmaster
Copy link

This PR adds a shutdown hook which automatically destroys the spawned process when the caller is about to quit.

Example usage: We have a babashka script which starts a bunch of (running processes). When we Ctrl-C to quit the script, we would like the processes to terminate as well.

We added an option to pass to process to disable this behavior.

@borkdude
Copy link
Contributor

@Heliosmaster Seems good to me. I prefer the name without question mark (use question marks for predicate functions only: bbatsov/clojure-style-guide#182 (comment)).

Proposed name: :destroy-on-shutdown which defaults to true.

@Heliosmaster
Copy link
Author

@borkdude sounds good. In our previous implementation it was opt-in with destroy-on-shutdown? as the option flag :-) We'll remove the question mark and force push.

@borkdude
Copy link
Contributor

@Heliosmaster Documentation would also be nice.

@hamann hamann force-pushed the feat-add-shutdown-hook branch from 9ed1b0d to 068d530 Compare September 28, 2020 10:21
@borkdude
Copy link
Contributor

borkdude commented Sep 28, 2020

@Heliosmaster @hamann I have an example bb script here to kill all children of a process:

https://github.com/borkdude/babashka/blob/master/test/babashka/scripts/kill_child_processes.bb

Maybe it would also be useful in this context. Not sure if it warrants a different option or that we should just do this by default using this option.

@hamann
Copy link

hamann commented Sep 28, 2020

@borkdude After playing with the proposed solution for several times we realized that just calling (.destroy proc) is not sufficient in our case because it immediately returns and let the main process exit, but the subprocess is still alive for a while, producing uncontrollable output on the terminal and then exits. So in our case we would also have to add (.waitFor proc) and maybe (.destroyForcibly proc) after some time as a last resort.
So I have the feeling that instead of implementing the shutdown logic in the process function we should give the caller control over the shutdown behaviour for example with a nasty callback function or passing an atom where the process object will be stored/referenced.

@borkdude
Copy link
Contributor

OK, I'll wait with merging until we have more clarity on this.

@borkdude borkdude marked this pull request as draft September 28, 2020 11:47
@borkdude borkdude closed this Oct 16, 2020
@borkdude borkdude reopened this Oct 20, 2020
@borkdude borkdude closed this Oct 20, 2020
@borkdude borkdude mentioned this pull request Oct 20, 2020
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.

3 participants