Make signal handling work on OCaml 5 and with Eio #993
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before installing a signal handler, Lwt now checks first with the engine, which may now prevent Lwt from installing its handler and instead forward signals from some external handler. This allows e.g. sharing the SIGCHLD handler with Eio.
|
Eio uses a plain OCaml signal handler for SIGCHLD, so this might make #994 more likely. |
If an OCaml signal handler runs in a Lwt worker thread then the process will dereference a NULL pointer and crash. The worker tried to avoid this by masking out all signals after starting, but by then it may be too late. Now, we mask out signals before creating a worker thread and then restore the mask in the main thread afterwards.
|
I've added a fix for #994 to this PR too (and updated the PR description). |
2 tasks
|
Looks good to me. |
raphael-proust
force-pushed
the
forward-signals
branch
from
e2ca6bc
to
ed81ff9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first commit fixes #981 by using
SA_ONSTACKwith OCaml 5 so that signals do not cause memory corruption.The second commit is a solution to #991. Before installing a signal handler, Lwt now checks first with the engine, which may now prevent Lwt from installing its handler and instead forward signals from some external handler.
This allows e.g. sharing the SIGCHLD handler with Eio.
It would also be possible to do it the other way around (so that Eio uses Lwt's signal handler). However, Lwt only notifies a single domain and if that domain is busy then the signal will be delayed, whereas Eio's signal handler notifies all interested domains immediately.
The third commit fixes #994, which causes the process to segfault if an OCaml signal handler runs in a Lwt worker thread before that thread has set the signal mask. This bug is more likely to be seen when using Lwt with Eio.
These changes should be low-risk:
SA_ONSTACKchange only affects OCaml 5, where Lwt already segfaults.false, causing the existing code to behave exactly as before./cc @raphael-proust