Skip to content

process: ignore asyncId 0 in exception handler - #41424

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-fix-async-id-0
Closed

process: ignore asyncId 0 in exception handler#41424
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-fix-async-id-0

Conversation

@apapirovski

Copy link
Copy Markdown
Contributor

Today, the global uncaught exception handler is the only place where asyncId 0 is not ignored and we still proceed to call emitAfter. This would've already failed one of our correctness tests in async_hooks if not for some other code meant to handle a different edge case.

Fixes: #22982

Tests failing before the fix:

=== release test-timers.setInterval === Path: async-hooks/test-timers.setInterval
Error: Found a handle whose after hook was invoked but not its init hook
at ActivityCollector._getActivity (/Users/apapirovski/Web/nodejs/test/async-hooks/init-hooks.js:169:19)
at ActivityCollector._after (/Users/apapirovski/Web/nodejs/test/async-hooks/init-hooks.js:200:20)
at emitHook (node:internal/async_hooks:233:38)
at emitAfterScript (node:internal/async_hooks:520:5)
at process._fatalException (node:internal/process/execution:192:9)
node:assert:123
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Checking invocations at stage "t1: when process exits":
Called "before" 1 time(s), but expected 2 invocation(s).
at checkHook (/Users/apapirovski/Web/nodejs/test/async-hooks/hook-checks.js:51:14)
at Array.forEach (<anonymous>)
at checkInvocations (/Users/apapirovski/Web/nodejs/test/async-hooks/hook-checks.js:28:62)
at process.<anonymous> (/Users/apapirovski/Web/nodejs/test/async-hooks/test-timers.setInterval.js:44:3)
at process.emit (node:events:532:35)
at process.exit (node:internal/process/per_thread:183:15)
at fatalError (node:internal/async_hooks:174:11)
at emitHook (node:internal/async_hooks:237:5)
at emitAfterScript (node:internal/async_hooks:520:5)
at process._fatalException (node:internal/process/execution:192:9) {
generatedMessage: false,
code: 'ERR_ASSERTION',
actual: 1,
expected: 2,
operator: 'strictEqual'
}
Node.js v18.0.0-pre
Command: out/Release/node /Users/apapirovski/Web/nodejs/test/async-hooks/test-timers.setInterval.js
=== release test-unhandled-exception-valid-ids === Path: async-hooks/test-unhandled-exception-valid-ids
Error: Found a handle whose after hook was invoked but not its init hook
at ActivityCollector._getActivity (/Users/apapirovski/Web/nodejs/test/async-hooks/init-hooks.js:169:19)
at ActivityCollector._after (/Users/apapirovski/Web/nodejs/test/async-hooks/init-hooks.js:200:20)
at emitHook (node:internal/async_hooks:233:38)
at emitAfterScript (node:internal/async_hooks:520:5)
at process._fatalException (node:internal/process/execution:192:9)
Command: out/Release/node /Users/apapirovski/Web/nodejs/test/async-hooks/test-unhandled-exception-valid-ids.js

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jan 7, 2022
Comment threadlib/internal/process/execution.js Outdated
Today, the global uncaught exception handler is the only
place where asyncId 0 is not ignored and we still proceed
to call emitAfter. This would've already failed one of
our correctness tests in async_hooks if not for some other
code meant to handle a different edge case.
Fixes: nodejs#22982
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 8, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@aduh95aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 17, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@FlarnaFlarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2022
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 18, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/41424
✔ Done loading data for nodejs/node/pull/41424
----------------------------------- PR info ------------------------------------
Title process: ignore asyncId 0 in exception handler (#41424)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch apapirovski:patch-fix-async-id-0 -> nodejs:master
Labels process, author ready, needs-ci
Commits 1
- process: ignore asyncId 0 in exception handler
Committers 1
- Anatoli Papirovski PR-URL: https://github.com/nodejs/node/pull/41424
Fixes: https://github.com/nodejs/node/issues/22982
Fixes: https://github.com/===
Reviewed-By: Gerhard Stöbich Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41424
Fixes: https://github.com/nodejs/node/issues/22982
Fixes: https://github.com/===
Reviewed-By: Gerhard Stöbich Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --------------------------------------------------------------------------------
ℹ This PR was created on Fri, 07 Jan 2022 06:08:00 GMT
✔ Approvals: 3
✔ - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/41424#pullrequestreview-847081629
✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/41424#pullrequestreview-847144468
✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41424#pullrequestreview-854777313
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2022-01-18T17:57:04Z: https://ci.nodejs.org/job/node-test-pull-request/42003/
- Querying data for job/node-test-pull-request/42003/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ No git cherry-pick in progress
✔ No git am in progress
✔ No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
* branch master -> FETCH_HEAD
✔ origin/master is now up-to-date
- Downloading patch for 41424
From https://github.com/nodejs/node
* branch refs/pull/41424/merge -> FETCH_HEAD
✔ Fetched commits as 81e88f27b719..6ecf9413bdaa
--------------------------------------------------------------------------------
[master c8803e9c75] process: ignore asyncId 0 in exception handler
Author: Anatoli Papirovski Date: Thu Jan 6 22:04:30 2022 -0800
3 files changed, 24 insertions(+), 5 deletions(-)
create mode 100644 test/async-hooks/test-unhandled-exception-valid-ids.js
✔ Patches applied
--------------------------------------------------------------------------------
⚠ Found Fixes: https://github.com/nodejs/node/issues/22982, skipping..
--------------------------------- New Message ----------------------------------
process: ignore asyncId 0 in exception handler

Today, the global uncaught exception handler is the only
place where asyncId 0 is not ignored and we still proceed
to call emitAfter. This would've already failed one of
our correctness tests in async_hooks if not for some other
code meant to handle a different edge case.

Fixes: #22982

PR-URL: #41424
Fixes: https://github.com/===
Reviewed-By: Gerhard Stöbich deb2001-github@yahoo.de
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com

[master 38f41892f8] process: ignore asyncId 0 in exception handler
Author: Anatoli Papirovski apapirovski@mac.com
Date: Thu Jan 6 22:04:30 2022 -0800
3 files changed, 24 insertions(+), 5 deletions(-)
create mode 100644 test/async-hooks/test-unhandled-exception-valid-ids.js
✖ 38f41892f8f7443d23e9d39a947742fafc158720
✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer
✔ 7:7 Valid fixes URL. fixes-url
✖ 10:7 Fixes must be a GitHub URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 9:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/1714616087

Flarna pushed a commit that referenced this pull request Jan 18, 2022
Today, the global uncaught exception handler is the only
place where asyncId 0 is not ignored and we still proceed
to call emitAfter. This would've already failed one of
our correctness tests in async_hooks if not for some other
code meant to handle a different edge case.
Fixes: #22982
PR-URL: #41424
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Flarna

Copy link
Copy Markdown
Member

Landed in 5178332

@FlarnaFlarna closed this Jan 18, 2022
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
Today, the global uncaught exception handler is the only
place where asyncId 0 is not ignored and we still proceed
to call emitAfter. This would've already failed one of
our correctness tests in async_hooks if not for some other
code meant to handle a different edge case.
Fixes: #22982
PR-URL: #41424
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadornoruyadorno mentioned this pull request Feb 8, 2022
danielleadams pushed a commit that referenced this pull request Feb 26, 2022
Today, the global uncaught exception handler is the only
place where asyncId 0 is not ignored and we still proceed
to call emitAfter. This would've already failed one of
our correctness tests in async_hooks if not for some other
code meant to handle a different edge case.
Fixes: #22982
PR-URL: #41424
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
Today, the global uncaught exception handler is the only
place where asyncId 0 is not ignored and we still proceed
to call emitAfter. This would've already failed one of
our correctness tests in async_hooks if not for some other
code meant to handle a different edge case.
Fixes: #22982
PR-URL: #41424
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-failedAn error occurred while landing this pull request using GitHub Actions.needs-ciPRs that need a full CI run.processIssues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[async_hooks] Wrong after callback in case of uncaughtException

5 participants

@apapirovski@nodejs-github-bot@Flarna@jasnell@aduh95