FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: expand on promises and async_hooks by ofrobots · Pull Request #18540 · nodejs/node · GitHub

/ node Public

doc: expand on promises and async_hooks - #18540

Merged
ofrobots merged 1 commit into
nodejs:masterfrom
ofrobots:fix-18520
Feb 5, 2018
ConversationCommits (1)ChecksFiles changed
Merged

doc: expand on promises and async_hooks#18540
ofrobots merged 1 commit into
nodejs:masterfrom
ofrobots:fix-18520

Conversation

ofrobots commented Feb 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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 mentioned this pull request Feb 2, 2018
Closed
vsemozhetbyt added the promises Issues and PRs related to ECMAScript promises. label Feb 3, 2018
jasnell reviewed Feb 3, 2018
View reviewed changes
Comment thread doc/api/async_hooks.md Outdated

jasnell Feb 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

an asynchronous hop?

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

kjin reviewed Feb 3, 2018
View reviewed changes
Comment thread doc/api/async_hooks.md Outdated

kjin Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Link doesn't seem to work.

kjin Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also execuationAsyncIds -> executionAsyncIds in the previous line.

vsemozhetbyt Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks. Fixed.

Comment thread doc/api/async_hooks.md Outdated

kjin Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🐯

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed all the 🐯s.

Comment thread doc/api/async_hooks.md Outdated

kjin Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Feb 3, 2018
View reviewed changes
Comment thread doc/api/async_hooks.md Outdated

vsemozhetbyt Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

instrospection -> introspection

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

Comment thread doc/api/async_hooks.md Outdated

vsemozhetbyt Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

tigger -> trigger

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

Comment thread doc/api/async_hooks.md Outdated

vsemozhetbyt Feb 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems this will be #async_hooks_promise_execution_tracking

ofrobots Feb 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, done.

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 merged commit d4b605b into nodejs:master Feb 5, 2018

ofrobots commented Feb 5, 2018

Copy link
Copy Markdown
Contributor Author

Thanks. Landed in d4b605b.

ofrobots deleted the fix-18520 branch February 5, 2018 17:43
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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
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
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 mentioned this pull request Feb 21, 2018
Merged
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
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 mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+2 more reviewers
kjin kjin left review comments
vsemozhetbyt vsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL