-
-
Notifications
You must be signed in to change notification settings - Fork 153
Stop server on SIGTERM by default #406
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
base: master
Are you sure you want to change the base?
Conversation
|
Looks good! I was wondering if we should also handle SIGQUIT but that already works. See also this discussion for the common signals to terminate processes. SIGHUP might be interesting at some point. |
In what context? I have But interestingly This appears to be an issue inside containers. Is |
| let on_sigterm = | ||
| let promise, resolve = Lwt.wait () in | ||
| ignore (Lwt_unix.on_signal Sys.sigterm (fun _ -> Lwt.wakeup_later resolve ())); | ||
| promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating this promise at program init time seems questionable now.
Before, the never promise was a sort of orphan promise in the heap that was fine to have floating around since init time.
But the on_sigterm promise should probably only be created if serve/run are actually called, since a program might end up with Dream linked in but never actually use it. It seems questionable to affect signal handling at init time even if Dream does not end up getting used (or at init time even if Dream does end up getting used).
|
TODO: reconsider approach for multicore. |
Don't install the signal handler at the module scope, to avoid polluting the user's application with potentially unused handlers.
Fix #174