lib: fix event race condition with -e #11958

bnoordhuis · 2017-03-21T10:50:56Z

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.

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.

bnoordhuis · 2017-03-21T10:53:25Z

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.

Fishrock123

LGTM with a comment

Fishrock123 · 2017-03-21T15:20:33Z

lib/internal/bootstrap_node.js

-    });
+    const result = module._compile(script, `${name}-wrapper`);
+    if (process._print_eval) console.log(result);
+    process._tickCallback();


Can you add a comment here like in module.js?

e.g. // Handle any nextTicks added in the first tick of the program

mhdawson

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>

jasnell · 2017-03-24T16:26:32Z

Landed in 9ff7ed2

Fishrock123 · 2017-03-24T16:34:58Z

I uh, would have liked the comment to be added. Someone's certainly going to be confused about this in the future.

jasnell · 2017-03-24T16:35:46Z

Doh! My apologies @Fishrock123 ... I completely missed your request

Add a comment to match lib/module.js, missed in nodejs#11958. Refs: nodejs#11958

gibfahn · 2017-03-26T15:10:20Z

I uh, would have liked the comment to be added.

#12050

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>

ChALkeR · 2017-03-30T14:57:35Z

c5b07d4 was backported to 6.x, perhaps this should be backported, too.
Refs: #11948, #8821.

/cc @nodejs/lts

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>

MylesBorins · 2017-04-18T22:41:09Z

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>

bnoordhuis · 2017-04-18T22:56:25Z

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>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 21, 2017

bnoordhuis mentioned this pull request Mar 21, 2017

message not received in child process when forked with --eval #11948

Closed

cjihrig approved these changes Mar 21, 2017

View reviewed changes

jasnell approved these changes Mar 21, 2017

View reviewed changes

Fishrock123 approved these changes Mar 21, 2017

View reviewed changes

mhdawson approved these changes Mar 21, 2017

View reviewed changes

watilde approved these changes Mar 22, 2017

View reviewed changes

jasnell closed this Mar 24, 2017

gibfahn added a commit to gibfahn/node that referenced this pull request Mar 26, 2017

lib: add comment to script eval _tickCallback

2dd907e

Add a comment to match lib/module.js, missed in nodejs#11958. Refs: nodejs#11958

gibfahn mentioned this pull request Mar 26, 2017

lib: add comment to script eval _tickCallback #12050

Closed

2 tasks

MylesBorins mentioned this pull request Mar 28, 2017

V7.8.0 proposal #12104

Merged

ChALkeR added the lts-watch-v6.x label Mar 30, 2017

bnoordhuis deleted the fix11948 branch March 30, 2017 16:04

kevinsawicki mentioned this pull request Apr 6, 2017

Cherry pick node fix for ChildProcess.fork --eval bug electron/electron#9132

Merged

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the land-on-v6.x label Apr 18, 2017

MylesBorins removed the lts-watch-v6.x label Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: fix event race condition with -e #11958

lib: fix event race condition with -e #11958

bnoordhuis commented Mar 21, 2017

bnoordhuis commented Mar 21, 2017

Fishrock123 left a comment

Fishrock123 Mar 21, 2017

mhdawson left a comment

jasnell commented Mar 24, 2017

Fishrock123 commented Mar 24, 2017

jasnell commented Mar 24, 2017

gibfahn commented Mar 26, 2017

ChALkeR commented Mar 30, 2017

MylesBorins commented Apr 18, 2017

bnoordhuis commented Apr 18, 2017

lib: fix event race condition with -e #11958

lib: fix event race condition with -e #11958

Conversation

bnoordhuis commented Mar 21, 2017

bnoordhuis commented Mar 21, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 Mar 21, 2017

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

jasnell commented Mar 24, 2017

Fishrock123 commented Mar 24, 2017

jasnell commented Mar 24, 2017

gibfahn commented Mar 26, 2017

ChALkeR commented Mar 30, 2017

MylesBorins commented Apr 18, 2017

bnoordhuis commented Apr 18, 2017