child_process: fix IPC 'message' event regression #13856
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
nodejs-github-bot
added
the
child_process
mcollina
reviewed
Jun 22, 2017
lib/internal/child_process.js
Outdated
| } | ||
| process.nextTick(() => { | ||
| target.emit(eventName, message, handle); | ||
| }); |
There was a problem hiding this comment.
can you restore the use of the emit() function to avoid allocating this closure?
|
@mcollina updated to only remove the |
mcollina
approved these changes
Jun 22, 2017
|
Benchmark results from my machine: Looks like only a fraction of the gains from #13459 were lost. |
|
Thanks Colin. Still LGTM. |
santigimeno
approved these changes
Jun 23, 2017
|
Confirm test fails on my machine as well (Windows). |
This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: nodejs#6909 Refs: nodejs#13459 Refs: nodejs#13648 PR-URL: nodejs#13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
|
The benchmark results from the original PR would be these since the PR was originally started before the V8 5.9 merge, so this revert has a larger impact actually. Probably the only thing that makes it have a little less of an impact is the |
addaleax
pushed a commit
that referenced
this pull request
Jun 29, 2017
This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: #6909 Refs: #13459 Refs: #13648 PR-URL: #13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Merged
addaleax
pushed a commit
that referenced
this pull request
Jul 11, 2017
This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: #6909 Refs: #13459 Refs: #13648 PR-URL: #13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
addaleax
pushed a commit
that referenced
this pull request
Jul 18, 2017
This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: #6909 Refs: #13459 Refs: #13648 PR-URL: #13856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
MylesBorins
added
the
baking-for-lts
|
I've thrown a baking label on this... we should likely land if we backport other change |
MylesBorins
removed
the
baking-for-lts
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 PR reverts 8208fda from #13459. When IPC messages are not emitted in the next tick, a
'message'handler that throws can break the IPC read loop. This was originally fixed in #6909.The second commit in this PR adds a regression test that fails with 8208fda, but passes with the revert (at least on my machine).
Note: I believe this can be fixed without a full revert of 8208fda, but wanted to use that as a starting point for this PR.
Refs: #6909
Refs: #13459
Refs: #13648
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
child_process, test