promises-aplus / constructor-spec

Discussion and drafts of a possible spec for creating and resolving promises
10 stars 4 forks source link

What happens when the factory throws an error? #21

Open domenic opened 11 years ago

domenic commented 11 years ago

In #18, I propose letting the factory run in a new turn per #20, and not catching any thrown errors (i.e. they should just crash the program).

The alternative is to catch any errors and transform them into rejections. This gets a bit complicated, though:

  1. How should resolve(x); throw y; behave?
  2. How should reject(x); throw y; behave?

Plus, it creates a somewhat false parallel between factory and the callbacks to then. The callbacks to then check both return values and thrown errors, while checking return values is not possible for factory.

This is related to what is decided in #20, but in theory any combination could be decided on (same-turn, capture; same-turn, crash; next-turn, capture; next-turn, crash).

briancavalier commented 11 years ago

How should resolve(x); throw y; behave?

Def tricky. It seems to me that the promise's fate has been sealed by the time resolve returns, and that course cannot be altered by the throw. The question is, what happens to y. Seems like we have to crash here.

How should reject(x); throw y; behave?

Same? Fate is sealed when reject has returned. Again, crash?

Plus, it creates a somewhat false parallel between factory and the callbacks to then. The callbacks to then check both return values and thrown errors, while checking return values is not possible for factory.

Hmmm. Maybe our new fulfillment is the goal ("fulfillment is possible" vs. "fulfillment is impossible") thinking can help here? Should an exception in the factory be taken to mean that fulfillment has become impossible? It seems intuitive to me that it should, so it seem ok to me to trap exceptions and ignore return values, despite the fact that then handles both.

domenic commented 11 years ago

So you're thinking a hybrid approach, wherein throw y; resolve(x); rejects with y, but resolve(x); throw y; crashes?

juandopazo commented 11 years ago

Errors should always be caught and rejected. It's easier to implement and think about:

try {
  initFunction(resolve, reject);
} catch (e) {
  reject(e);
}
domenic commented 11 years ago

@juandopazo that code would silence y in the resolve(x); throw y; case, since the promise is already resolved and so attempts to reject it must fail.

briancavalier commented 11 years ago

throw y; resolve(x); rejects with y

Ah yes, good question. Seems problematic to reject, since it would mean that throw's effect would be different (reject vs. crash) depending on whether it executes before or after resolve/reject. That'd be totally confusing, imho.

Seems like ensuring a consistent behavior for throw means we have two choices:

  1. throw always crashes
  2. throw always behaves as if it were a call to reject (as @juandopazo just described), meaning that it seals fate if not already sealed, and is silenced otherwise. One possible weirdness with this "symmetry" is that throw doesn't actually behave like reject, since it prevents all subsequent lines of code from executing, but reject does not.
briancavalier commented 11 years ago

Consider the case where the throw isn't explicit:

resolve(x);
nonExistentFunction(); // oops I made a coding mistake

If we silence this implicit throw as we would a rejection, you might never discover it ... that is, until someone else later refactors this code and the whole application breaks.

juandopazo commented 11 years ago

@domenic damn I was thinking too much about then. You're right.

If we silence this throw as we would a rejection, you might never discover it

@briancavalier yes, that's what I want to avoid too.

novemberborn commented 11 years ago

Same-turn and crash. Assuming promises are constructed inside then callbacks the error will still propagate somewhere. You can always try/catch the first promise construction in the application entry points.

ForbesLindesay commented 11 years ago

I would much prefer to always treat throw as a call to reject.

We really can't go with throwing synchronously because that leads to people having to wrap calls to async methods in try/catch as well as .then(null, catch) which sucks for users.

Crashing entire applications on thrown exceptions is pretty much never acceptable outside of a web-browser. So I think we should avoid that at all costs.

We already silence double rejections and rejections after resolution, so I see no real problem with silencing throw after resolve and throw after reject. It's mostly an obscure corner case anyway.

The other alternative is to make resolve and reject always have their affect in the next turn of the event loop, but make throw have it's effect in the current turn of the event loop so that:

var promise = Q.promise(function (resolve, reject) {
  resolve(x);
  nonExistentFunction(); // oops I made a coding mistake
});
//promise is rejected with the exception resulting
//from calling undefined function

The exception rejects the promise before the call to resolve actually gets as far as resolving the promise.

novemberborn commented 11 years ago

We really can't go with throwing synchronously because that leads to people having to wrap calls to async methods in try/catch as well as .then(null, catch) which sucks for users.

Yes, but without due research you wouldn't know whether the async method you call could crash before the promise is even constructed. It strikes me that most code is already executing inside a then-callback, so thrown exceptions won't crash the program.

People must assume that any function they call, even if it's supposed to return a promise, may throw instead. For example due to bad inputs. Next-turn/crash will require (non-promise) library authors to carefully guard their inputs as to not crash from the factory. Capture will require authors to carefully guard their inputs as to be able to throw useful errors, because otherwise they may be hidden by the promise library.

Same-turn/crash means the (non-promise) library authors can deal with exceptions thrown by the factory as they see fit. Either by crashing the program, returning a rejected promise, etc.

ForbesLindesay commented 11 years ago

Capture shouldn't impose any such requirements. If errors are being hidden by the promise library, then the promise library is being misused. Most promise libraries provide a .done method which can be used to ensure errors aren't silenced and anything which seeks to convert back to the callback method of handling asynchrony should also be sure to handle error's properly.

People actually rarely have to assume that functions they call may throw even if they're supposed to return promises. Even if people should be, they rarely do. It's mostly just much too complicated to think about something that could throw both synchronously and asynchronously. Pick one, and stick to it. Either you return synchronously and throw synchronously or you return asynchronously and throw asynchronously.

domenic commented 11 years ago

In light of https://github.com/promises-aplus/promises-spec/pull/76, I think it should be capture, not crash.