-
Notifications
You must be signed in to change notification settings - Fork 586
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
Fix more instances of the child_process.on('exit') race condition #4721
Merged
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
octogonz
requested review from
iclanton,
apostolisms,
D4N14L and
dmichon-msft
as code owners
May 17, 2024 19:37
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.
When signal
is truthy, exitCode === null
, so some of these checks need to change to account for that.
iclanton
approved these changes
May 23, 2024
7 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.
Summary
Following up from PR #4711, I audited the Rush Stack code base to look for any other invocations that might be affected by the same issue.
That PR did not trigger publishing of Rush, so I've also included a
rush change
file to publish a Rush release.Details
In this lengthy Zulip chat we investigated a Rush build cache failure on Windows Subsystem for Linux (WSL) that turned out to be a race condition in the way that
child_process.on('exit')
was handled.Background info
The sequence of events is like this:
child_process.on('data')
is being used to collect STDERR and STDOUTchild_process.on('exit')
fires indicating that the process has terminatedchild_process.on('data')
continues to fire a few more times as STDERR/STDOUT finish flushing 👈👈👈child_process.on('close')
finally fires indicating that the flush is complete, and includes the same exit code information that was available in'exit'
Note that
child_process.on('error')
can also fire, either up front to indicate spawning failed entirely (in which case there is noexit
event at all), or after somedata
has been received to indicate other kinds of errors.Therefore,
child_process.on('exit')
is redundant and does NOT normally need to be handled at all; its main value is if the parent wants to be notified as early as possible.The bug
The race condition bug occurs when functions are handling
data
to collect output, but handleexit
by rejecting immediately. In this case, their STDERR/STDOUT can get truncated if the flushing is not complete.This race condition is somewhat rare. In the case of #4711, the affected code had existed for years and years without any issues being reported. But on Windows Subsystem for Linux, @akres encountered a situation where it caused a noticeable malfunction and was reproducible.
How it was tested
I did not try to reproduce the race condition in the modified code. However I did confirm that removing the 500ms delay does not break Rundown.