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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Apr 25, 2016 •
edited by estliberitas
Loading

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)

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:

  • An unhandled promise is GC'd
  • Unhandled promises are not handled by the time the process exits normally.

Please take a look @nodejs/ctc @nodejs/promises @benjamingr @chrisdickinson etc

@Fishrock123 Fishrock123 added semver-major PRs that contain breaking changes and should be released in the next major version. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 25, 2016
@Fishrock123 Fishrock123 added this to the 6.0.0 milestone Apr 25, 2016
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 25, 2016
This was referenced Apr 25, 2016
Closed
Closed
@Fishrock123
Copy link
Contributor Author

For background on why, please see nodejs/promises#26 (comment)

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

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Apr 25, 2016 •
edited
Loading

Ugh a rebase screwed up. I'll deal with it later

indutny
indutny reviewed Apr 25, 2016
View reviewed changes
tools/test.py Outdated
@@ -1527,6 +1527,7 @@ def Main():
vmArch = archEngineContext.stdout.rstrip()
if archEngineContext.exit_code is not 0 or vmArch == "undefined":
print "Can't determine the arch of: '%s'" % vm
print archEngineContext.stderr.rstrip()
Copy link
Member

Choose a reason for hiding this comment

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

Rationale behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be in a separate PR. Sometimes the test runner will swallow startup errors here.

@Fishrock123 Fishrock123 force-pushed the promise-unhandled-default branch from 9e07e01 to b49931d Compare April 25, 2016 18:36
@Fishrock123
Copy link
Contributor Author

Ugh, guess I rebased onto a newer v8, now all the WeakCallback stuff is broken... 😞

../src/track-promise.cc:35:29: error: reference to non-static member function must be called
  persistent_.SetWeak(this, WeakCallback);
                            ^~~~~~~~~~~~
../src/track-promise.h:20:15: note: possible target for call
  inline void WeakCallback(
              ^
../src/track-promise.h:24:15: note: possible target for call
  inline void WeakCallback(v8::Isolate* isolate, v8::Local<v8::Object> object);
              ^
1 error generated.

@addaleax
Copy link
Member

@Fishrock123 Don’t think so… If I read this correctly, the compiler is right, TrackPromise::WeakCallback(const WeakCallbackData<Object, TrackPromise>& data) should be static?

ofrobots
ofrobots reviewed Apr 25, 2016
View reviewed changes
src/track-promise.cc Outdated

void TrackPromise::WeakCallback(
const WeakCallbackData<Object, TrackPromise>& data) {
data.GetParameter()->WeakCallback(data.GetIsolate(), data.GetValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have word that this won't actually be removed remotely soon?

Copy link
Contributor

@ofrobots ofrobots Apr 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ofrobots ofrobots Apr 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hacked a quick patch to switch you over to the new SetWeak API: ofrobots@d1e98e8

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, I added WeakCallbackType::kFinalizer to the WeakCallbackInfo<> API to preserve the old behavior here https://codereview.chromium.org/1883173002

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!)

Copy link

Choose a reason for hiding this comment

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

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.

@Fishrock123
Copy link
Contributor Author

@addaleax Interesting... seems to work, I don't really understand why though.

@Fishrock123 Fishrock123 force-pushed the promise-unhandled-default branch from b49931d to 6a54c3c Compare April 25, 2016 18:53
@Fishrock123
Copy link
Contributor Author

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

@addaleax
Copy link
Member

@Fishrock123 When you use a non-static WeakCallback, it would like to have a this object it can refer to when it’s being executed, and C++ doesn’t allow this (in a simple way). https://isocpp.org/wiki/faq/pointers-to-members explains this quite extensively, if you’re interested.

@Fishrock123 Fishrock123 force-pushed the promise-unhandled-default branch from 6a54c3c to 9bb24e6 Compare April 25, 2016 21:11
@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Apr 25, 2016 •
edited
Loading

Updated, nits addressed, more tests added, CI: https://ci.nodejs.org/job/node-test-pull-request/2390/

Fixed more stuff: https://ci.nodejs.org/job/node-test-pull-request/2391/

@Fishrock123 Fishrock123 force-pushed the promise-unhandled-default branch 2 times, most recently from 55a3103 to 7e2a08c Compare April 25, 2016 21:33
@Fishrock123
Copy link
Contributor Author

Also, is this really semver-major? It would make my life significantly easier if it wasn't.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2016

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

@Fishrock123 Fishrock123 mentioned this pull request Apr 25, 2016
Closed
@Fishrock123 Fishrock123 changed the title lib,src: throw on unhanded promise rejections lib,src: "throw" on unhanded promise rejections Apr 26, 2016
@Fishrock123 Fishrock123 force-pushed the promise-unhandled-default branch from ea2d9e8 to 8470ed1 Compare January 13, 2017 00:48
@Fishrock123
Copy link
Contributor Author

ping again @nodejs/v8 - can anyone help me with issues related to the new WeakCallbackInfo API and not receiving notification on GC from it?

I'd really like to at least have this working properly again before I open up a new PR for visibility.

@bnoordhuis
Copy link
Member

@Fishrock123 What is the question?

@Fishrock123
Copy link
Contributor Author

@bnoordhuis Either: why the new API doesn't fire the WeakCallbackInfo event/handle/callback at GC, or, why GC doesn't happen for promises here.

test/message/reject_promise.js is the example test that does not work correctly, and the relevant tracking code is all in src/track_promise.cc.

@LewisJEllis LewisJEllis mentioned this pull request Jan 26, 2017
Closed
@Fishrock123 Fishrock123 force-pushed the promise-unhandled-default branch from 8470ed1 to 8a2e21a Compare February 10, 2017 16:16
@hashseed
Copy link
Member

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.

@Fishrock123
Copy link
Contributor Author

Look in src/debug/debug.cc for SendAsyncTaskEventCancel.

Sorry, I really don't understand what is going on there. :(

@addaleax addaleax self-requested a review February 28, 2017 22:41
@addaleax
Copy link
Member

addaleax commented Mar 1, 2017

@Fishrock123 a6a36a5...21e5bb5 works and you can pull it in here if you like the approach (if you do please remove my debug code 😄)

@Timer Timer mentioned this pull request Mar 13, 2017
Closed
@Fishrock123
Copy link
Contributor Author

Closing this in favor of opening a new pull request. Will link shortly.

@Fishrock123 Fishrock123 mentioned this pull request Mar 23, 2017
Closed
4 tasks
@Fishrock123
Copy link
Contributor Author

I have filed a new pull request at #12010, if you are still interested or against this, please go there.

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 28, 2017
@Fishrock123
lib,src: throw on unhanded promise rejections
1f9635d 
Refs: nodejs#5292
Refs: nodejs/promises#26
Refs: nodejs#6355
PR-URL: nodejs#6375
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 28, 2017
@Fishrock123
lib,src: throw on unhanded promise rejections
f6e35fe 
Refs: nodejs#12010
Refs: nodejs#5292
Refs: nodejs/promises#26
Refs: nodejs#6355
PR-URL: nodejs#6375
@Fishrock123 Fishrock123 mentioned this pull request Apr 28, 2017
Closed
4 tasks
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Sep 26, 2017
@Fishrock123 @BridgeAR
lib,src: throw on unhanded promise rejections
711d699 
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
Reviewers

@matthewloring matthewloring matthewloring left review comments

@benjamingr benjamingr benjamingr left review comments

@ofrobots ofrobots ofrobots left review comments

@addaleax addaleax Awaiting requested review from addaleax

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. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.