Fix child process stdio data loss with slow piped consumers #7185
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
While commit 12274a5 fixed data loss when a readable event handler is attached, the problem still exists when the stdio stream has a piped consumer that doesn't read fast enough. Signed-off-by: Petros Angelatos <[email protected]>
This commit prevents child process stdio streams from being automatically flushed on child process exit/close if a piped consumer has been attached at the time of exit. Without this, child process stdio data can be lost if the process exits quickly and the piped consumer hasn't had the chance to read the remaining data. Fixes: nodejs#7184 Signed-off-by: Petros Angelatos <[email protected]>
In order for the testcase to fail, the calls to read() must be done after 'exit' is emitted and after flushStdio has been called. With this change, the testcase will catch any regressions on this issue. Signed-off-by: Petros Angelatos <[email protected]>
nodejs-github-bot
added
the
child_process
petrosagg
changed the title
| let buf; | ||
| while (buf = this.read()) | ||
| buffer.push(buf); | ||
| }); |
There was a problem hiding this comment.
This testcase was added to cover the issue fixed here: balena-io-experimental@12274a5
But the testcase passes even if the change of that commit is reverted. So I just fixed the testcase to reflect what was actually fixed there.
There was a problem hiding this comment.
@petrosagg Can confirm that this test case worked even before 12274a5 – nice catch! A separate PR seems like a good idea, yep
|
Confirmed that this issue is fixed in #7160 |
|
@petrosagg I’d like to get both of your changes to the tests in… are you interested in PRing yourself? |
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.
Checklist
Affected core subsystem(s)
Description of change
The
flushStdio()checks if there are readable handlers attached to the stdio streams to prevent data loss but it doesn't check if there are piped consumers. Change it so that it checkspipesCounttoo.I also fixed the previous testcase on the same issue to actually catch the regression as it would previously pass regardless of what flushStdio was doing.
Fixes #7184