Skip to content
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.

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

promises: more robust stringification #13784

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

Fixes: #13771

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

promises

@bnoordhuis
Copy link
Member

Swallowing exceptions seems like an - wait for it - exceptionally bad idea to me. The test case in #13771 works the way I would expect it to.

@mscdex mscdex added promises Issues and PRs related to ECMAScript promises. process Issues and PRs related to the process subsystem. labels Jun 19, 2017
@jasnell
Copy link
Member

jasnell commented Jun 19, 2017

I'm not too keen on this either. I'd almost prefer to go in the other direction and just make it:

const warning = new Error('Unhandled promise rejection ' +
                          `(rejection id: ${uid}): ${reason}`);

@TimothyGu
Copy link
Member Author

@bnoordhuis While I would generally agree with that sentiment, this case is a special one, since the thrown error is uncatchable.

@jasnell That would mean reverting #11640.

@bnoordhuis
Copy link
Member

All the more reason to let it bubble up. If .toString() throws an exception, something is probably deeply wrong with the application. Moving on as if nothing happened is just bad.

The more I think about it, the more I'm -1.

'use strict';
const common = require('../common');

const expectedDeprecationWarning = 'Unhandled promise rejections are ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should test this part, the below is good.

@benjamingr
Copy link
Member

All the more reason to let it bubble up. If .toString() throws an exception, something is probably deeply wrong with the application. Moving on as if nothing happened is just bad.

The issue isn't that toString threw an exception - is that Node using your code to give you a warning invoked toString which threw an error.

If no node side promise hooks were involved - the code wouldn't throw and the server wouldn't crash. Here, adding instrumentation can crash the server which is not a great idea.

@benjamingr
Copy link
Member

I would prefer to have something like <toString() threw exception ${ex}> with an error instance that preserves both the original and new error.

That said, what we should do is align with browser behavior (I don't think this is good enough reason to diverge) - did anyone check what chrome's hooks do?

@TimothyGu
Copy link
Member Author

@benjamingr

I would prefer to have something like <toString() threw exception ${ex}>

Here, you are coercing ex to a string again… and so you see the challenge.

did anyone check what chrome's hooks do?

Chrome's Dev Tools offers to inspect the object and doesn't invoke any methods to serialize it.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I’m good with this change for the reasons @benjamingr mentioned

@benjamingr
Copy link
Member

Here, you are coercing ex to a string again… and so you see the challenge.

@TimothyGu I'm sorry if it wasn't clear from my comment, but I'm +1 on these changes and the pull request.

It is unreasonable to swallow exceptions when the user causes them but it is unacceptable to crash the process for something the user didn't do for our internal tooling

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making my -1 explicit so it's not overlooked.

Swallowing arbitrary exceptions is tantamount to process.on('uncaughtException', () => {}), it leaves the process in an undefined state.

It's bad enough when users do it. It has no place in core.

@addaleax
Copy link
Member

@nodejs/ctc

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am -0 on this matter, as swallowing the exception does not give me the reason for that exception and that stacktrace. So it would total to 2 exceptions that should have crashed my process and didn't.

However this does make the situation somewhat better, but I'd prefer to have node 9 enforcing that deprecation, which would lead to this code being removed anyway.

In case anyone is interested, I wrote this: https://github.com/mcollina/make-promises-safe

try {
return savedString(value);
} catch (err) {
return '<toString() threw exception>';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please include the message of that error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina Keep in mind that err can be any JS value, and practically any operation we do with it can lead to yet another exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok for crashing badly in that case. It's always better to crash in those situations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If even the error thrown can not be stringified safely then IMO it's reasonable to crash with what we've got. Not including the error message makes it more confusing than what it is now if you have no idea whose toString() is throwing...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can a promise be rejected with that can't either be stringified in some way or util/inspect-ed?

I suppose if toString() throws we could either inspect the object or just <Object>.prototype.toString()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 A promise can be rejected with any JS value, and inspecting that is not foolproof.

Also, since we have [Symbol.toStringTag], Object.prototype.toString() may throw as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message, as it is, will not be clear to the users. Maybe something like, "stringifying promise rejection reason failed" would be better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thefourtheye The final error message already contains the information that it’s about promise rejection reasons, so that would be duplicating a lot. Also, I think V8 reports the same literal string in similar scenarios.

@benjamingr
Copy link
Member

benjamingr commented Jun 22, 2017

I want to point out that this is not swallowing an error, this is Node's instrumentation causing an error and then swallowing that internal error.

If we didn't have the promise hooks in place the error would not occur in the first place. It just happens to be an error. If no exception was thrown would that help with your objection @bnoordhuis ? (We can check explicitly for what error is)

  • Check if it's an object.
  • If it's not - String it and return.
  • Check if it is an Error object.
  • If it is, assume it has a stack and print it and its stack.
  • If it's not, go over the keys and print the keys (does util.inspect do that?). This is what chrome does.

However, I think that's way more complicated than checking if it threw an exception and catching that - it's an exception we're throwing and we're catching and I'm having a really hard time coming with a realistic case where I'd prefer to crash the user's server for something they didn't do.

@mcollina
Copy link
Member

However, I think that's way more complicated than checking if it threw an exception and catching that - it's an exception we're throwing and we're catching and I'm having a really hard time coming with a realistic case where I'd prefer to crash the user's server for something they didn't do.

We are not throwing that exception, a user custom toString() method is. IMHO we should not call toString() at all. util.inspect might be a good idea.

I would note that we are already swallowing an error and not crashing. It's the full point of 'unhandledRejection' and this deprecation. It's always better to crash vs swallowing/not handling an exception, as swallowing can make file descriptors and other native resources leak.

@TimothyGu
Copy link
Member Author

util.inspect might be a good idea.

Unfortunately util.inspect may crash also because it calls property getters that may throw. Heck, even Object.prototype.toString might throw if the Symbol.toStringTag property is implemented as a getter. Unlike Dev Tools, we are simply not in a position to print anything about the object without worrying about side effects. In this light, toString is the best we can do other than not trying to do anything about the object more than printing rejection value: object, which would be a great shame as most of promise rejection values are just plain Error objects.

@mcollina
Copy link
Member

I like the proposal @benjamingr did in #13784 (comment). It's complex, but I can see that topic coming up more and more often.

@bnoordhuis
Copy link
Member

If no exception was thrown would that help with your objection

If you mean inspecting the value in a way that no user code is invoked, making it impossible for exceptions to occur - yes, that's a good approach.

@bnoordhuis
Copy link
Member

I'm removing the ctc-review label, please add it back if you think that is a mistake.

@addaleax
Copy link
Member

addaleax commented Aug 1, 2017

@bnoordhuis @nodejs/ctc I do think we should come to a conclusion on this, and I do think this is the right thing to do, so I’m adding it to the CTC agenda.

@hashseed
Copy link
Member

hashseed commented Dec 19, 2017

git blame says it was last touched in 2015, and if memory serves me correctly we had that long before that.

@MylesBorins
Copy link
Contributor

@TimothyGu this will need to be manually backported if we are going to land on v6.x. Please lmk if you want to do this, as an alternative we can decide not to land

@bnoordhuis
Copy link
Member

does anyone know which version of v8 v8::Value::ToDetailString()was introduced in?

I think it's been in V8 since its first public release. I know for a fact it was in node.js v0.0.1!

@TimothyGu
Copy link
Member Author

TimothyGu commented Dec 19, 2017

This should definitely go into v6.x. But, I'll have limited time and availability over the break so it would be great if someone else could take care of this.

TimothyGu added a commit to TimothyGu/node that referenced this pull request Dec 23, 2017
PR-URL: nodejs#13784
Fixes: nodejs#13771
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@TimothyGu
Copy link
Member Author

Backported to v6.x in #17833.

MylesBorins pushed a commit that referenced this pull request Jan 18, 2018
Backport-PR-URL: #17833
PR-URL: #13784
Fixes: #13771
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
Backport-PR-URL: #17833
PR-URL: #13784
Fixes: #13771
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 11, 2018
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
Backport-PR-URL: #17833
PR-URL: #13784
Fixes: #13771
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 12, 2018
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
Backport-PR-URL: #17833
PR-URL: #13784
Fixes: #13771
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 13, 2018
This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins added a commit that referenced this pull request Feb 13, 2018
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MylesBorins added a commit that referenced this pull request Feb 13, 2018
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    nodejs#12678
* crypto:
  - expose ECDH class (Bryan English)
    nodejs#8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    nodejs#10209
  - warn on invalid authentication tag length (Tobias Nießen)
    nodejs#17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    nodejs#16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    nodejs#7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    nodejs#13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    nodejs#13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    nodejs#16386
* net:
  - return this from getConnections() (Sam Roberts)
    nodejs#13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    nodejs#13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    nodejs#14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    nodejs#16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    nodejs#12087
  - add process.ppid (cjihrig)
    nodejs#16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    nodejs#12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    nodejs#15179
* url:
  - WHATWG URL api support (James M Snell)
    nodejs#7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    nodejs#10308

PR-URL: nodejs#18342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhandled promise rejection with throwing toString crashes node