Skip to content

lib: trigger uncaught exception handler for microtasks #23794

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
Merged
merged 1 commit into from
Oct 23, 2018
Merged

lib: trigger uncaught exception handler for microtasks #23794

merged 1 commit into from
Oct 23, 2018

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Oct 21, 2018

@jasnell this should make 11.0.0

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests are updated
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1322/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 21, 2018
@devsnek devsnek requested a review from addaleax October 21, 2018 03:17
addaleax
addaleax approved these changes Oct 21, 2018
View reviewed changes
@devsnek
Copy link
Member Author

devsnek commented Oct 21, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/18017/

@devsnek
Copy link
Member Author

devsnek commented Oct 21, 2018

arm rebuild https://ci.nodejs.org/job/node-test-commit-arm/19363/

@devsnek devsnek added lib / src Issues and PRs related to general changes in the lib or src directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 21, 2018
@devsnek devsnek added this to the 11.0.0 milestone Oct 21, 2018
@mmarchini
Copy link
Contributor

Should also update relevant docs:

https://github.com/nodejs/node/blob/master/doc/api/globals.md#queuemicrotaskcallback

diff --git doc/api/globals.md doc/api/globals.md
index e4965a43af..b46ef6f08a 100644
--- doc/api/globals.md
+++ doc/api/globals.md
@@ -119,8 +119,8 @@ added: REPLACEME
 * `callback` {Function} Function to be queued.

 The `queueMicrotask()` method queues a microtask to invoke `callback`. If
-`callback` throws an exception, the [`process` object][] `'error'` event will
-be emitted.
+`callback` throws an exception, the [`process` object][] `'uncaughtException'`
+event will be emitted.

 In general, `queueMicrotask` is the idiomatic choice over `process.nextTick()`.
 `process.nextTick()` will always run before the microtask queue, and so

@mmarchini
Copy link
Contributor

This (and the previous behavior) doesn't cope well with --abort-on-uncaught-exception (expected the process to generate a core dump, but nothing happened). I'm guessing this will change if https://bugs.chromium.org/p/v8/issues/detail?id=8326 is fixed, but in the meantime we might want to abort on FatalException if it came from a microtask and --abort-on-uncaught-exception is set? I can do this in a follow-up Pull Request.

@devsnek devsnek force-pushed the fix/unhandled-in-microtask branch from 17a1415 to 6a0f394 Compare October 22, 2018 14:25
@devsnek
Copy link
Member Author

devsnek commented Oct 22, 2018

@mmarchini all fixed up

@devsnek

This comment has been minimized.

Sign in to view
@devsnek

This comment has been minimized.

Sign in to view
@devsnek devsnek force-pushed the fix/unhandled-in-microtask branch 2 times, most recently from b389143 to cab0d86 Compare October 22, 2018 17:05
@devsnek
Copy link
Member Author

devsnek commented Oct 22, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/18047/

@devsnek
Copy link
Member Author

devsnek commented Oct 22, 2018

windows rebuild https://ci.nodejs.org/job/node-test-commit-windows-fanned/21699/
linux rebuild https://ci.nodejs.org/job/node-test-commit-linux/22570/

@devsnek
Copy link
Member Author

devsnek commented Oct 22, 2018

ci is a bit weird, linking my build to an unrelated one, but as far as i can tell everything is now green.

@devsnek devsnek mentioned this pull request Oct 23, 2018
Closed
4 tasks
@devsnek devsnek force-pushed the fix/unhandled-in-microtask branch from cab0d86 to 2caf079 Compare October 23, 2018 18:05
@devsnek
Copy link
Member Author

devsnek commented Oct 23, 2018 •
edited
Loading

landed in 2caf079

@devsnek
lib: trigger uncaught exception handler for microtasks
2caf079 
PR-URL: nodejs#23794
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@devsnek devsnek merged commit 2caf079 into nodejs:master Oct 23, 2018
@devsnek devsnek deleted the fix/unhandled-in-microtask branch October 23, 2018 18:08
targos pushed a commit that referenced this pull request Oct 24, 2018
@devsnek @targos
lib: trigger uncaught exception handler for microtasks
22cd537 
PR-URL: #23794
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
@MylesBorins
Copy link
Contributor

Should this land on 8.x or 10.x? If so it will require a manual backport

@devsnek devsnek added dont-land-on-v6.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v10.x labels Nov 26, 2018
@devsnek
Copy link
Member Author

devsnek commented Nov 26, 2018

@MylesBorins this patches behaviour in a semver-major feature (global.queueMicrotask)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@mmarchini mmarchini mmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@devsnek @nodejs-github-bot @mmarchini @MylesBorins @jasnell @addaleax @cjihrig