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

fix signal handling on unix #1191

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

despairblue
Copy link
Contributor

Using Unix.create_process_env starts a child process, but nothing is
setup to forward signals or interrupts. Making it impossible for the
started program to handle signal itself using Sys.signal or
Sys.catch_break. This is a minor inconvinience for CLI programs (e.g.
you can't implement a REPL that only exits when sending CTRL-C twice
like nodejs does), but this is a major problem for program that are
supposed to run in k8s.

K8s expects programs to keep serving requests when it sends a TERM
signal. The TERM signal is send before k8s makes sure that they won't
receive traffic anymore. It's expected that they mark themselves as not
ready, handle all open traffic, and shutdown when done. For this the
programs need to handle the TERM signal.

Using `Unix.create_process_env` starts a child process, but nothing is
setup to forward signals or interrupts. Making it impossible for the
started program to handle signal itself using `Sys.signal` or
`Sys.catch_break`. This is a minor inconvinience for CLI programs (e.g.
you can't implement a REPL that only exits when sending CTRL-C twice
like nodejs does), but this is a major problem for program that are
supposed to run in k8s.

K8s expects programs to keep serving requests when it sends a TERM
signal. The TERM signal is send before k8s makes sure that they won't
receive traffic anymore. It's expected that they mark themselves as not
ready, handle all open traffic, and shutdown when done. For this the
programs need to handle the TERM signal.
@ManasJayanth
Copy link
Member

PR looks great. The Alpine build failure is unrelated.

@despairblue In future, I revert/modify this commit and add a proper signal forwarding. Might add you as a reviewer/tester. Thanks for this PR!

@ManasJayanth
Copy link
Member

ManasJayanth commented Oct 13, 2020

@despairblue A nightly build should be ready for consumption (npm i -g @esy-nightly/esy) now

@despairblue
Copy link
Contributor Author

Latest seems to be 05ada39.

@despairblue
Copy link
Contributor Author

Yep, just checked, my commit is not yet available, I'll test it as soon as it's available :)

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.

None yet

2 participants