Skip to content

lib: trigger uncaught exception handler for microtasks#23794

Merged
devsnek merged 1 commit intonodejs:masterfrom
devsnek:fix/unhandled-in-microtask
Oct 23, 2018
Merged

lib: trigger uncaught exception handler for microtasks#23794
devsnek merged 1 commit intonodejs:masterfrom
devsnek:fix/unhandled-in-microtask

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

nodejs-github-bot commented Oct 21, 2018

@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

mmarchini commented Oct 22, 2018

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

mmarchini commented Oct 22, 2018

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 26, 2018

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