promise: Add --throw-unhandled-rejection and --trace-unhandled-rejection #6355

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@yosuke-furukawa
Member

yosuke-furukawa commented Apr 23, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

promise

Description of change

We are writing a lots of Promise modules and test these modules in our development environment. When process.on('unhandledRejection') error is occurred, default node behavior does not send any message, no warnings, no exceptions. So we have to prepare the following code and add tests.

process.on('unhandledRejection', (reason) => {
  throw reason;
});

This is not so easy for Promise / Node beginner. So I would like to propose the following 2 options.

  1. --throw-unhandled-rejection option will throw Exception when no unhandledRejection listener
  2. --trace-unhandled-rejection option will output stderr when no unhandledRejection listener

I know the default unhandledRejection behavior is discussing here
nodejs/promises#26

But I would like to add these options to help our developments.

@addaleax

View changes

test/parallel/test-throw-default-error-unhandled-rejections.js
+if (process.argv[2] === 'child') {
+ const rejectPromise = () => {
+ return new Promise((resolve, reject) => {
+ setTimeout(reject(new Error('Reject Promise')), 100);

This comment has been minimized.

@addaleax

addaleax Apr 23, 2016

Member

If you want to have the reject() call delayed, you need to wrap it in a closure, i.e. setTimeout(() => reject(new Error('Reject Promise')), 100);. Right now, the whole child block is equivalent to Promise.reject(new Error('Reject Promise')).

@addaleax

addaleax Apr 23, 2016

Member

If you want to have the reject() call delayed, you need to wrap it in a closure, i.e. setTimeout(() => reject(new Error('Reject Promise')), 100);. Right now, the whole child block is equivalent to Promise.reject(new Error('Reject Promise')).

This comment has been minimized.

@yosuke-furukawa

yosuke-furukawa Apr 24, 2016

Member

Thanks, I fixed it.

@yosuke-furukawa

yosuke-furukawa Apr 24, 2016

Member

Thanks, I fixed it.

@addaleax

View changes

lib/internal/process/promises.js
@@ -44,6 +47,14 @@ function setupPromises(scheduleMicrotasks) {
if (!process.emit('unhandledRejection', reason, promise)) {
// Nobody is listening.
// TODO(petkaantonov) Take some default action, see #830
+
+ if (traceUnhandledRejection && reason.stack) {

This comment has been minimized.

@addaleax

addaleax Apr 23, 2016

Member

reason can be of any type, including null. How abount printing reason.stack when reason && reason.stack and otherwise just reason itself?

@addaleax

addaleax Apr 23, 2016

Member

reason can be of any type, including null. How abount printing reason.stack when reason && reason.stack and otherwise just reason itself?

This comment has been minimized.

@yosuke-furukawa

yosuke-furukawa Apr 24, 2016

Member

Sounds good. fixed.

@yosuke-furukawa

yosuke-furukawa Apr 24, 2016

Member

Sounds good. fixed.

@addaleax

View changes

test/parallel/test-trace-default-error-unhandled-rejections.js
+ assert(errorMessage.match(/Reject Promise/));
+ }));
+ child.on('exit', common.mustCall((code) => {
+ assert(code === 0);

This comment has been minimized.

@addaleax

addaleax Apr 23, 2016

Member

assert.strictEqual(code, 0);?

@addaleax

addaleax Apr 23, 2016

Member

assert.strictEqual(code, 0);?

This comment has been minimized.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
Member

ChALkeR commented Apr 24, 2016

promise: Add --throw-unhandled-rejection and --trace-unhandled-reject…
…ion option

--throw-unhandled-rejection option can throw Exception when no
unhandledRejection listener

--trace-unhandled-rejection option can output error on stderr
when no unhandledRejection listener
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Apr 24, 2016

Member

I'm -1 on this change.

We should not be adding flags for these things - the reason we have an "abort on uncaught exceptions" flag is because of core dumps.

What we should be doing is defaulting to logging these errors rather than silently swallowing them - like browsers and other languages do.

Member

benjamingr commented Apr 24, 2016

I'm -1 on this change.

We should not be adding flags for these things - the reason we have an "abort on uncaught exceptions" flag is because of core dumps.

What we should be doing is defaulting to logging these errors rather than silently swallowing them - like browsers and other languages do.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 24, 2016

Member

Don't throws have the same stacktrace?

The behavior should really just throw by default as I have mentioned before. Ideally after checking in a more deterministic way. (e.g. GC)

In lieu of that, logging could be fine. I won't be able to finish the GC idea for v6, that's for sure.

Member

Fishrock123 commented Apr 24, 2016

Don't throws have the same stacktrace?

The behavior should really just throw by default as I have mentioned before. Ideally after checking in a more deterministic way. (e.g. GC)

In lieu of that, logging could be fine. I won't be able to finish the GC idea for v6, that's for sure.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Apr 24, 2016

Member

@Fishrock123 great, can we maybe bring logging by default to a vote and can I count on your vote with the caveat that we can might instead throw on GC collection?

Member

benjamingr commented Apr 24, 2016

@Fishrock123 great, can we maybe bring logging by default to a vote and can I count on your vote with the caveat that we can might instead throw on GC collection?

@yosuke-furukawa

This comment has been minimized.

Show comment
Hide comment
@yosuke-furukawa

yosuke-furukawa Apr 24, 2016

Member

@benjamingr @Fishrock123
Logging by default is enough for my situation.
I am preparing to send a next PR, here.

https://github.com/nodejs/node/compare/master...yosuke-furukawa:promise-logging-default-unhandled-rejection?expand=1

If no one has a strong objection to logging messages, I will close this PR and send next PR.

Member

yosuke-furukawa commented Apr 24, 2016

@benjamingr @Fishrock123
Logging by default is enough for my situation.
I am preparing to send a next PR, here.

https://github.com/nodejs/node/compare/master...yosuke-furukawa:promise-logging-default-unhandled-rejection?expand=1

If no one has a strong objection to logging messages, I will close this PR and send next PR.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 24, 2016

Member

I'm getting the GC and processes end handlers working. Standby..

Member

Fishrock123 commented Apr 24, 2016

I'm getting the GC and processes end handlers working. Standby..

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 25, 2016

Member

Updated #5292 ... still working on one last point. PTAL.

Member

Fishrock123 commented Apr 25, 2016

Updated #5292 ... still working on one last point. PTAL.

@Fishrock123 Fishrock123 referenced this pull request Apr 25, 2016

Closed

lib,src: "throw" on unhandled promise rejections #6375

3 of 4 tasks complete
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 25, 2016

Member

#6375 should probably supersede this.

Member

Fishrock123 commented Apr 25, 2016

#6375 should probably supersede this.

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 25, 2016

@yosuke-furukawa

This comment has been minimized.

Show comment
Hide comment
@yosuke-furukawa

yosuke-furukawa Apr 28, 2016

Member

I think my PR is finished its role, some following PRs are created and discussion is started, so I close this PR.
#6439
#6375

Member

yosuke-furukawa commented Apr 28, 2016

I think my PR is finished its role, some following PRs are created and discussion is started, so I close this PR.
#6439
#6375

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Dec 16, 2016

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Feb 10, 2017

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 28, 2017

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 28, 2017

BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 31, 2017

lib,src: throw on unhanded promise rejections
src: use std::map for the promise reject map

Refs: nodejs#5292
Refs: nodejs/promises#26
Refs: nodejs#6355
Refs: nodejs#6375

BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 26, 2017

lib,src: throw on unhanded promise rejections
src: use std::map for the promise reject map

Refs: nodejs#5292
Refs: nodejs/promises#26
Refs: nodejs#6355
Refs: nodejs#6375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment