Proposal for hook into resolution attempts of already resolved/rejected promise

@MayaLekova

As discussed during the summit, V8 could provide a hook to notify the embedder (Node/Chrome) about swallowed promise rejections, specifically with the Promise constructor (see Use Case #3 in promise-use-cases). I've written a design document how to extend the V8 C++ API here.

cc @MayaLekova @benjamingr

The hook sounds good to me (Confirming Node.js can do it and that it is sufficient to address the use case in question and expose a hook from Node's side).

Regarding the API - I'm not sure it should be the same function since no actual promise rejection is happening .

It'd be convenient to re-use the existing C++ API, since it exposes everything we need. But that's not a requirement, we can also introduce a new C++ API if you think that's better.

@bmeurer

@bmeurer I think it's confusing to reuse the hook since at the moment it's only called when a promise rejects but I don't have very strong feelings about it.

Also cc @misterdjules who has raised this concern about promises before and @getify who raised this issue as well.

@benjamingr

@benjamingr sounds reasonable. I added another possible solution which would introduce a dedicated C++ hook just for this case.

Without the ability to differentiate between programmer errors (bugs that materialize as an uncaught exception) and operational errors (reject is called with an error object), I'm not sure it's going to be possible for the embedder or a developer to determine whether the process should crash or not.

Would it be possible to add the ability to differentiate between those two different use cases?

Would it be possible to add the ability to differentiate between those two different use cases?

Unfortunately programmer errors and operational errors in the Promise constructor are not always distinguishable by whether or not it's a throw or a reject. I've seen multiple projects throw in the Promise constructor for operational stuff explicitly when doing the research for the summit.

Note that this hook is specifically for multiple throws/rejects and the current hook for promise rejections doesn't create a distinction. The line gets even blurrier in async functions (although that's not this issue).

I would love discussing how we can better distinguish programmer/operational errors and be more helpful towards programmers. (Although I think that should be a separate issue IMO).

If you think there is value in the promise rejection hook exposing whether or not a promise was thrown and not rejected for APMs or debugging then that's also worth discussing (again, I believe in a separate issue).

If I understand it correctly - this issue is specifically about detecting multiple rejects or throws in the promise constructor and providing a hook for that that APMs can hook into or Node.js can warn on.

@bmeurer

Unfortunately programmer errors and operational errors in the Promise constructor are not always distinguishable by whether or not it's a throw or a reject. I've seen multiple projects throw in the Promise constructor for operational stuff explicitly when doing the research for the summit.

That's good to know. Do you have more details to share about that specific part of your research (how it was done, your findings, etc.)? I think it could be useful to include in one of the documents in this repository for future reference.

Note that this hook is specifically for multiple throws/rejects and the current hook for promise rejections doesn't create a distinction. The line gets even blurrier in async functions (although that's not this issue).

Ah you're right, I was confused by the title of the issue. Reading the design document it's now clear that this is about a very specific use case. @bmeurer Do you think renaming this issue as "Proposal for hook into already resolved/rejected promise" would help clarify this?

I agree in this case it's clear that there is a bug in the application and that developers/embedders can take appropriate action, including crashing.

I also agree that being able to differentiate between promises rejected due to programmer or operational error should be discussed in a different issue.

Thanks for the clarification @benjamingr!

@misterdjules

@misterdjules Added "Proposal for hook into already resolved/rejected promise" as subtitle to the design document. 👍

this issue is specifically about detecting multiple rejects or throws in the promise constructor

Is there a separate issue for catching exceptions that happened in the promise constructor without the use of throw, like calling doesNotExist()? That is absolutely an important use-case for me.

Unfortunately programmer errors and operational errors in the Promise constructor are not always distinguishable by whether or not it's a throw or a reject.

Certainly that line may be blurry. But the line that shouldn't be blurry is whether an exception came from an explicit throw or whether it came from a runtime mistake in the programming. THAT distinction is very important to me. If that's a different issue or hook, fine. But I just wanted to make sure that wasn't getting glossed over.

Is it also possible to resolve after rejecting, needing a

kPromiseResolvedAfterReject = 3

@mhdawson

@mhdawson I guess that'd be kPromiseResolveAfterResolved. The terminology is a bit odd here, but resolved for a promise actually means that it's no longer pending.

@benjamingr

Thanks for the ping @benjamingr.

I'm very much in favor of issuing a warning in these cases of double reject/resolve.

In my post on error handling in Promises, I'm hinting at how I would have ideally liked this to be designed, but I guess I don't explain it in too much detail.

I'm aware the following it's a fairly drastic proposal, but I'm not yet convinced it is crazy-talk. This will be a bit long, but I think it's relevant to the discussion.

My take on this is that the complexities we're talking about here are not going to be top-of-mind for 99% of programmers using Promises. They'll be unaware of the case that you can do both reject and resolve on the same Promise. They'll be unaware of when a Promise rejection can be considered unhandled. Etc. So most programmers will be blind to a bunch of problems that may happen.

The solution I propose is to throw away resolve/reject entirely, in favor of return/throw. Then you get language level support for there being only one of these happening for a given Promise. In other words, we stop doing this:

return new Promise((resolve, reject) => {
    if (thing.isAThing) { resolve(thing); }
    if (thing.hasErrors) { reject(thing.accumulatedErrors); }
})

And only do it like this:

return new Promise(() => {
    if (thing.isAThing) { return thing; }
    if (thing.hasErrors) { throw new Error(thing.accumulatedErrors); }
})

This removes a whole class of problems. But, it opens a new question: How would you implement rejects and resolves from async functions? I.e. how would you implement this:

return new Promise((resolve, reject) => {
    setTimeout(() => {
        resolve("ok");
    }, 100);
})

Here, I propose that this is done with a new variant of setTimeout, i.e:

return new Promise(() => {
    return await setTimeout2(() => {
        return "ok";
    }, 100);
})

Here setTimeout2(fn) has the semantics that:

It returns whatever fn returns.
It rethrows whatever fn throws.
It's awaitable

I initially thought this would need runtime support, but I think it's doable in user land. Then you don't need reject/resolve any more. There is a small number of use cases that go away. For example, you can't do this:

return new Promise((resolve, reject) => {
    button.onclick = () => resolve("button was clicked");
})

I think that's fine. There's a small loss of generality, in favor of a serious reduction in complexity. I think that's exactly what one should want from a design. It's exactly why array.forEach() is nicer than a straight for-loop, for example.

It's also clear that such a design would work, because it's exactly the design of the .NET Task Parallel Library.

The reduced complexity from this would be pretty massive. This entire discussion thread would be uneeded, and it seems like almost everything that is complex about Promises goes away.

It's a fairly radical proposal. If this was five years ago, and Promises was not implemented in browsers, I would argue strongly in its favor. Today, I still think it's a better design, but it's possible the cost of switching to it may be too high.

Actually removing resolve/reject seems like a no-go at this point, but perhaps one could move forward by downplaying resolve/reject in favor of using return/throw along with a new API for setTimeout().

Assuming everyone buys into this being a better design (big assumption), there are still some questions:

What conflicts, if any, exist with other parts of the current Promises API?
What's a good way to discourage use of reject/resolve? Issuing warnings when they are called for the first time?
Given that the reject/resolve design already exists, is it even worth it to push this other way of doing things?

@larspars

@larspars I think you're barging into an wide open door :)

The tl;dr; is that we agree, we want to provide APIs so that people never have to write new Promise and we want to provide hooks to improve debugging support for the promise constructor anyway just in case (like the case you raised in your article).

Ideally I want people to never have to use the promise constructor at all - that's why we have util.promisify and promise APIs. Ideally, people will use async functions and not "raw" promises in their code when possible and engines will provide better debugging tooling for those like async stack traces.

return new Promise(() => {
    if (thing.isAThing) { return thing; }
    if (thing.hasErrors) { throw new Error(thing.accumulatedErrors); }
})

Can literally be return and throw inside an async function.

let delay = util.promisify(setTimeout); // Added in Node 8
async function doFoo() {
   await delay(100);
   // can `throw` or `return` here.
}
doFoo();

I couldn't agree more with:

My take on this is that the complexities we're talking about here are not going to be top-of-mind for 99% of programmers using Promises. They'll be unaware of the case that you can do both reject and resolve on the same Promise. They'll be unaware of when a Promise rejection can be considered unhandled. Etc. So most programmers will be blind to a bunch of problems that may happen.

(We do intend to do a survey to validate it)

Ideally - just like in .NET where people don't use TaskCompletionSource but use APIs that already return promises. We already have this experimentally for fs and dns is coming next. We also have async iterator (for... await) support for streams. Hopefully in a year or two we'll have such APIs for all the relevant APIs in Node.

It is somewhat radical to provide promise APIs for everything - but given that's what people use and that's what the language does - we're still going to do it.

@benjamingr

@benjamingr Aha, sorry if I was barging in and stating what everyone was already thinking, haha. I haven't followed the discussions around this, but what you describe looks really nice, and looks like it would solve the problems I was talking about.

@larspars

@larspars if you have any other ideas please do let me know, personally I posted a canonical Q&A about it - proposed this hook in the Node collaborator summit, proposed .promisify (and pushed for it and bluebird before it was native) and am trying to promote the new promise APIs where I can.

I don't think that's enough (or that we're moving quickly enough) which is why I think a lot of people are running into the same issues you are (and aren't writing blog posts about them or are as willing to interact).

I personally don't believe we're in a very good place with promises and how we work with them (yet) - so any suggestions for hooks, debugging ideas etc. are welcome. Benedikt here is in V8 and while this is a node repository I would also happily connect you to relevant individuals in different browsers or the spec if you can prototype with hooks and provide feedback to implementors with CatchJS (or as an individual).

@benjamingr

@benjamingr Sweet, I'll look over your links and let you know if I think I have anything semi-intelligent to contribute :)

@larspars

@larspars sure, just don't assume we've thought of most of this stuff or have done so correctly - if you have ideas please do bring them up by opening an issue here or in nodejs/node

deps: cherry-pick 2075910 from upstream V8

deps: cherry-pick 907d7bc from upstream V8

process: add `multipleResolves` event

Navigation Menu

Repository navigation

Proposal for hook into resolution attempts of already resolved/rejected promise #8

71 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Proposal for hook into resolution attempts of already resolved/rejected promise #8

Description

Activity

benjamingr commented on Jun 13, 2018

bmeurer commented on Jun 13, 2018

benjamingr commented on Jun 13, 2018

bmeurer commented on Jun 13, 2018

misterdjules commented on Jun 13, 2018

benjamingr commented on Jun 13, 2018

misterdjules commented on Jun 13, 2018

bmeurer commented on Jun 13, 2018

getify commented on Jun 13, 2018

mhdawson commented on Jun 13, 2018

bmeurer commented on Jun 14, 2018

71 remaining items

larspars commented on Jul 29, 2018

benjamingr commented on Jul 29, 2018

larspars commented on Jul 29, 2018

benjamingr commented on Jul 29, 2018

larspars commented on Jul 29, 2018

benjamingr commented on Jul 29, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions