Switch from "Thenable Assimilation Procedure" to "Promise Resolution Procedure"

[domenic]

This was prompted by #87 and #88, and in particular the discussions about how you should only retrieve a reference to then once, and then call that same reference.

This necessitated refactoring from "if x is a thenable, run the Assimilation(thenable, promise); otherwise, fulfill promise with x"---which necessarily seems to involve a two-step process---to simply "run Resolve(promise, x)." I think it turned out rather nicely, actually.

A second commit tries to resolve the question of exception handling by treating something with a throwing getter for then as a non-thenable. It seems right to me, but I am willing to be persuaded in the other direction. I included it as a separate commit specifically because the refactoring is separate from the exact approach of how we handle throwing getters for then.

You can see the end result here.

[domenic]

Wasn't sure whether to nest this or not.

[briancavalier]

Seems ok to me not to nest it.

[briancavalier]

I like this change a lot, @domenic. Even though it seems like just terminology, I think not introducing a new concept of "assimilation" into the spec is a good thing. This may also help the "resolve"/"resolved" teminology situation a bit.

[domenic]

Awesome :). Indeed, I was thinking of adding definitions for "settled" and "resolved", but couldn't find a great place to put them.

[novemberborn]

One consequence of this procedure, or rather how it deals with .then getters, is that you cannot use an isThenable() method as the first time you inspect the thenable you need to cache the then value.

Nonetheless, I like it.

[briancavalier]

@novemberborn Yep, that's correct. Internally, the duck type test followed by a second access to thenable.then isn't really safe, unfortunately.

Interestingly, this means that a public isThenable method isn't actually safe for callers either, since it hides an access to then :/

[domenic]

Updated with the other exception handling strategy. Will merge in a few days if there are no objections, and re-start the ratification procedure.

[ForbesLindesay]

I've been a bit busy so haven't managed to follow this discussion very closely. The only slight issue I have with it is that when we introduce ES6 proxies, it might be very reasonable to construct a proxy which throws when you attempt to access any property that's not defined, which would lead to a rejection here, when you probably really want it to lead to the object not being treated as a promise.

[ForbesLindesay]

I'm not saying it's necessarily worth changing things, but it's worth considering.

[novemberborn]

(I implemented the previous wording of the procedure yesterday.)

I believe there is a distinction between determining whether a given value is a thenable, and adopting the state of potentially unreasonable thenables.

@erights wrote in #88:

don't drop thrown errors if there's a natural promise to break with that error as the reason

Observation should not lead to rejection. If we can't determine the type of the then property we should conclude the value is not a thenable.

If we are dealing with a thenable, however, we need to be able to deal with unreasonable thenables which:

1. Call resolvePromise or rejectPromise multiple times
2. Pass multiple arguments to resolvePromise or rejectPromise
3. Don't call resolvePromise or rejectPromise at all
4. Throw after calling resolvePromise or rejectPromise
5. Throw before calling resolvePromise or rejectPromise

The first two scenarios are easily dealt with. The third means the promise remains pending, which we can interpret as the thenable remaining pending. In the fourth scenario we drop the error. The fifth scenario is interesting. It seems reasonable to assume resolvePromise or rejectPromise will never be called, and hence we reject with the thrown error.

If we really cared about not dropping thrown errors we would still reject the promise in the fourth scenario. Similarly with the promises-aplus/constructor-spec#18 proposal.

[erights]

@novemberborn I agree that (4) drops a thrown error. However, this does not contradict the text of mine you quoted. There is no "natural promise to break", since the returned promise has already been resolved by the call to resolvePromise or rejectPromise.

But I take seriously your

Observation should not lead to rejection. If we can't determine the type of the then property
we should conclude the value is not a thenable.

This makes sense too. I am now truly undecided and puzzled.

[novemberborn]

@erights:

I agree that (4) drops a thrown error. However, this does not contradict the text of mine you quoted. There is no "natural promise to break", since the returned promise has already been resolved by the call to resolvePromise or rejectPromise.

I'd almost say that's cheating ;-) If a "natural promise to break" is only a non-resolved promise then why are we so concerned about not dropping errors?

I want to avoid leaving promises in a hard-to-debug pending state because thenable.then throws or the resolver function crashes. In these scenarios we should assume the promise will never resolve normally. Rejecting with the thrown error is the next best option, assuming authors log unexpected rejections of their promise chains.

Scenarios in which an error would otherwise be dropped can be handled by reporting infrastructure in the promise libraries themselves.

[ForbesLindesay]

It's OK to not report an exception/rejection when it's been handled. In this case, we can handle it by assuming the object is not a real thenable. That's a totally acceptable course of action. Alternatively we could reject the promise.

I'm definitely in favour of handling it by assuming the object is not a real thenable, because creating a thenable that sometimes throws when you try and access then would be really stupid, and as far as I'm aware nobody does it.

The advantage I can see to causing the rejection would be that if you were creating some weird proxying promise like object you may well end up accidentally throwing errors when people accessed then, and that would be difficult to debug.

[rkatic]

I think it is not wise to support inconsistent (buggy?) proxies, as it's not wise to support bad thenables. So rejecting in case a thenable.then(...) throws seems the only right thing to do.

[ForbesLindesay]

Nobody's suggesting anything other than rejecting when thenable.then(...) throws. Only when typeof thenable.then === 'function' throws. The difference is significant because the first case represents a behaved thenable but the later represents a value that is not a thenable at all.

It's only a thenable if we make the check for a then property and it's a function. Otherwise it's just a value, and values are allowed to behave however they like and can still be called values.

[rkatic]

Maybe I was not clear enough. By this change, you have to retrieve then only once because proxies could be "inconsistent", so you can not simply do thenable.then(...) because that would imply a second [Get]. So you have to do then.call(thenable, callback, errback) or even Function.call.call(then, thenable, callback, errback). And all this only to support inconsistent proxies. Also this would imply that retrieving then and calling it, is done in the same tick, and that could be a too strong (if not a wrong) requirement (Q).

[ForbesLindesay]

Ah, I'm fine with rejecting if a second call to get then triggers an exception. I was more concerned with what happens the first time you attempt to get then.

[rkatic]

I am ok with rejecting when typeof tenable.then throws, but I am opposing to the "only one [Get] call" requirement.

[rkatic]

Ah yes. If typeof x.then throws and that means that x is not a thenable, then it seems right to fulfill the promise with x as the regular value.

[rkatic]

Also, how about defining an IsThenable? That would also simplify the Resolve algorithm.

[domenic]

"only one [Get] call" requirement.

This is nonnegotiable, and Q will certainly be adopting this algorithm. It follows the same behavior as the ES5 spec in many places.

Also, how about defining an IsThenable? That would also simplify the Resolve algorithm.

This is not possible, precisely because determining if something is a thenable cannot be formally separated from the resolve algorithm---otherwise then could have inconsistent values.

[rkatic]

I certainly understand that having an IsThenable would imply multiple [Get] calls into Resolve.

It follows the same behavior as the ES5 spec in many places.

Can you please be more specific of which behaviors in ES5 you are referring?

and Q will certainly be adopting this algorithm.

And is the behaviour of retrieving then, and calling it in separate ticks ok with you?

[domenic]

Can you please be more specific of which behaviors in ES5 you are referring?

I know you have in the past justified questions like this by appealing to your laziness, but I'm going to have to kindly ask you to not comment on the Promises/A+ repositories if you can't do the minimum background reading first. For example, the linked issues in the original post. It wastes everyone's time, and while I was OK with repeating myself once or twice for your benefit, it has continually derailed Promises/A+ threads as I need to explain things that have already been explained elsewhere. Sorry to be harsh.

Anyway, here's the post you didn't read, in #88 (comment), which is linked in the original post:

And is the behaviour of retrieving then, and calling it in separate ticks ok with you?

No, it is not. The entire Promise Resolve Procedure will be performed in a separate tick. then will be retrieved only once.

[rkatic]

I am on a trip and my mobile app was not showing #nn as links. I am sorry for missing them and I am even more sorry for giving the wrong impression of expecting from anyone to repeat himself.

[rkatic]

The entire Promise Resolve Procedure will be performed in a separate tick.

Ok. Is it also ok to stay on the same tick in case x is a true promise? Since it is not specified otherwise, I am assuming it is allowed.

Is there any intention in the future to specify which parts (besides callbacks) are expected to run in separate ticks?

[domenic]

Is it also ok to stay on the same tick in case x is a true promise? Since it is not specified otherwise, I am assuming it is allowed.

Yes, I can't see anything wrong with that. That kind of performance benefit was one of the primary use cases of allowing a separate clause for promises.

Is there any intention in the future to specify which parts (besides callbacks) are expected to run in separate ticks?

If we ever specify assimilation (i.e. specify Q.resolve/Q or when) then this would be important, yeah. We'll see if that happens.

[briancavalier]

Is it also ok to stay on the same tick in case x is a true promise? Since it is not specified otherwise, I am assuming it is allowed.

Yes, I can't see anything wrong with that. That kind of performance benefit was one of the primary use cases of allowing a separate clause for promises.

Agree: a trusted promise is just that, trusted. IMHO, an implementation can assume that it doesn't need to force a new stack to protect itself from its own promises.

[rkatic]

Thanks. My concern (that I missed to specify) was if it would introduce unexpected inconsistency with the case where x is a fulfillment value (sync promise resolution, but async fulfillment with x). While it is not stated otherwise, I will assume it is not an issue.