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: context loss when awaiting a thenable #27599

Closed
Qard opened this issue May 7, 2019 · 8 comments
Closed

async_hooks: context loss when awaiting a thenable #27599

Qard opened this issue May 7, 2019 · 8 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@Qard
Copy link
Member

Qard commented May 7, 2019

It seems async_hooks is losing context, in certain cases, when awaiting a thenable. This appears to occur in all major versions of Node.js with async_hooks available. See the code example and output for reference.

Within the then function on the thenable object, the trigger id is 0. Note that this does not happen when awaiting a thenable as the first await within an async function.

cc @nodejs/diagnostics

Code example
const async_hooks = require('async_hooks')

const asyncHook = async_hooks.createHook({
  init (asyncId, type, triggerAsyncId) {
    log({
      event: 'init',
      asyncId,
      type,
      triggerAsyncId
    })
  },

  before (asyncId) {
    log({
      event: 'before',
      asyncId
    })
  },

  after (asyncId) {
    log({
      event: 'after',
      asyncId
    })
  }
})
asyncHook.enable()

function log (data) {
  process._rawDebug(JSON.stringify(data))
}

function tick () {
  return new Promise(setImmediate)
}

async function main () {
  await tick()
  await {
    then (fn) {
      log({
        executionAsyncId: async_hooks.executionAsyncId(),
        triggerAsyncId: async_hooks.triggerAsyncId(),
        message: 'these ids should not be zero'
      })
      setImmediate(fn)
    }
  }
}

main()
Output
{"event":"init","asyncId":2,"type":"PROMISE","triggerAsyncId":1}
{"event":"init","asyncId":3,"type":"PROMISE","triggerAsyncId":1}
{"event":"init","asyncId":4,"type":"Immediate","triggerAsyncId":1}
{"event":"init","asyncId":5,"type":"PROMISE","triggerAsyncId":3}
{"event":"before","asyncId":4}
{"event":"after","asyncId":4}
{"event":"before","asyncId":5}
{"event":"init","asyncId":6,"type":"PROMISE","triggerAsyncId":2}
{"event":"init","asyncId":7,"type":"PROMISE","triggerAsyncId":6}
{"event":"after","asyncId":5}
{"executionAsyncId":0,"triggerAsyncId":0,"message":"these ids should not be zero"}
{"event":"init","asyncId":8,"type":"Immediate","triggerAsyncId":0}
{"event":"before","asyncId":8}
{"event":"after","asyncId":8}
{"event":"before","asyncId":7}
{"event":"after","asyncId":7}
@Qard Qard added the async_hooks Issues and PRs related to the async hooks subsystem. label May 7, 2019
@BridgeAR BridgeAR added the feature request Issues that request new features to be added to Node.js. label May 7, 2019
@BridgeAR
Copy link
Member

BridgeAR commented May 7, 2019

AFAIK there has never been support for thenables and thus I marked this as feature request.

@Qard
Copy link
Member Author

Qard commented May 7, 2019

Seems to me like a requirement for async_hooks to actually be useful. There are many popular libraries that use the thenable pattern. In my particular case, we ran into knex using thenables internally, but in theory any userland promise implementation should suffer from the same problem.

@Qard
Copy link
Member Author

Qard commented May 7, 2019

Adding diag-agenda to discuss if this should be considered a bug or feature request at the next meeting. Let me know if you disagree.

@Qard Qard added the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label May 7, 2019
@devsnek
Copy link
Member

devsnek commented May 7, 2019

related to/duplicate of?

#26064
#22360

@kjin
Copy link
Contributor

kjin commented May 7, 2019

Yes, this is the same issue as I observed in #26064.

@devsnek
Copy link
Member

devsnek commented May 7, 2019

Duplicate of #26064

@devsnek devsnek marked this as a duplicate of #26064 May 7, 2019
@devsnek devsnek closed this as completed May 7, 2019
@devsnek devsnek removed the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label May 7, 2019
@devsnek
Copy link
Member

devsnek commented May 7, 2019

@Qard i deduplicated this, #22360 has the most conversation on this so i made that the diag-agenda item

@Qard
Copy link
Member Author

Qard commented May 7, 2019

Yes, this appears to be a duplicate. I do still think this needs to be brought up in the diagnostics meeting though to figure out how to proceed. The AsyncResource fix is valid, but the fact that async_hooks continues to be considered experimental discourages the community from using it and therefore means a huge chunk of the ecosystem will straight up break async_hooks currently.

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. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

4 participants