Fix race on open3/fork and session->register($pid) call -- second attempt
#22
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
The `collect_status` handler must be set before we fork/open3 a subprocess, otherwise we have a potential race that we miss that event.
cfconrad
changed the title
open3/fork and session->register($pid) call -- second attemptopen3/fork and session->register($pid) call -- second attempt
cfconrad
force-pushed
the
fix_race_on_register_process_v2
branch
from
0b00399
to
483476e
Compare
|
set to WIP as this c1c0ba9 test fails with this code! |
cfconrad
force-pushed
the
fix_race_on_register_process_v2
branch
from
c1c0ba9
to
16621eb
Compare
cfconrad
changed the title
open3/fork and session->register($pid) call -- second attemptopen3/fork and session->register($pid) call -- second attempt
If a sub process is exiting before the parent process calls `session->register()` we miss the `collect_status` thus the process end up in `session->orphan()`. Also we did a lot of work inside the signal handler context. This is actually bad as well. This fix change the work of the signalhandler to a minimum, it just collect the process info and store it inside of an array. Then on each `is_running()` call, we process this list and call the corresponding events.
The `session->reset()` method should also reset the handler function, otherwise the sighandler doesn't work in a subprocess.
cfconrad
force-pushed
the
fix_race_on_register_process_v2
branch
from
16621eb
to
a37fc97
Compare
|
I changed this code. The SIG_CHLD handler now just collect the process info and later, not inside the signal handler context, we consume it and emit all these events. |
|
Thanks @cfconrad!! |
Merged
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.
This is a second option to fix this issue, see #21.
I would like to discuss what way fit's more.
The disadvantage of this process is, that we change the SIGMASK and the child will inherit it.
But we only care of SIG_CHLD and this is reset in
fork()path anyway.