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

async_hooks: emitAfter correctly on fatalException #14914

Merged
merged 1 commit into from
Aug 30, 2017

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Aug 18, 2017

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/

@nodejs-github-bot 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 trevnorris added the async_hooks Issues and PRs related to the async hooks subsystem. label Aug 18, 2017
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few questions:

  1. why is this better? feels less abstract/more "internal implementation exposed"
  2. who actually pops the ids?
  3. cache emitAfter?
  const emitAfter = NativeModule.require('async_hooks').emitAfter;
  do {
    emitAfter(async_uid_fields[kCurrentAsyncId]);
  }...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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().

  1. who actually pops the ids?

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

  1. 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@trevnorris trevnorris Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Another CI after rebase

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

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

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

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

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

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>
@trevnorris trevnorris merged commit 244ada3 into nodejs:master Aug 30, 2017
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
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>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
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>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Aug 31, 2017
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>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
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 MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
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 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
Copy link
Contributor

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

@trevnorris
Copy link
Contributor Author

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

@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants