lib: fix event race condition with -e by bnoordhuis · Pull Request #11958 · nodejs/node · GitHub

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.

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.

LGTM with a comment

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

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

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

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>

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>

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>

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

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

lib: fix event race condition with -e by bnoordhuis · Pull Request #11958 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: fix event race condition with -e - #11958

lib: fix event race condition with -e#11958
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix11948

bnoordhuis commented Mar 21, 2017

Uh oh!

bnoordhuis commented Mar 21, 2017

Uh oh!

Fishrock123 left a comment

Uh oh!

Fishrock123 Mar 21, 2017

Uh oh!

mhdawson left a comment

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

Fishrock123 commented Mar 24, 2017

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

gibfahn commented Mar 26, 2017

Uh oh!

ChALkeR commented Mar 30, 2017

Uh oh!

MylesBorins commented Apr 18, 2017

Uh oh!

bnoordhuis commented Apr 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lib: fix event race condition with -e by bnoordhuis · Pull Request #11958 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: fix event race condition with -e - #11958

lib: fix event race condition with -e#11958bnoordhuis wants to merge 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:fix11948bnoordhuis/io.js:fix11948Copy head branch name to clipboard

Conversation

bnoordhuis commented Mar 21, 2017

Uh oh!

bnoordhuis commented Mar 21, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Mar 21, 2017

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

Fishrock123 commented Mar 24, 2017

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

gibfahn commented Mar 26, 2017

Uh oh!

ChALkeR commented Mar 30, 2017

Uh oh!

MylesBorins commented Apr 18, 2017

Uh oh!

bnoordhuis commented Apr 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lib: fix event race condition with -e#11958
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix11948