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.

Sign up for GitHub

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

Jump to bottom

doc: expand on promises and async_hooks #18540

Merged
merged 1 commit into from
Feb 5, 2018
Merged

doc: expand on promises and async_hooks #18540

merged 1 commit into from
Feb 5, 2018

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Feb 2, 2018 •
edited
Loading

AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

Fixes: #18520

Checklist
Affected core subsystem(s)

doc:async_hooks

/cc @nodejs/async_hooks @nodejs/diagnostics
EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request-lite/146/

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. labels Feb 2, 2018
@ofrobots ofrobots mentioned this pull request Feb 2, 2018
Closed
@vsemozhetbyt vsemozhetbyt added the promises Issues and PRs related to ECMAScript promises. label Feb 3, 2018
jasnell
jasnell reviewed Feb 3, 2018
View reviewed changes
doc/api/async_hooks.md Outdated
```

Observe that the `then` callback claims to have executed in the context of the
outer scope even though there was asynchronous hop involved. Also note that the
Copy link
Member

Choose a reason for hiding this comment

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

an asynchronous hop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

kjin
kjin reviewed Feb 3, 2018
View reviewed changes
doc/api/async_hooks.md Outdated
@@ -509,6 +509,9 @@ const server = net.createServer(function onConnection(conn) {
});
```

Note that promise contexts may not get precise execuationAsyncIds by default.
See the section on [promise execution tracking][#promise_execution_tracking].
Copy link
Contributor

Choose a reason for hiding this comment

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

Link doesn't seem to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also execuationAsyncIds -> executionAsyncIds in the previous line.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this should be [promise execution tracking][]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

doc/api/async_hooks.md Outdated

Observe that the `then` callback claims to have executed in the context of the
outer scope even though there was asynchronous hop involved. Also note that the
tiggerAsyncId value is 0, which means that we are missing context about the
Copy link
Contributor

Choose a reason for hiding this comment

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

🐯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed all the 🐯s.

doc/api/async_hooks.md Outdated
promise with asyncId 7. This promise was triggered by async resource 6.

Another subtlety with promises is that `before` and `after` callbacks are run
only on chained promises. That means promises not created by `then` will not
Copy link
Contributor

Choose a reason for hiding this comment

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

This is because callbacks passed to then (or catch; I think you should add that) are added to the Promise micro-task queue, right? Maybe mention that callbacks passed to then are different than the ones passed to the Promise constructor because they will not run synchronously from where then is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added mention of catch callbacks. The reason is actually due to the implementation choice in PromiseHooks. It was entirely possible for V8 to implement things in a way that promises created by new or Promise.resolve() etc. would get before / after callbacks too. For the tradeoffs used, see this doc.

vsemozhetbyt
vsemozhetbyt reviewed Feb 3, 2018
View reviewed changes
doc/api/async_hooks.md Outdated
## Promise execution tracking

By default, promise executions are not assigned asyncIds due to the relatively
expensive nature of the [promise instrospection API][PromiseHooks] provided by
Copy link
Contributor

Choose a reason for hiding this comment

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

instrospection -> introspection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

doc/api/async_hooks.md Outdated
By default, promise executions are not assigned asyncIds due to the relatively
expensive nature of the [promise instrospection API][PromiseHooks] provided by
V8. This means that programs using promises or `async`/`await` will not get
correct execution and tigger ids for promise callback contexts by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

tigger -> trigger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

doc/api/async_hooks.md Outdated
@@ -655,3 +709,5 @@ constructor.
[`destroy` callback]: #async_hooks_destroy_asyncid
[`init` callback]: #async_hooks_init_asyncid_type_triggerasyncid_resource
[Hook Callbacks]: #async_hooks_hook_callbacks
[PromiseHooks]: https://docs.google.com/document/d/1rda3yKGHimKIhg5YeoAmCOtyURgsbTH_qaYR79FELlk
[promise execution tracking]: #promise_execution_tracking
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this will be #async_hooks_promise_execution_tracking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

@ofrobots
doc: expand on promises and async_hooks
d4b605b 
AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

PR-URL: nodejs#18540
Fixes: nodejs#18520
Reviewed-By: James M Snell <jasnell@gmail.com>
@ofrobots ofrobots merged commit d4b605b into nodejs:master Feb 5, 2018
@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 5, 2018

Thanks. Landed in d4b605b.

@ofrobots ofrobots deleted the fix-18520 branch February 5, 2018 17:43
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@ofrobots @MylesBorins
doc: expand on promises and async_hooks
53094da 
AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

PR-URL: #18540
Fixes: #18520
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@ofrobots @MylesBorins
doc: expand on promises and async_hooks
722e33e 
AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

PR-URL: #18540
Fixes: #18520
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@ofrobots @MylesBorins
doc: expand on promises and async_hooks
2418c86 
AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

PR-URL: #18540
Fixes: #18520
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
@ofrobots @gibfahn
doc: expand on promises and async_hooks
25db460 
AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

PR-URL: #18540
Fixes: #18520
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ofrobots @MayaLekova
doc: expand on promises and async_hooks
757fa4e 
AsyncHooks have a few subtleties with being able to track promises.
This commit adds a section to the docs that explains things the issues.

PR-URL: nodejs#18540
Fixes: nodejs#18520
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjin kjin kjin left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

async_hooks: promise callbacks have no context unless init hook present
6 participants
@ofrobots @jasnell @kjin @vsemozhetbyt @gibfahn @nodejs-github-bot