-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
lib: fix event race condition with -e #11958
Conversation
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.
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. |
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.
LGTM with a comment
}); | ||
const result = module._compile(script, `${name}-wrapper`); | ||
if (process._print_eval) console.log(result); | ||
process._tickCallback(); |
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.
Can you add a comment here like in module.js?
e.g. // Handle any nextTicks added in the first tick of the program
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.
LGTM
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Landed in 9ff7ed2 |
I uh, would have liked the comment to be added. Someone's certainly going to be confused about this in the future. |
Doh! My apologies @Fishrock123 ... I completely missed your request |
Add a comment to match lib/module.js, missed in nodejs#11958. Refs: nodejs#11958
|
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
phew, got the backport working. that was fun! |
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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. |
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Add a comment to match lib/module.js, missed in #11958. PR-URL: #12050 Ref: #11958 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
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 <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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.