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: If first hook is disabled, async ID tracking is permanently disabled #27585

Closed
kjin opened this issue May 6, 2019 · 2 comments
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises.

Comments

@kjin
Copy link
Contributor

kjin commented May 6, 2019

  • Version: v12.0.0
  • Platform: macOS 10.14.3 (likely irrelevant)
  • Subsystem: async_hooks

If I create an async hook and then disable it, then async ID tracking no longer works, even if subsequent async hooks are created and enabled, or if the first async hook is re-enabled.

Repro:

const ah = require('async_hooks');
// switch the order of these two lines, and things appear to work correctly
ah.createHook({ init: () => {} }).enable().disable().enable();
ah.createHook({ init: () => {} }).enable();

async function main() {
  console.log(ah.executionAsyncId()); // should print 1
  await 0;
  console.log(ah.executionAsyncId()); // should print 7, but prints 1
}

main();

This appears to affect all of v12+ so far.

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises. v8 engine Issues and PRs related to the V8 dependency. and removed v8 engine Issues and PRs related to the V8 dependency. labels May 6, 2019
@addaleax
Copy link
Member

addaleax commented May 6, 2019

Sorry, this happened because of my 96c3224 – it just happened to work out before, but the issue here is that disabling the promise hook happens asynchronously, i.e. after all the synchronous enable() calls.

#27585 should address this. :)

addaleax added a commit to addaleax/node that referenced this issue May 6, 2019
The promise hook has been disabled asynchronously in order to solve
issues when an async hook is disabled during a microtask.

This means that after scheduling the disable-promise-hook call,
attempts to enable it synchronously will later be unintentionally
overridden.

In order to solve this, make sure that the promise hooks are still
no longer desired at the time at which we would disable them.

Fixes: nodejs#27585
@kjin
Copy link
Contributor Author

kjin commented May 6, 2019

@addaleax Thanks for the fast fix! I checkout out your change and it does seem to fix the underlying issue for the module in which I encountered this problem.

targos pushed a commit that referenced this issue May 13, 2019
The promise hook has been disabled asynchronously in order to solve
issues when an async hook is disabled during a microtask.

This means that after scheduling the disable-promise-hook call,
attempts to enable it synchronously will later be unintentionally
overridden.

In order to solve this, make sure that the promise hooks are still
no longer desired at the time at which we would disable them.

Fixes: #27585

PR-URL: #27590
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants