Should we trap exceptions?

[domenic]

There are a number of pathological cases, mainly involving badly-behaved promise subclasses, that can cause unexpected state (e.g. a deferred whose [[Resolve]] is not callable).

Before the last day's worth of changes, these possibilities were either ignored (e.g. #51) or converted into rejections (see e.g. 420e9270429a8d9d6cac6ef5eee6b08bccd20493 for how that was done in Promise.race). After the recent set of changes, they are allowed to propagate as thrown exceptions via various ReturnIfAbrupt calls.

I want to use this issue to discuss whether this new strategy of bubbling exceptions is correct, or if we should handle the pathological cases and transform them into appropriate rejections. Here are the ones I can find immediately, mostly by searching for "throw a TypeError" or "ReturnIfAbrupt" in the spec:

Current Spec Status

Cases that Bubble the Exception

• Attempting to use any Promise static factory or combinator methods on a this that returns a non-true-promise, e.g.

  class Evil extends Promise {
  constructor() { return { not: "a real promise" }; }
  }
  
  Evil.race([a, b]); // throws a TypeError

• Attempting to use any Promise static factory or combinator methods on a this that is not a constructor.
• Attempting to use any Promise static factory or combinator methods on a this that is a constructor but throws an error when passed a single function argument.
• Attempting to use Promise.prototype.then on an object whose constructor property is a throwing getter.
• Attempting to use Promise.prototype.then on an object whose constructor property is not a constructor.
• Attempting to use Promise.prototype.then on a this.constructor that is a constructor but throws an error when passed a single function argument.

• Any operation that needs to resolve a promise when used in the context of a constructor that passes a non-callable as the first argument to the resolver, e.g.

  class Evil2 extends Promise {
  constructor(resolver) { resolver(5, 10); }
  }

• The same Evil2 example, but for rejections.

• Any errors from the resolve and reject functions themselves, e.g.

  class Evil3 extends Promise {
  constructor(resolver) {
    resolver(() => throw new Error("boo!"), () => throw new Error("aah!"));
  }
  }

• Any errors while iterating through the iterables passed to Promise.all and Promise.race, e.g.

  var evilIterable1 = { [Symbol.iterator]() {
  return { next() { throw new Error("blargh!"); } };
  } };
  
  var evilIterable2 = { [Symbol.iterator]() {
  return { get next() { throw new Error("oh noes!"); } };
  } };

  Cases that Trap the Exception

• Errors thrown by a resolver function passed to new Promise are converted into rejections. Previously covered in #23.

  Arguments

  Arguments for Trapping

• Promise-returning methods should always return promises, and never have synchronous behavior. There are two worlds of functions, synchronous and asynchronous, and they should not overlap.
• See previous discussion in #24.

  Arguments for Bubbling

• All of the cases that bubble as of the current spec are truly pathological and should never occur in normal code. They represent massive programmer failure, beyond just passing in bad arguments, but instead stemming from creating bad subclasses or bad iterables.
• Performance: at least for user promise implementations, and possibly for native ones as well, the try/catch blocks necessary to trap these errors would cause a performance hit. (V8 at least deoptimizes any function with a try/catch block.)
• As many of these cases have to do with poorly-behaved subclasses, we would have to revert to returning rejected instances of the base Promise class to be sure we don't just trigger another failure. So e.g. Evil.race would return a rejected Promise, not a rejected Evil. That's weird.

  My Thoughts

I think the current status quo makes sense. It would have been nice to have completely asynchronous behavior, but it seems infeasible, in the face of all the things that can go wrong. I don't feel that strongly either way, though, so I am willing to be swayed. We could move one or two of the currently-bubbling cases into trapping, if you think they are more indicative of potential bad user input (see #24) than massive programmer failure.

[domenic]

Interestingly both RSVP and Q trap "evil iterable" cases for .all. Maybe that should be considered "argument validation" and trapped, instead of being allowed to bubble.

[domenic]

In 7e845e44045e4063c1e48cd3cfcab419620ae59b I shifted some bubbled exceptions to trapped rejections for argument validation in all/race. This moves the "evil iterable" cases into the rejections camp and makes all the bubbled exceptions stem from evil promise subclasses.

I believe this is a good place to be. Since this issue hasn't seen much interest, I will call it satisfactorily resolved, and close it for now. Let me know if it should be reopened for discussion.

[Nathan-Wall]

The following are two consistent options I see for dealing with errors in promise-generating functions.

1. Always produce a rejected promise.
2. Throw when the error is unambiguously detectable synchronously; return a rejected promise when the error is potentially asynchronous.

I might have missed it, but I haven't seen 2 discussed in these terms exactly. It seems most of the discussion in the past has revolved around what kind of error is being produced: Is it one that "looks like" an early error or does it "look like" the kind of error that should be swallowed? Maybe the thinking could be shifted to can this error always be detected synchronously?

For instance, when the then property of the originator is accessed in UpdateDerived, if it throws, it should result in a rejection because this happens in a queued up microtask (I think most people agree on that). However, the iterable argument passed to Promise.all can clearly be determined to be an invalid iterable synchronously, so perhaps it should result in a thrown exception.

So basically: Inside a microtask, reject; outside a microtask, throw. The language of the spec can be written such that sync internal functions always throw, but if they're called from inside a microtask the microtask will trap the exception and reject a promise instead.

Not sure I like 2 better than 1 at the moment... but it gives clear guidelines for how to proceed in any situation so that the choice of whether to throw or reject isn't arbitrary, and it sounds reasonable.

Thoughts?

[domenic]

@Nathan-Wall that is interesting, and I agree it does give somewhat consistent semantics. But I would rather produce a rejected promise for argument validation, for the reasons discussed in #24.

Ideally I would rather return a rejected promise for the "this/this.constructor-validation" situations, but on the other hand if your this is messed up, you're going to have a hard time constructing a rejected promise to return in the first place, so that is my reasoning for drawing the line where I did.

[medikoo]

I think all functions either synchronous or asynchronous should expose invalid arguments errors in same manner. It's rejection because of algorithm error (function cannot be executed), and that's in all cases synchronous.

In node.js all async functions throw on invalid arguments (same I believe is in all other async JavaScript API's that we have so far). It's expected behavior, and I don't remember anyone expecting something different. It's postponing such errors that may become confusing.