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: proper handling for AH in runInAsyncScope #30965

Closed

Conversation

apapirovski
Copy link
Member

We should never try to manually run emitAfter in case of an error,
the exception handler will do it for us, if we're going to recover.

This PR expands on the fix in #30087

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

We should never try to manually run emitAfter in case of an error,
the exception handler will do it for us, if we're going to recover.
@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Dec 14, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 15, 2019
Trott pushed a commit that referenced this pull request Dec 17, 2019
We should never try to manually run emitAfter in case of an error,
the exception handler will do it for us, if we're going to recover.

PR-URL: #30965
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 17, 2019

Landed in 503900b

@Trott Trott closed this Dec 17, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
We should never try to manually run emitAfter in case of an error,
the exception handler will do it for us, if we're going to recover.

PR-URL: #30965
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
We should never try to manually run emitAfter in case of an error,
the exception handler will do it for us, if we're going to recover.

PR-URL: #30965
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
We should never try to manually run emitAfter in case of an error,
the exception handler will do it for us, if we're going to recover.

PR-URL: #30965
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@MylesBorins
Copy link
Contributor

This seems to have broken some things, more specifically it seems related to #31783.

I'm reverting it from v12.x and we should reland when the fix has been on current and the reported bugs are fixed

addaleax pushed a commit that referenced this pull request Feb 14, 2020
The exception handler user-defined will not automatically emit after
for the async resource.

Also removes a duplicated case
`test-emit-after-uncaught-exception-runInAsyncScope.js`
which is identical to test-emit-after-uncaught-exception.js.

Refs: #30965
PR-URL: #31784
Fixes: #31783
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Feb 14, 2020
The exception handler user-defined will not automatically emit after
for the async resource.

Also removes a duplicated case
`test-emit-after-uncaught-exception-runInAsyncScope.js`
which is identical to test-emit-after-uncaught-exception.js.

Refs: #30965
PR-URL: #31784
Fixes: #31783
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
The exception handler user-defined will not automatically emit after
for the async resource.

Also removes a duplicated case
`test-emit-after-uncaught-exception-runInAsyncScope.js`
which is identical to test-emit-after-uncaught-exception.js.

Refs: #30965
PR-URL: #31784
Fixes: #31783
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants