promise: emit error on domain unhandled rejections #36082

benjamingr · 2020-11-11T12:53:39Z

Fixes #35232

Thanks for the help and guidance with this @addaleax

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

BridgeAR

Awesome work!

nodejs-github-bot · 2020-11-11T14:36:56Z

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

nodejs-github-bot · 2020-11-11T16:43:13Z

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

nodejs-github-bot · 2020-11-11T20:28:09Z

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

ExE-Boss · 2020-11-11T20:54:46Z

lib/internal/process/promises.js

@@ -266,10 +273,9 @@ function generateUnhandledRejectionError(reason) {
 function listenForRejections() {
  setPromiseRejectCallback(promiseRejectHandler);
 }
-


This line probably shouldn’t be deleted.

ExE-Boss · 2020-11-11T20:55:51Z

lib/internal/process/promises.js

@@ -117,7 +117,8 @@ function unhandledRejection(promise, reason) {
  maybeUnhandledPromises.set(promise, {
    reason,
    uid: ++lastPromiseId,
-    warned: false
+    warned: false,
+    domain: process.domain


Since this PR is adding trailing commas to module.exports = {…}, it should probably also add them here:

Suggested change

domain: process.domain

domain: process.domain,

We don't actually enforce (or particularly care) about trailing commas I believe?

I believe that if we do we should enforce it through eslint.

nodejs-github-bot · 2020-11-12T11:58:53Z

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

github-actions · 2020-11-16T08:53:20Z

Commit Queue failed

- Loading data for nodejs/node/pull/36082 ✔ Done loading data for nodejs/node/pull/36082 ----------------------------------- PR info ------------------------------------ Title promise: emit error on domain unhandled rejections (#36082) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch benjamingr:domain-unhandled-rejection-promise -> nodejs:master Labels domain, process, promises, repl Commits 3 - promise: emit error on domain unhandled rejections - fixup! behaviour change, remove test for old behaviour - fixup! linter Committers 1 - Benjamin Gruenbaum PR-URL: https://github.com/nodejs/node/pull/36082 Reviewed-By: Ruben Bridgewater Reviewed-By: Yongsheng Zhang Reviewed-By: Rich Trott ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36082 Reviewed-By: Ruben Bridgewater Reviewed-By: Yongsheng Zhang Reviewed-By: Rich Trott -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-11-12T11:58:53Z: https://ci.nodejs.org/job/node-test-pull-request/34362/ - Querying data for job/node-test-pull-request/34362/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Wed, 11 Nov 2020 12:53:39 GMT ✔ Approvals: 3 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/36082#pullrequestreview-528154304 ✔ - Yongsheng Zhang (@ZYSzys): https://github.com/nodejs/node/pull/36082#pullrequestreview-528159397 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36082#pullrequestreview-529098695 -------------------------------------------------------------------------------- ✔ 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 36082 From https://github.com/nodejs/node * branch refs/pull/36082/merge -> FETCH_HEAD ✔ Fetched commits as 2fd22353ea1a..f234181e18da -------------------------------------------------------------------------------- [master ccad277770] promise: emit error on domain unhandled rejections Author: Benjamin Gruenbaum Date: Wed Nov 11 14:52:07 2020 +0200 2 files changed, 24 insertions(+), 8 deletions(-) [master ea8f10ba73] fixup! behaviour change, remove test for old behaviour Author: Benjamin Gruenbaum Date: Wed Nov 11 16:00:00 2020 +0200 1 file changed, 24 deletions(-) [master 5b471c6ed2] fixup! linter Author: Benjamin Gruenbaum Date: Wed Nov 11 16:26:43 2020 +0200 1 file changed, 1 deletion(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
promise: emit error on domain unhandled rejections

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 1a1f1b4da6] promise: emit error on domain unhandled rejections
Author: Benjamin Gruenbaum [email protected]
Date: Wed Nov 11 14:52:07 2020 +0200
2 files changed, 24 insertions(+), 8 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! behaviour change, remove test for old behaviour

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 986cf1e314] fixup! behaviour change, remove test for old behaviour
Author: Benjamin Gruenbaum [email protected]
Date: Wed Nov 11 16:00:00 2020 +0200
1 file changed, 24 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! linter

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 560bb24300] fixup! linter
Author: Benjamin Gruenbaum [email protected]
Date: Wed Nov 11 16:26:43 2020 +0200
1 file changed, 1 deletion(-)

Successfully rebased and updated refs/heads/master.
✖ 1a1f1b4da6938cb81bfe0e67363bddca501a6b4d
✔ 0:0 skipping fixes-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
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Invalid subsystem: "promise" subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ 986cf1e3145e1cad30b2585efee57d21aeb5b1b6
✔ 0:0 skipping fixes-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
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length
✖ 560bb24300578ecdddc1d364eb72383de95acdaa
✔ 0:0 skipping fixes-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
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. 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.

Commit Queue action: https://github.com/nodejs/node/actions/runs/365652327

benjamingr · 2020-11-16T08:54:27Z

Ok, TIL commit queue doesn't squash 😅 I'll land manually

Edit: landed (cleanly) with NCU in cd31340

PR-URL: #36082 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos · 2020-11-16T09:15:32Z

Commit queue squashes if you use the standard fixup! commit messages

benjamingr · 2020-11-16T09:39:10Z

@targos I did use fixup! in the commit message, see output here: #36082 (comment)

targos · 2020-11-16T10:55:13Z

It's not just fixup!. The text after it must be exactly the same as the title of the commit that your are fixing. This is what git automatically generates if you run git commit --fixup REF -m "some message" and is necessary so that git knows where to put the fixup commit in the interactive rebase session

richardlau · 2020-11-16T10:55:23Z

@targos I did use fixup! in the commit message, see output here: #36082 (comment)

@benjamingr For fixup! commits to work in git the rest of the message should match that of an earlier commit so that git can work out which commit is being fixed up. So for example if you wanted to fixup a commit that had commit message promise: emit error on domain unhandled rejections the fixup! commits should be titled fixup! promise: emit error on domain unhandled rejections (see documentation for --autosquash in https://git-scm.com/docs/git-rebase).

FWIW the reason #36082 (comment) failed was not the autosquash -- promise is not currently a valid subsystem.

benjamingr · 2020-11-16T12:14:08Z

Thanks @targos @richardlau, I didn't know that or autosquash! I just assumed the fixup! gets picked up by the interactive rebase. I've just checked it and that's cool - TIL :]

PR-URL: #36082 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos · 2021-05-16T14:00:32Z

This lands cleanly on v14.x-staging but the test fails:

$ ./node test/parallel/test-domain-promise.js
/Users/targos/git/nodejs/v14.x/test/common/index.js:616
const crashOnUnhandledRejection = (err) => { throw err; };
                                             ^

Error: foo
    at Domain.<anonymous> (/Users/targos/git/nodejs/v14.x/test/parallel/test-domain-promise.js:136:20)
    at Domain.<anonymous> (/Users/targos/git/nodejs/v14.x/test/common/index.js:376:15)
    at Domain.run (domain.js:373:15)
    at Object.<anonymous> (/Users/targos/git/nodejs/v14.x/test/parallel/test-domain-promise.js:135:5)
    at Module._compile (internal/modules/cjs/loader.js:1084:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1113:10)
    at Module.load (internal/modules/cjs/loader.js:949:32)
    at Function.Module._load (internal/modules/cjs/loader.js:789:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47

benjamingr · 2021-05-16T16:55:14Z

@targos yes, the test is rightfully failing and should be removed since this is specifically a behaviour change that breaks the test.

promise: emit error on domain unhandled rejections

67e4966

benjamingr added repl Issues and PRs related to the REPL subsystem. promises Issues and PRs related to ECMAScript promises. labels Nov 11, 2020

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Nov 11, 2020

benjamingr added the domain Issues and PRs related to the domain subsystem. label Nov 11, 2020

benjamingr requested a review from BridgeAR November 11, 2020 13:05

BridgeAR approved these changes Nov 11, 2020

View reviewed changes

ZYSzys approved these changes Nov 11, 2020

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020

fixup! behaviour change, remove test for old behaviour

f6f23e5

nodejs deleted a comment from nodejs-github-bot Nov 11, 2020

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020

fixup! linter

f234181

nodejs deleted a comment from nodejs-github-bot Nov 11, 2020

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020

ExE-Boss reviewed Nov 11, 2020

View reviewed changes

Trott approved these changes Nov 12, 2020

View reviewed changes

benjamingr added request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 16, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 16, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 16, 2020

benjamingr added a commit that referenced this pull request Nov 16, 2020

promise: emit error on domain unhandled rejections

cd31340

PR-URL: #36082 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]>

benjamingr closed this Nov 16, 2020

benjamingr deleted the domain-unhandled-rejection-promise branch November 16, 2020 08:58

codebytere pushed a commit that referenced this pull request Nov 22, 2020

promise: emit error on domain unhandled rejections

eb9295b

PR-URL: #36082 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]>

codebytere mentioned this pull request Nov 22, 2020

v15.3.0 proposal #36232

Merged

wentwrong mentioned this pull request Nov 26, 2020

fix(server-tests): errored tests after Node 15.3.0 update DevExpress/testcafe#5744

Merged

benjamingr mentioned this pull request Dec 10, 2020

executionAsyncId doesn't return expected value in context of process.on('unhandledRejection') #26794

Closed

benjamingr mentioned this pull request Feb 6, 2021

asyncId for unhandled rejected Promise #37244

Closed

targos added the backport-requested-v14.x label May 16, 2021

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

promise: emit error on domain unhandled rejections #36082

promise: emit error on domain unhandled rejections #36082

benjamingr commented Nov 11, 2020

BridgeAR left a comment

nodejs-github-bot commented Nov 11, 2020

nodejs-github-bot commented Nov 11, 2020

nodejs-github-bot commented Nov 11, 2020

ExE-Boss Nov 11, 2020

ExE-Boss Nov 11, 2020

benjamingr Nov 16, 2020

nodejs-github-bot commented Nov 12, 2020

github-actions bot commented Nov 16, 2020

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

benjamingr commented Nov 16, 2020 •

edited

Loading

targos commented Nov 16, 2020

benjamingr commented Nov 16, 2020

targos commented Nov 16, 2020 •

edited

Loading

richardlau commented Nov 16, 2020

benjamingr commented Nov 16, 2020 •

edited

Loading

targos commented May 16, 2021

benjamingr commented May 16, 2021

promise: emit error on domain unhandled rejections #36082

promise: emit error on domain unhandled rejections #36082

Conversation

benjamingr commented Nov 11, 2020

Checklist

BridgeAR left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 11, 2020

nodejs-github-bot commented Nov 11, 2020

nodejs-github-bot commented Nov 11, 2020

ExE-Boss Nov 11, 2020

Choose a reason for hiding this comment

ExE-Boss Nov 11, 2020

Choose a reason for hiding this comment

benjamingr Nov 16, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 12, 2020

github-actions bot commented Nov 16, 2020

PR-URL: #36082 Reviewed-By: Ruben Bridgewater [email protected] Reviewed-By: Yongsheng Zhang [email protected] Reviewed-By: Rich Trott [email protected]

PR-URL: #36082 Reviewed-By: Ruben Bridgewater [email protected] Reviewed-By: Yongsheng Zhang [email protected] Reviewed-By: Rich Trott [email protected]

PR-URL: #36082 Reviewed-By: Ruben Bridgewater [email protected] Reviewed-By: Yongsheng Zhang [email protected] Reviewed-By: Rich Trott [email protected]

benjamingr commented Nov 16, 2020 • edited Loading

targos commented Nov 16, 2020

benjamingr commented Nov 16, 2020

targos commented Nov 16, 2020 • edited Loading

richardlau commented Nov 16, 2020

benjamingr commented Nov 16, 2020 • edited Loading

targos commented May 16, 2021

benjamingr commented May 16, 2021

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #36082
Reviewed-By: Ruben Bridgewater [email protected]
Reviewed-By: Yongsheng Zhang [email protected]
Reviewed-By: Rich Trott [email protected]

benjamingr commented Nov 16, 2020 •

edited

Loading

targos commented Nov 16, 2020 •

edited

Loading

benjamingr commented Nov 16, 2020 •

edited

Loading