lib,src: "throw" on unhandled promise rejections #6375
Conversation
|
For background on why, please see nodejs/promises#26 (comment) |
|
Ugh a rebase screwed up. I'll deal with it later |
tools/test.py
Outdated
There was a problem hiding this comment.
Should be in a separate PR. Sometimes the test runner will swallow startup errors here.
9e07e01 to
b49931d
Compare
|
Ugh, guess I rebased onto a newer v8, now all the WeakCallback stuff is broken... 😞 |
|
@Fishrock123 Don’t think so… If I read this correctly, the compiler is right, |
src/track-promise.cc
Outdated
There was a problem hiding this comment.
You're using the deprecated SetWeak API. In the non-deprecated version, the weak object is already garbage collected by the time the weak callback gets called. In other words, you won't be able to get your hands onto the promise object in the weak callback.
There was a problem hiding this comment.
I have word that this won't actually be removed remotely soon?
There was a problem hiding this comment.
I would strongly encourage you to use the new SetWeak API to make sure this feature is implementable on top of the new API. The old APIs have been deprecated for a long time switched over to imminently deprecated in V8 4.9. /cc @jeisinger.
There was a problem hiding this comment.
@ofrobots You just said this isn't possible with that API though?
I need access to the promise "reason", at very least in an associable way to storage of the "reason" (the object that is rejected).
There was a problem hiding this comment.
Is it possible for you to get and save the promise reason before the weak callback gets invoked? If so, this should be implementable using the new API.
There was a problem hiding this comment.
Basically, at the time you need to create the TrackPromise object, you need to keep a reference to the reason in the TrackPromise object. Later when the weak callback gets called, you should look up the reason object you hold in the TrackPromise C++ object rather than depending upon the promise object being available.
The other thing you will need to ensure (@jeisinger may know this off the top of his head) is that the reason object doesn't hold a reference to the promise.
There was a problem hiding this comment.
I hacked a quick patch to switch you over to the new SetWeak API: ofrobots@d1e98e8
There was a problem hiding this comment.
btw, I added WeakCallbackType::kFinalizer to the WeakCallbackInfo<> API to preserve the old behavior here https://codereview.chromium.org/1883173002
There was a problem hiding this comment.
@nodejs/v8 Looks like this old API was removed, could someone point me to how to use the new replacement API? If it is easier to guide me on something like IRC that would totally work too. (Sorry C++ are very rusty!)
There was a problem hiding this comment.
I don't have too much context on the PR but this patch https://github.com/matthewloring/node/commit/f740100758db9de6d20531feba7e512d64a54f37 should get things compiling again.
|
@addaleax Interesting... seems to work, I don't really understand why though. |
b49931d to
6a54c3c
Compare
|
@Fishrock123 When you use a non-static |
6a54c3c to
9bb24e6
Compare
|
Updated, nits addressed, more tests added, Fixed more stuff: https://ci.nodejs.org/job/node-test-pull-request/2391/ |
55a3103 to
7e2a08c
Compare
|
Also, is this really semver-major? It would make my life significantly easier if it wasn't. |
|
The change itself seems fine to me but I'll leave the sign off to others who know the details of Promises and uncaught exceptions more than I do. Given the likelihood that this will break existing code that depend on the rejections being swallowed, I'm hesitant to make this anything but major. I'm also hesitant on landing this so soon before cutting v6. But I certainly won't object if @nodejs/CTC are +1 |
ea2d9e8 to
8470ed1
Compare
|
ping again @nodejs/v8 - can anyone help me with issues related to the new I'd really like to at least have this working properly again before I open up a new PR for visibility. |
|
@Fishrock123 What is the question? |
|
@bnoordhuis Either: why the new API doesn't fire the
|
Refs: nodejs#5292 Refs: nodejs/promises#26 Refs: nodejs#6355 PR-URL: nodejs#6375
8470ed1 to
8a2e21a
Compare
does not work compiles segfaults
|
I can take a look soon. FWIW in v8's debugger we also have an event for when the promise has been collected, also building on top of weak handles. Look in src/debug/debug.cc for SendAsyncTaskEventCancel. |
Sorry, I really don't understand what is going on there. :( |
|
@Fishrock123 a6a36a5...21e5bb5 works and you can pull it in here if you like the approach (if you do please remove my debug code 😄) |
|
Closing this in favor of opening a new pull request. Will link shortly. |
|
I have filed a new pull request at #12010, if you are still interested or against this, please go there. |
Refs: nodejs#5292 Refs: nodejs/promises#26 Refs: nodejs#6355 PR-URL: nodejs#6375
Refs: nodejs#12010 Refs: nodejs#5292 Refs: nodejs/promises#26 Refs: nodejs#6355 PR-URL: nodejs#6375
src: use std::map for the promise reject map Refs: nodejs#5292 Refs: nodejs/promises#26 Refs: nodejs#6355 Refs: nodejs#6375
Checklist
Affected core subsystem(s)
promises or something
Description of change
Refs: #5292
Refs: nodejs/promises#26
Refs: #6355
Makes Promises "throw" rejections which exit like regular uncaught errors. However, it only happens if:
Please take a look @nodejs/ctc @nodejs/promises @benjamingr @chrisdickinson etc