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

executionAsyncId() reports parent context ID in new Promise(fn) function #37407

Closed
isaacs opened this issue Feb 16, 2021 · 5 comments
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises.

Comments

@isaacs
Copy link
Contributor

isaacs commented Feb 16, 2021

  • Version:
  • Platform:
  • Subsystem:

What steps will reproduce the bug?

const { executionAsyncId, createHook } = require('async_hooks')

const {writeSync} = require('fs')
const {format} = require('util')
// hacky console.error() that doesn't cause more contexts to be created
const log = (...msg) => writeSync(2, format(...msg) + '\n')

const hook = createHook({})
hook.enable()

log('before promises', executionAsyncId())
new Promise(resolve => {
  log('eid, first promise', executionAsyncId())
  setTimeout(resolve)
}).then(() => new Promise(resolve => {
  log('eid, second promise', executionAsyncId())
}))

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

The executionAsyncId within the Promise function should be that of the Promise itself, rather than the parent context.

before promises 1
eid, first promise {some number greater than 1 and less than 4}
eid, second promise 4

What do you see instead?

before promises 1
eid, first promise 1
eid, second promise 4

Additional information

Possibly related to #26794, not sure.

Note that the Promise constructor callback does report the proper executionAsyncId when the Promise is created in the context of a Promise chain.

@isaacs
Copy link
Contributor Author

isaacs commented Feb 16, 2021

Here's a TAP-emitting test that you can use to comb through these. Note that the "expect" value seems to be the thing that's mistaken in the current node master branch, which is a huge step in the right direction!

https://gist.github.com/isaacs/c8d15def6869ff738c69037155dda2c2

@sajal50
Copy link
Contributor

sajal50 commented Feb 16, 2021

I think this might be due to the fact that a new Promise()'s fn callback is called synchronously. Therefore, the execution context didn't change.

@benjamingr benjamingr added async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises. labels Feb 16, 2021
@sajal50
Copy link
Contributor

sajal50 commented Feb 17, 2021

With the existing Promise Hooks types, I don't see any information/hooks given out to the embedder as to start and end points of the callback provided to the Promise. I'd be interested too in knowing if this is possible.

@Flarna
Copy link
Member

Flarna commented Feb 17, 2021

@sajal50 is correct. The function passed to new Promise() is called sync therefore it has the same eId as it is the same async operation. new Promise() creates/inits a new asyncId which should be signaled via the init hook and is that one you should see in the then callback.

In the end this is quite similar then callback based APIs, e.g. in fs.readFile('file', cb)) the eId within fs.readFile is the same as for the caller. fs.readFile creates a new id which is active in the callback.

@isaacs isaacs changed the title executionAsyncId() incorrectly reports parent context ID in new Promise(fn) function executionAsyncId() ~incorrectly~ reports parent context ID in new Promise(fn) function Feb 19, 2021
@isaacs isaacs changed the title executionAsyncId() ~incorrectly~ reports parent context ID in new Promise(fn) function executionAsyncId() reports parent context ID in new Promise(fn) function Feb 19, 2021
@isaacs
Copy link
Contributor Author

isaacs commented Feb 19, 2021

Hm. That does make sense. It's a bit weird that the executionAsyncId changes between the throw and the catch there, but anywhere you put that boundary is going to be strange. And since the executionAsyncId refers to the promise itself in the error/rejection handler, that's fine for making async-hook-domains work properly.

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

No branches or pull requests

4 participants