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: buffer bootstrap events #29848

Closed
wants to merge 10 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 4, 2019

This provides async_hooks support for an asynchronous Node.js core bootstrap. This is necessary for async_hooks events emitted during the async experimental modules bootstrap to have their init events properly emitted, as userland code can't attach to those otherwise.

The approach taken is to buffer the internal bootstrap async_hooks events, and then re-emit these buffered events as soon as there is an attachment to async_hooks during the initial sync execution of the module execution. As soon as the initial sync execution has completed, the buffer is cleared and async_hooks is disabled if no listeners were attached. When using the --no-force-async-hooks-checks flag this buffering is entirely disabled.

A bootstrap boolean flag is also provided to the init hook to be able to check which async hooks correspond to bootstrap events or not.

This work is a prerequisite to --experimental-modules unflagging (as in #29866).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 4, 2019
@devsnek
Copy link
Member

devsnek commented Oct 4, 2019

Are these events only needed for tests?

@guybedford
Copy link
Contributor Author

@devsnek it's more about getting the right architectural model of how async hooks interact with an async bootstrap. The ES module test failures are just a symptom of not having it worked out.

@devsnek
Copy link
Member

devsnek commented Oct 5, 2019

@guybedford I'm just curious why these events specifically need to be buffered. I assume random user code doesn't need to trace node internals?

@guybedford
Copy link
Contributor Author

It's necessary to do the buffering for async hooks user code to have a complete picture of all the async hooks events going on.

@jasnell
Copy link
Member

jasnell commented Oct 5, 2019

For various diagnostic tools having those early events would be ideal.

@@ -107,7 +107,7 @@ specifics of all functions that can be passed to `callbacks` is in the
const async_hooks = require('async_hooks');

const asyncHook = async_hooks.createHook({
init(asyncId, type, triggerAsyncId, resource) { },
init(asyncId, type, triggerAsyncId, resource, bootstrap) { },
Copy link
Member

Choose a reason for hiding this comment

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

The other way we could do this would be to use a special trigger ID for bootstrap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By special ID do you mean something like making it -1 for the bootstrap?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, something along those lines. Rather than negative tho, since Node.js is the one assigning the numbers, we could just say that a triggerAsyncId of 1 === bootstrap.

@guybedford guybedford mentioned this pull request Oct 7, 2019
14 tasks
@guybedford
Copy link
Contributor Author

I can confirm that this PR fixes all the async hooks tests for the unflagging PR. These changes that go with this PR can be seen at d85bb91.

@jasnell
Copy link
Member

jasnell commented Oct 7, 2019

The buffering definitely addresses the issue of catching the init events that happen before user code has an opportunity to attach a handler but there are a couple of concerns we need to be sure about before landing:

  1. What is the additional performance impact of adding the buffering. If it's significant, we may consider adding a command line option to disable it on startup.
  2. As I mention in the other comments, we need to decide if adding the bootstrap argument or reserving a trigger ID value is best.
  3. We should do some testing with some existing code that is using async hooks to make sure this doesn't break anything.

@guybedford
Copy link
Contributor Author

guybedford commented Oct 7, 2019

@jasnell thanks for the review.

  1. What is the additional performance impact of adding the buffering. If it's significant, we may consider adding a command line option to disable it on startup.

Which benchmarks should we be checking here?

  1. As I mention in the other comments, we need to decide if adding the bootstrap argument or reserving a trigger ID value is best.

One thing that may be an issue is that I was noticing from the bootstrap the asyncTriggerId was ending up at 29 (due to nesting of promises?). It seems if we want to make 1 the ID for core triggers that might not naturally work with the current approach to incrementing asyncTriggerId?

  1. We should do some testing with some existing code that is using async hooks to make sure this doesn't break anything.

Do you have some code examples that would apply to this. I can tell you now that it will most certainly be breaking, but once a bootstrap event filter is added (like I had to do in all the tests), it would not be breaking.

@guybedford guybedford added the async_hooks Issues and PRs related to the async hooks subsystem. label Oct 7, 2019
@guybedford
Copy link
Contributor Author

Another thing that is worth noting here is that while in the CommonJS case the async hooks are being fully disabled so these tests pass, in the ES modules case, the disabling of the hook causes a corruption of the async hooks stack if it is done synchronously (as in the comment in disableHook). So this PR is setting the disable to still happen asynchronously before execution resulting in the hooks still being attached at execution time (and still buffering).

The alternative sync disable would need to dive into handling that async hooks stack corruption case directly.

@jasnell would value your advice on this as well. I can confirm that the above summarizes all the issues in this PR, which are the only remaining technical blockers to unflag experimental modules.

@guybedford
Copy link
Contributor Author

One thing that may be an issue is that I was noticing from the bootstrap the asyncTriggerId was ending up at 29 (due to nesting of promises?). It seems if we want to make 1 the ID for core triggers that might not naturally work with the current approach to incrementing asyncTriggerId?

Alternatively we could just enforce the getDefaultTriggerAsyncId function to return 1 during bootstrap. Let me know if that sounds sensible.

@guybedford
Copy link
Contributor Author

Closing as we were able to make the async bootstrap apply lazily only when needed for the unflagging PR. We could possibly come back to an approach like this for the 13 / 14 major though. Happy to discuss further, but will close for now to reduce noise.

@guybedford guybedford closed this Oct 11, 2019
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. 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.

4 participants