async_hooks: emitAfter correctly on fatalException #14914

trevnorris · 2017-08-18T11:32:32Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

Notes

Previously calling emitAfter() from _fatalException would skip the
first asyncId. Instead use the size() of the std::stack to determine how
many times to loop and call emitAfter().

CI: https://ci.nodejs.org/job/node-test-pull-request/9730/

refack · 2017-08-18T11:45:09Z

lib/internal/bootstrap_node.js

        // Emit the after() hooks now that the exception has been handled.
        if (async_hook_fields[kAfter] > 0) {
          do {
            NativeModule.require('async_hooks').emitAfter(
              async_uid_fields[kCurrentAsyncId]);
-          // popAsyncIds() returns true if there are more ids on the stack.
-          } while (popAsyncIds(async_uid_fields[kCurrentAsyncId]));
+          } while (asyncIdStackSize() > 0);


Few questions:

why is this better? feels less abstract/more "internal implementation exposed"

who actually pops the ids?

cache emitAfter?

const emitAfter = NativeModule.require('async_hooks').emitAfter; do { emitAfter(async_uid_fields[kCurrentAsyncId]); }...

Are we sure asyncIdStackSize() > 0 at the first iteration?

why is this better? feels less abstract/more "internal implementation exposed"

because it fixes the bug :) and it's actually the other way around. previously I was overriding implementation details in emitAfter().

who actually pops the ids?

emitAfter() pops the id. that's why it was skipping every other id in the stack.

cache emitAfter?

I'm doing it for the same reason as the require('timers') call below. Because if the script fails early in the startup process then the module might not have been loaded yet. And require() already caches the script into an object, doesn't it?

who actually pops the ids?

emitAfter() pops the id. that's why it was skipping every other id in the stack.

Ohh emitAfter is emitAfterScript not emitAfterNative

cache emitAfter?

I'm doing it for the same reason as the require('timers') call below. Because if the script fails early in the startup process then the module might not have been loaded yet. And require() already caches the script into an object, doesn't it?

Ack, but if you do it just before the loop it (1) looks cleaner [to me] (2) still lazy enough.

Ack, but if you do it just before the loop it (1) looks cleaner [to me] (2) still lazy enough.

do you mean creating let async_hooks = null; in setupProcessFatal() then doing something like?:

if (async_hooks === null) async_hooks = NativeModule.require('async_hooks');

Or just const async_hooks = NativeModule.require('async_hooks'); before the loop every time?

refack · 2017-08-18T11:53:54Z

test/parallel/test-emit-after-uncaught-exception.js

+  assert.strictEqual(er.message, 'bye');
+  collect = false;
+}));
+


what happens is you throw here (where AFAICT the stack is still empty)?
What's the value of async_uid_fields[kCurrentAsyncId])?

The first pass of the script is the "bootstrap", which always has an id of 1. But as soon as it throws the stack is cleared and async_uid_fields[kCurrentAsyncId] === 0 until another stack is entered. That answer your question?

trevnorris · 2017-08-22T09:35:36Z

Another CI after rebase

CI: https://ci.nodejs.org/job/node-test-pull-request/9785/

jasnell · 2017-08-23T18:02:49Z

New CI run: https://ci.nodejs.org/job/node-test-pull-request/9801/

(previous had a couple of unrelated failures butjust in case...)

jasnell · 2017-08-24T22:42:11Z

Running partial CI again: https://ci.nodejs.org/job/node-test-commit-arm/11727/

@trevnorris ... is this ready to go?

Previously calling emitAfter() from _fatalException would skipt the first asyncId. Instead use the size() of the std::stack to determine how many times to loop and call emitAfter(). PR-URL: nodejs#14914 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Previously calling emitAfter() from _fatalException would skipt the first asyncId. Instead use the size() of the std::stack to determine how many times to loop and call emitAfter(). PR-URL: nodejs/node#14914 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Previously calling emitAfter() from _fatalException would skipt the first asyncId. Instead use the size() of the std::stack to determine how many times to loop and call emitAfter(). PR-URL: nodejs#14914 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Previously calling emitAfter() from _fatalException would skipt the first asyncId. Instead use the size() of the std::stack to determine how many times to loop and call emitAfter(). PR-URL: #14914 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins · 2017-09-20T19:10:16Z

Do we want to backport async_hooks as a whole before 6 goes to maintenance

trevnorris · 2017-09-21T08:47:39Z

@MylesBorins We'd need to do work to remove Promise support, since the API won't exist in v6.x. @addaleax thoughts?

trevnorris requested a review from addaleax August 18, 2017 11:32

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 18, 2017

trevnorris added the async_hooks Issues and PRs related to the async hooks subsystem. label Aug 18, 2017

refack reviewed Aug 18, 2017

View reviewed changes

jasnell approved these changes Aug 18, 2017

View reviewed changes

refack approved these changes Aug 19, 2017

View reviewed changes

trevnorris force-pushed the fatalex-after-fix branch from 55e8178 to 8c506d9 Compare August 22, 2017 09:32

trevnorris force-pushed the fatalex-after-fix branch from 8c506d9 to e4a04e8 Compare August 22, 2017 09:39

trevnorris force-pushed the fatalex-after-fix branch from e4a04e8 to 244ada3 Compare August 30, 2017 06:39

trevnorris merged commit 244ada3 into nodejs:master Aug 30, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Sep 20, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: emitAfter correctly on fatalException #14914

async_hooks: emitAfter correctly on fatalException #14914

trevnorris commented Aug 18, 2017 •

edited

Loading

refack Aug 18, 2017

refack Aug 18, 2017

trevnorris Aug 18, 2017

refack Aug 18, 2017

trevnorris Aug 22, 2017 •

edited

Loading

refack Aug 18, 2017

trevnorris Aug 22, 2017

trevnorris commented Aug 22, 2017

jasnell commented Aug 23, 2017

jasnell commented Aug 24, 2017

MylesBorins commented Sep 20, 2017

trevnorris commented Sep 21, 2017

async_hooks: emitAfter correctly on fatalException #14914

async_hooks: emitAfter correctly on fatalException #14914

Conversation

trevnorris commented Aug 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Notes

refack Aug 18, 2017

Choose a reason for hiding this comment

refack Aug 18, 2017

Choose a reason for hiding this comment

trevnorris Aug 18, 2017

Choose a reason for hiding this comment

refack Aug 18, 2017

Choose a reason for hiding this comment

trevnorris Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 18, 2017

Choose a reason for hiding this comment

trevnorris Aug 22, 2017

Choose a reason for hiding this comment

trevnorris commented Aug 22, 2017

jasnell commented Aug 23, 2017

jasnell commented Aug 24, 2017

MylesBorins commented Sep 20, 2017

trevnorris commented Sep 21, 2017

trevnorris commented Aug 18, 2017 •

edited

Loading

trevnorris Aug 22, 2017 •

edited

Loading