lib: fix event race condition with -e#11958
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards.
nodejs-github-bot added the lib / src MemberAuthor
bnoordhuis commented Mar 21, 2017
This is the alternative take from #8821 (comment). Guess I should've landed that instead but then we wouldn't have had test coverage for this particular case. |
cjihrig approved these changes Mar 21, 2017
jasnell approved these changes Mar 21, 2017
Fishrock123 approved these changes Mar 21, 2017
| }); | ||
| constresult=module._compile(script,`${name}-wrapper`); | ||
| if(process._print_eval)console.log(result); | ||
| process._tickCallback(); |
Contributor
There was a problem hiding this comment.
Can you add a comment here like in module.js?
e.g. // Handle any nextTicks added in the first tick of the program
watilde approved these changes Mar 22, 2017
jasnell pushed a commit that referenced this pull request Mar 24, 2017
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards. PR-URL: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Member
jasnell commented Mar 24, 2017
Landed in 9ff7ed2 |
Contributor
Fishrock123 commented Mar 24, 2017
I uh, would have liked the comment to be added. Someone's certainly going to be confused about this in the future. |
Member
jasnell commented Mar 24, 2017
Doh! My apologies @Fishrock123 ... I completely missed your request |
gibfahn added a commit to gibfahn/node that referenced this pull request Mar 26, 2017
Add a comment to match lib/module.js, missed in nodejs#11958. Refs: nodejs#11958
2 tasks
Member
gibfahn commented Mar 26, 2017
|
jasnell pushed a commit that referenced this pull request Mar 28, 2017
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards. PR-URL: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Merged
Member
ChALkeR commented Mar 30, 2017
kevinsawicki pushed a commit to electron/node that referenced this pull request Apr 6, 2017
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards. PR-URL: nodejs/node#11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards. PR-URL: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
Contributor
MylesBorins commented Apr 18, 2017
phew, got the backport working. that was fun! |
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Richard Lau <[email protected]>
MemberAuthor
bnoordhuis commented Apr 18, 2017
Let me guess, because of the message tests? They are a lot of work for little value. I might just file a pull request to remove them altogether one of these days. |
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards. PR-URL: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the to-be-evaluated code at a later time than before because it switches from `process.nextTick()` to `setImmediate()`. It affects `-e 'process.on("message", ...)'` because there is now a larger time gap between startup and attaching the event listener, increasing the chances of missing early messages. I'm reasonably sure `process.nextTick()` was also susceptible to that, only less pronounced. Avoid the problem altogether by evaluating the code synchronously. Harmonizes the logic with `Module.runMain()` from lib/module.js which also calls `process._tickCallback()` afterwards. PR-URL: nodejs/node#11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Add a comment to match lib/module.js, missed in nodejs#11958. PR-URL: nodejs/node#12050 Ref: nodejs/node#11958 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
10 participants
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.
Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the
to-be-evaluated code at a later time than before because it switches
from
process.nextTick()tosetImmediate().It affects
-e 'process.on("message", ...)'because there is now alarger time gap between startup and attaching the event listener,
increasing the chances of missing early messages. I'm reasonably
sure
process.nextTick()was also susceptible to that, only lesspronounced.
Avoid the problem altogether by evaluating the code synchronously.
Harmonizes the logic with
Module.runMain()from lib/module.jswhich also calls
process._tickCallback()afterwards.