"thenable" with then getter that throws

[briancavalier]

See #87. We probably need to specify how to deal with this situation for assimilation.

[briancavalier]

My current strategy in when.js is to reject the assimilating promise if an exception is thrown when trying to access thenable.then. The only other option I can think of is to treat thenable as a non-thenable in that case--i.e. use it as a fulfillment value.

Also, see @domenic's point here about just how much trouble a then getter can cause.

[domenic]

One way of dealing with it is code like this:

try {
  if (x) {
    var then = x.then;
    if (typeof then === "function") {
      then.call(x, resolvePromise, rejectPromise);
    } else {
      fulfillPromise(x);
    }
  }
} catch (e) {
  rejectPromise(e);
}

Notable is the use of then.call(x, ...) instead of x.then(...).

Specifying this seems like it would be really hairy though. But probably important.

[briancavalier]

Yeah, that's basically what I've started doing in when.js. The important thing, as your code points out, is that you should only access then once during assimilation.

[domenic]

Although hmm, treating it as a non-thenable seems more accurate.

if (x) {
  var then;
  try {
    then = x.then;
  } catch (e) {
    fulfillPromise(x);
  }

  if (typeof then === "function") {
    try {
      then.call(x, resolvePromise, rejectPromise);
    } catch (e) {
      rejectPromise(e);
    }
  } else {
    fulfillPromise(x);
  }
}

[domenic]

The ES5 spec actually deals with this kind of stuff a lot. E.g. in [[DefaultValue]]:

1. Let toString be the result of calling the [[Get]] internal method of object O with argument "toString".
2. If IsCallable(toString) is true then,
   1. Let str be the result of calling the [[Call]] internal method of toString, with O as the this value and an empty argument list.

[briancavalier]

Heh, right. then.call may not even be safe. To be really sure: Function.prototype.call.call(then, thenable, resolve, reject)

[briancavalier]

A couple thoughts to support why we might treat it as a non-thenable:

1. It makes it easy to treat IE9 (see IE9 duck-type weirdness #87) the same as other envs, and
2. Since the throw prevents us from completing the duck type test, one could argue that we could not definitively prove it is a thenable, so we should treat it as a non-thenable.

[briancavalier]

And a couple thoughts of why rejecting might be better:

1. In the code above, the exception thrown due to accessing x.then is unobservable.
2. Whenever something "goes wrong" in other parts of Promises/A+, we tend to favor rejecting. For example: if the resolver in new Promise(resolver) throws.

[domenic]

@erights, do you have any comments? Seems like the kind of situation you consider all the time :)

[erights]

@domenic yes. Thanks for checking!

Function.prototype.call.call(then, thenable, resolve, reject) isn't safe either because Function.prototype.call or call.call may have been corrupted. See http://wiki.ecmascript.org/doku.php?id=conventions:safe_meta_programming

Good points about getting .then only once, and about the possibility that getting it might itself throw.

I have just updated makeQ.js to correct all three issues. See https://code.google.com/p/es-lab/source/browse/trunk/src/ses/makeQ.js#211 and let me know if you see any problems with it.

I am taking the philosophy stated above by @briancavalier explaining why rejecting is better. Essentially, don't drop thrown errors if there's a natural promise to break with that error as the reason. I see the tension, but I think this is better than treating the value as a non-thenable.

This is also the right decision for making the assimilation pattern useful as an async analog of Q.promise:

var p = Q({then: function(resolve, reject) {
  ... p, resolve, and reject all in scope and initialized ...
}});

If the body above throws, we'd want it to reject p, just as we would if the factory function passed to Q.promise throws.

[briancavalier]

Thanks, @erights.

Function.prototype.call.call(then, thenable, resolve, reject) isn't safe either because Function.prototype.call or call.call may have been corrupted

For sure. Grabbing/binding/ucurryThis'ing early is the best thing.

I have just updated makeQ.js to correct all three issues. See https://code.google.com/p/es-lab/source/browse/trunk/src/ses/makeQ.js#211 and let me know if you see any problems with it.

Looks good to me.

Accessing x.then before doing typeof also normalizes the behavior in IE9, which nullifies one of my reasons for treating as a non-thenable.

[domenic]

@erights Sounds good, and thanks for chiming in! Just to clarify, though:

This is also the right decision for making the assimilation pattern useful as an async analog of Q.promise:
If the body above throws, we'd want it to reject p, just as we would if the factory function passed to Q.promise throws.

This makes sense, but on the other hand, it wasn't quite the example in question. What about this example?

let badThing = { get then() { throw new Error("argh!"); };
var p = Q(badThing);

Should p be rejected with the "argh!" error, or fulfilled with badThing?

[erights]

@domenic Right, that is a different example. But the governing principle is the same -- don't throw away errors is there's a natural promise to reject with that error as the reason. p should be rejected with "argh!". My corrected makeQ code I link to above does that.

[domenic]

OK, cool. I'll update #89 to have that behavior then. If anyone has objections, now's the time! (Although I would imagine it's us three who are most concerned about edge cases like this.)

[briancavalier]

No objections here :)

[briancavalier]

I'm still on the side that rejecting is the right thing, but feeling a little less certain about it. Here's where my head is at the moment.

First, I'm feeling a little stuck on this:

Since the throw prevents us from completing the duck type test, one could argue that we could not definitively prove it is a thenable, so we should treat it as a non-thenable.

This makes a lot of sense to me. The problem is that the thrown exception has to be silenced :/

@novemberborn's point in #87 that observation shouldn't cause a rejection is a good one. However, the problem is that in the ES5-and-later JS world, observation can cause an exception. If we maintain the parallel between thrown exceptions and rejections, that implies observation can cause a rejection. And practically speaking, we must observe thenable.then to determine if it's a thenable or not.

In sync code, a throwing getter obviously will prevent subsequent sync code from executing, and will propagate up the stack until the exception is caught.

If we want to parallel that in async, observing a thing should be allowed to produce a rejection that propagates until handled. Unfortunately, we're observing it inside the promise machinery, which isn't really obvious to the user.

Another thought is that if there are no reasonable use cases (I have no idea if there are or aren't) for pathological throwing thenable doppelgangers, maybe we should prevent them from ever entering the promise machinery at all, i.e. reject these insane things as soon as we see them.

[domenic]

@briancavalier For what it's worth I agree with both sides of your argument. It seems clearly "correct" from a first principles point of view that these things are not thenables, and thus should be treated as non-thenables and just passed through as fulfillment values. But then again, silencing the error is unfortunate and if we try and draw a parallel with sync functions, rejecting seems like a reasonable reaction. So I'm OK either way.

Unfortunately, we're observing it inside the promise machinery, which isn't really obvious to the user.

I think this is analogous to observing valueOf when inside "the + operator machinery," so I wouldn't worry about it.

Another thought is that if there are no reasonable use cases

@ForbesLindesay pointed out a pretty good one: proxy objects which throw when you try to access properties that are not defined on them. Again I think the valueOf example helps assuage my spirit, at least somewhat: just like you'd know you shouldn't do +proxyWithoutValueOf, you shouldn't do return proxyWithoutThen from within a promise.

[rkatic]

With last observations made by @domenic, it is clear to me that rejecting is the right thing to do.

I think this is analogous to observing valueOf when inside "the + operator machinery," so I wouldn't worry about it.

This is also true on observing toString and implicit casting to strings.
I think then should behave similarly, as the hook-function used by implicit "casting" to promises.

just like you'd know you shouldn't do +proxyWithoutValueOf, you shouldn't do return proxyWithoutThen from within a promise.

And, if I am right, spec already expects from users to embrace some responsibility on what is resolved (thenables), and to wrap when there is no enough knowledge about results.