process: ensure uvfinalize and _uv_close_cb are synchronized #44476
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
There appeared to be a possibility they could race and the data field might already be destroyed before we reached the close callback, from looking at the state of the program when reproducing #44460. This is because the uv_return_spawn set the handle to NULL, which later can cause the uvfinalize to exit early (if the finalizer gets run on another thread, since we have disabled finalizers on our thread). Then the GC can reap the julia Process object prior to uv_close cleaning up the object. We solve this by calling disassociate_julia_struct before dropping the reference to the handle. But then we also fully address any remaining race condition by having uvfinalize acquire a lock also. Fixes #44460
vtjnash
added
backport 1.8
47 tasks
…ronized The uv_return_spawn callback also needs to be synchronized with the constructor, since we might have arrived there before we finished allocating the Process struct here, leading to missed exit events.
KristofferC
pushed a commit
that referenced
this pull request
Mar 11, 2022
There appeared to be a possibility they could race and the data field might already be destroyed before we reached the close callback, from looking at the state of the program when reproducing #44460. This is because the uv_return_spawn set the handle to NULL, which later can cause the uvfinalize to exit early (if the finalizer gets run on another thread, since we have disabled finalizers on our thread). Then the GC can reap the julia Process object prior to uv_close cleaning up the object. We solve this by calling disassociate_julia_struct before dropping the reference to the handle. But then we also fully address any remaining race condition by having uvfinalize acquire a lock also. The uv_return_spawn callback also needs to be synchronized with the constructor, since we might have arrived there before we finished allocating the Process struct here, leading to missed exit events. Fixes #44460 (cherry picked from commit c591bf2)
50 tasks
|
Needs a manual backport towards |
KristofferC
pushed a commit
that referenced
this pull request
Apr 19, 2022
There appeared to be a possibility they could race and the data field might already be destroyed before we reached the close callback, from looking at the state of the program when reproducing #44460. This is because the uv_return_spawn set the handle to NULL, which later can cause the uvfinalize to exit early (if the finalizer gets run on another thread, since we have disabled finalizers on our thread). Then the GC can reap the julia Process object prior to uv_close cleaning up the object. We solve this by calling disassociate_julia_struct before dropping the reference to the handle. But then we also fully address any remaining race condition by having uvfinalize acquire a lock also. The uv_return_spawn callback also needs to be synchronized with the constructor, since we might have arrived there before we finished allocating the Process struct here, leading to missed exit events. Fixes #44460 (cherry picked from commit c591bf2)
45 tasks
78 tasks
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.
There appeared to be a possibility they could race and the data field
might already be destroyed before we reached the close callback,
from looking at the state of the program when reproducing #44460.
This is because the uv_return_spawn set the handle to NULL, which later
can cause the uvfinalize to exit early (if the finalizer gets run on
another thread, since we have disabled finalizers on our thread). Then
the GC can reap the julia Process object prior to uv_close cleaning up
the object. We solve this by calling disassociate_julia_struct before
dropping the reference to the handle. But then we also fully address any
remaining race condition by having uvfinalize acquire a lock also.
Fixes #44460
(added backport labels, since in quick testing, it seems to apply cleanly to those branches)