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: fix AsyncLocalStorage in unhandledRejection cases #41202

Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Dec 16, 2021

Right now async local storage doesn't properly propagate through unhandledRejections. This fixes that.

@bmeck bmeck added confirmed-bug Issues with confirmed bugs. promises Issues and PRs related to ECMAScript promises. async_hooks Issues and PRs related to the async hooks subsystem. labels Dec 16, 2021
@bmeck bmeck requested review from Qard and vdeturckheim December 16, 2021 18:46
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Dec 16, 2021
@bmeck bmeck added the wip Issues and PRs that are still a work in progress. label Dec 17, 2021
@bmeck
Copy link
Member Author

bmeck commented Dec 17, 2021

there is some kind of bug with the async id stack with this PR, DO NOT MERGE yet

@bmeck
Copy link
Member Author

bmeck commented Dec 17, 2021

@Qard and I did some poking and it seems some other stuff is likely messed up around here #41222 , I cannot write a test that covers all the branches of unhandledRejection + uncaughtExceptionMonitor because of it.

Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

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

LGTM with the call stack issues to be fixed. This would actually help me for implementing an idea I have for #40954

@vdeturckheim
Copy link
Member

@bmeck does that mean only the tests are broken and not the actual feature?

@bmeck
Copy link
Member Author

bmeck commented Dec 17, 2021

@vdeturckheim the feature isn't fully fixed it looks like, if you register uncaughtExceptionMonitor things look like they aren't correct. unhandledRejection seems to work fine with this PR. Registering both events gets all sort of broken (regardless of this PR).

@bmeck
Copy link
Member Author

bmeck commented Dec 20, 2021

@Qard please re-review

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Looks to be some lint errors, but after those are resolved should be good to go.

test/parallel/test-async-wrap-pop-id-during-load.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bmeck bmeck added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Dec 21, 2021
@bmeck
Copy link
Member Author

bmeck commented Dec 21, 2021

Landed in c3866b0

@bmeck bmeck closed this Dec 21, 2021
bmeck added a commit that referenced this pull request Dec 21, 2021
PR-URL: #41202
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41202
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
PR-URL: #41202
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41202
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41202
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. confirmed-bug Issues with confirmed bugs. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants