Fix thread safety risk when closing uv process handles #6878
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.
A confusing file-handle related issue from TSAN runs (spun out from #6616).
Full stack is below, looks like some cross-thread interaction over file handles. Always involves the
ProcessWriter
orProcessReader
fromprocess_launcher.h
. Investigation of that code reveals a bunch of inline calls toclose()
. I think those are unsafe - we shouldn't be callingclose()
and trying to free the underlyinguv
handle within a callback, we should rely on the existingclose_ptr
for that. That requires changing the ownership rules, so those are stashed in theProcessEntry
and persist until the process completes.(A minor implementation detail - it took me a lot of trial-and-error to work out that the
close_ptr
andproxy_ptr
do not support null-initialisation. They are default-constructed pointing to a valid instance, and trying to make them start out with anullptr
likely results in a downstreamT()
default-constructor never being called. So where we want nullable semantics, we have to wrap them again in aunique_ptr
orshared_ptr
. I think this is fixable, but out-of-scope here).Error trace for the curious: