Skip to content
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

Impossible to listen to beforeExit in single-tick --eval #8534

Closed
Fishrock123 opened this issue Sep 14, 2016 · 2 comments · Fixed by #8821
Closed

Impossible to listen to beforeExit in single-tick --eval #8534

Fishrock123 opened this issue Sep 14, 2016 · 2 comments · Fixed by #8821
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem.

Comments

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 14, 2016

  • Version: v2.2.0+ (?)
  • Platform: all
  • Subsystem: process

The following exits without printing:

node -e "process.on('beforeExit', () => process._rawDebug('hi'))"

However, this does print:

node -e "process.on('beforeExit', () => process._rawDebug('hi')); setImmediate(()=>{})"

I did some debugging and using this patch I get the following output:

diff --git a/lib/events.js b/lib/events.js
index d676580..16ee82c 100644
--- a/lib/events.js
+++ b/lib/events.js
@@ -138,6 +138,8 @@ EventEmitter.prototype.emit = function emit(type) {
   var needDomainExit = false;
   var doError = (type === 'error');

+  // process._rawDebug((new Error()).stack)
+
   events = this._events;
   if (events)
     doError = (doError && events.error == null);
@@ -169,6 +171,9 @@ EventEmitter.prototype.emit = function emit(type) {

   handler = events[type];

+  process._rawDebug('01.5:' + type)
+  process._rawDebug(typeof handler)
+
   if (!handler)
     return false;

@@ -234,6 +239,9 @@ function _addListener(target, type, listener, prepend) {
   }

   if (!existing) {
+    process._rawDebug('@@@@')
+    // process._rawDebug((new Error()).stack)
+    // process._rawDebug(listener.toString())
     // Optimize the case of one listener. Don't need the extra array object.
     existing = events[type] = listener;
     ++target._eventsCount;
diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js
index 529645a..5f43ba0 100644
--- a/lib/internal/process/next_tick.js
+++ b/lib/internal/process/next_tick.js
@@ -138,6 +138,8 @@ function setupNextTick() {
   }

   function nextTick(callback) {
+    process._rawDebug('#####')
+    process._rawDebug((new Error()).stack)
     if (typeof callback !== 'function')
       throw new TypeError('callback is not a function');
     // on the way out, don't bother. it won't get fired anyway.
./node -e "process.on('beforeExit', () => process._rawDebug('hi'))"
@@@@
@@@@
01.5:newListener
function
@@@@
#####
Error
    at process.nextTick (internal/process/next_tick.js:142:24)
    at evalScript (bootstrap_node.js:344:13)
    at run (bootstrap_node.js:110:11)
    at run (bootstrap_node.js:382:7)
    at startup (bootstrap_node.js:109:9)
    at bootstrap_node.js:497:3
01.5:beforeExit
undefined
01.5:newListener
function
@@@@
01.5:exit
undefined

That is, beforeExit fires before the listener is attached.

Turns out that at evalScript (bootstrap_node.js:344:13) leads to this code and comment in evalScript():

    // Defer evaluation for a tick.  This is a workaround for deferred
    // events not firing when evaluating scripts from the command line,
    // see https://github.com/nodejs/node/issues/1600.
    process.nextTick(function() {
      const result = module._compile(script, `${name}-wrapper`);
      if (process._print_eval) console.log(result);
    });

That comment leads back to #1600 - net.listen does not emit 'listening' event when in --eval mode.

That was fixed in 93a44d5 by @bnoordhuis and reviewed by @trevnorris resulting in this code in evalScript().

Not sure how to fix this right now, but it seems like a deeper bug somewhere relating to startup and nextTick.

Refs: #1793 & #1600 & also the older nodejs/node-v0.x-archive#14168

Edit: found by @cxreg

@Fishrock123 Fishrock123 added confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem. labels Sep 14, 2016
@Fishrock123
Copy link
Contributor Author

ping @bnoordhuis and @trevnorris

@bnoordhuis
Copy link
Member

#8821

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Oct 24, 2016
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script.  'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate().  Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Fixes: nodejs#8534
PR-URL: nodejs#8821
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this issue Nov 2, 2016
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script.  'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate().  Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Fixes: #8534
PR-URL: #8821
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 18, 2016
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script.  'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate().  Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Fixes: #8534
PR-URL: #8821
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 18, 2016
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script.  'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate().  Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Ref: nodejs#9680
Fixes: nodejs#8534
PR-URL: nodejs#8821
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 19, 2016
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.

A side effect of that change is that 'beforeExit' listeners run before
the actual script.  'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.

Fix that by using setImmediate().  Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.

Fixes: #8534
PR-URL: #8821
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants