Why shouldn't promise-returning functions throw?

[tabatkins]

The spec-writing guide says that promise-returning functions should never throw; instead, they should just reject the returned promise with the relevant error.

It even gives a specific example of argument-parsing errors. Why is this? In existing callback functions, argument-parsing is usually an immediate thrown error, rather than calling the errback. Some types of argument-parsing errors aren't even handled by the specs - they're handled automatically by WebIDL.

I definitely understand why "content" errors should always happen in the promise, even if they can be detected "early enough" to throw synchronously instead. But argument syntax errors seem like a separate class to me.

As a further argument, getting a syntax error from an argument list is sometimes used as a test for support for some new functionality added to the function. Requiring the test to wait for its rejection handler to tell whether the new functionality is supported or not seems annoying.

[domenic]

It forces duplicate error handling logic to both wrap a function in try catch to catch one type of exception, and use the rejection handler to catch other types of exceptions. This is not a good API pattern. Mixed sync-async behavior releases Zalgo upon your code.

[domenic]

Some types of argument-parsing errors aren't even handled by the specs - they're handled automatically by WebIDL.

Yes, WebIDL definitely needs revision to accomodate promises into its overload resolution algorithm.

[tabatkins]

It forces duplicate error handling logic to both wrap a function in try catch to catch one type of exception, and use the rejection handler to catch other types of exceptions.

Yeah, but only if you're actually trying to catch argument-parsing errors. How often do you defensively code against accidentally passing bad arguments to a function?

[domenic]

It's not about defensive coding, it's about handling errors.

Fundamentally, promises are based around a uniform error-handling paradigm. Breaking from that surprises consumers of promise-using functions.

[tabatkins]

I get the arguments for uniformity. I'm not yet convinced that it's a useful uniformity for this specific class of error.

I know the distinction between "compile-time" and "run-time" errors is arbitrary and changes with each language (and for JS doesn't have much meaning at all), but there's an underlying rough separation of errors into "logic" errors and "you fucked up while writing this" errors. The first is recoverable - some quality of your argument is wrong, its foo property is set to "bar" rather than "baz" or somesuch. The second is a simple mistake. You "recover" by fixing your code, not by dealing with it at run-time. The only use-case I know of for dealing with those types of errors at run-time is what I already outlined - testing for support for a new argument signature. That particular use-case is better served by throwing synchronously, so it can move on to the next test immediately - it doesn't actually want any of the information that you'd be doing async work for.

It might be that the consistency argument is strong enough to make this worthwhile - that we are dividing the world into "sync" and "async" functions, and you fundamentally interact with the two in sync or async ways, respectively. I'm just not sure about this.

(Note that my usage of promises in Font Load Events is wrong even by my argument - I'm currently synchronously throwing on some argument-inspection errors, like if a string fails to parse as a 'font' property value. I think this falls into the recoverable logic-error bucket, and so I should switch it over to rejecting the promise instead. But I think if you passed an Element instead of a DOMString, it makes sense to throw immediately.)

[domenic]

This is the exact same argument that people have made for not allowing SyntaxErrors or ReferenceErrors or similar to be transformed into rejections when you throw inside a promise handler. We are retreading old ground, and like before, in JavaScript there is no distinction; all exceptions are the same. The only distinction that really matters is early errors (compile time, while-parsing-the-script) vs. runtime errors, and type validation errors are not early errors since JS is not typed.

The first is recoverable ... The second is a simple mistake. You "recover" by fixing your code,

You cannot rely on all code paths to be hit in testing, and runtime recovery from such errors is something you need to be able to do, e.g. trap it and log it back to the server for analytics while popping up a dialog saying "oops, something went wrong frobbing the widget, we'll look at the logs and fix that later." With synchronous code you would express this by encapsulating the complicated process of widget-frobbing in a function like frobWidget, then do

try {
  frobWidget();
} catch (e) {
  sendErrorToServer('/widget/frobbing/errors', e);
  alert("Oops, sorry we couldn't frob your widget!");
}

In asynchronous code it's much the same:

frobWidget().catch(e => {
  sendErrorToServer('/widget/frobbing/errors', e);
  alert("Oops, sorry we couldn't frob your widget!");
});

If frobWidget() has, in some crazy edge case involving user input you never thought possible, managed to pass an argument of an unexpected type to some internal promise-returning function, this should not break the app and bubble up through the synchrononous call stack to surrounding code unprepared to deal with it, putting you in a bad state, but instead should log the error to the server and tell the user.

[tabatkins]

Cool, that's convincing.

(So if we did have type annotations in JS that produced compile-time errors, it would be fine for async functions to still throw syntax errors for violating the type contracts, right?)

[domenic]

Yeah, definitely, in the same way that var 5; throws a syntax error at compile time.

[medikoo]

It forces duplicate error handling logic to both wrap a function in try catch

I don't agree. It's strictly about algorithm errors, same as you never throw errors intentionally to break application, same you never pass invalid arguments to functions intentionally. There's no logical place for try/catch here.

And if you test some foreign function, to see if it works properly, then no matter whether it returns promise or not you wrap it with try/catch (after all we're bulletproofing and take any unhandled/unexpected errors into account), so we're also save here.

In asynchronous code it's much the same:

frobWidget().catch(e => {
  sendErrorToServer('/widget/frobbing/errors', e);
  alert("Oops, sorry we couldn't frob your widget!");
});

If frobWidget strictly relies on some input argument, it definitely should throw, again it's an algorithm error and not error that we assume may happen in normal algorithm flow.

Of course I'm speaking strictly about cases that in 100% of cases would produce errors, e.g.

fs.readFile(); // no path provided, in node.js similar function throws

But if in some circumstances given arguments are ok, and in other are invalid, and (by some weird chance) we're aware of that immediately, then indeed function should return promise that rejects.

[Raynos]

@domenic

The problem with trapping these non recoverable errors is (syntax errors, typos, null references, invalid arguments) a lack of a catch all.

Having build a large app with a system that captures all errors & boxes them I kept forgetting to add error handlers to things which I can't recover from because that's not a natural programming flow. The problem then became my entire app stopped working because one of the hundreds of boxes in the flow contained an error and I had zero insight into what it was without adding error handling at every step (which is unexceptable if (err) return cb(err) style boilerplate).

Currently in node & browsers you can add a single application error reporter to process.uncaughtException and window.onerror. Adding debugger & developer tools to inspect these errors feels silly since we already have tools to inspect synchronous errors, i.e. intercept at window.onerror or look in the console.

Nobody writes code like this:

try {
  frobWidget();
} catch (e) {
  sendErrorToServer('/widget/frobbing/errors', e);
  alert("Oops, sorry we couldn't frob your widget!");
}

They write

// frob.js
frobWidget()
// bab.js
babWidget()
// blargh.js
blarghWidget()
// error.js
window.onerror = function (err) {
  sendErrorToServer(err)
  alert("oops something went wrong")
}

These types of "I dont expect any exceptions" are very common when working with async data & edge cases. Mainly due to corrupted data, database being in a weird state or other unexpected edge cases.

Does promises have an equivalent of window.onerror ?

[tabatkins]

Depending on exactly what you mean, yes.

var result = getWidget().then(frobWidget).then(babWidget).then(blarghWidget);
widget.catch(function(err){...});

[stefanpenner]

@Raynos

In RSVP we have something somewhat like window.onerror. It has one caveat, it can report false positives. Such as if rejections are handled asynchronously, they will still trigger this handler.

RSVP.on('error', function(reason) {
  if (reason instanceof Error) {
    console.assert(false, error); // stops when purple debug mode is enabled
    console.error(error); // clickable stack
  }
});

In practice the increased visibility is well worth it the odd false positive, especially in development.

[Raynos]

@stefanpenner I don't mind false positives. Handling rejections asynchronously is not a use case I cater to.

Is something like RSVP.on('error') easy to implement in user land for arbitrary promises.

[stefanpenner]

@Raynos Ya its pretty simple. poke around -> https://github.com/tildeio/rsvp.js/blob/master/lib/rsvp/promise.js

[Raynos]

@stefanpenner I mean can we add it to native promises where we can't hook into the machinery.

[stefanpenner]

@Raynos ah, no.

[medikoo]

I don't mind false positives.

To me that's not acceptable. Only real errors should be reported, false positives are noise in console we definitely don't want to see and deal with.

Error reporting should be strict, as it's in any other JS API, and we can have it only with Promise.prototype.done, with which you explicitly state the intent of not extending further promise chain. Check long discussion under #19 about that (above issue is not really related to that problem).

[stefanpenner]

@medikoo the method described also catches the places where you forgot to done or chain correctly. Increasing visibility, even with the VERY infrequent false positive, especially during development has been a massive improvement in developer ergonomics.

[medikoo]

@stefanpenner I don't see how reporting false positives among real errors can improve development. Doesn't sound logical at all.

Also once you understand how done works, it's not the thing you can forget. It becomes first choice function (same as forEach is first choice over map in case of array iteration).

[stefanpenner]

@medikoo if you have used this approach you would realize it is quite valuable. The only false positive is asynchronously handled rejections where reason instanceof Error === true which is defiantly not a common scenario.

Also I fully understand how done works, and this is not a good comparison.

[medikoo]

@stefanpenner I use done so I don't have a problem of unhandled errors that are not exposed.

[stefanpenner]

what you just said is essentially: “I don’t make mistakes, and by not making mistakes I don’t have the problem of fixing or diagnosing mistakes" so you have missed the point..

[medikoo]

@stefanpenner I don't see how you get that. I stated that all my errors (mistakes) are exposed naturally, as I use done. I don't have problem of them being not exposed.

[stefanpenner]

ajax(request).then(function(response){

  // forgotten return
  response.then(function(){
    // oops typo
  });
}).done()

done didn't help.

[medikoo]

@stefanpenner This example doesn't make much sense. If someone writes promise.then(function (obj) { obj.then(...); } it means, he doesn't have a clue how promises and then works.

Still, as you used done, proper error would be exposed to end user, it'll most likely be: Object response has no method 'then'.

[Raynos]

@medikoo

To me that's not acceptable. Only real errors should be reported, false positives are noise in console we definitely don't want to see and deal with.

Then use maybes or monads or never throw exceptions which all have deterministic error behaviour.

.done() only works if a) you never forget it. or b) the promise is lazy until you call .done() i.e. you won't get the value OR the error until you do .done() otherwise you still run into the problem of swallowed syntax errors.

[Raynos]

I did think of a better idea. The debugger tools in chrome currently allow you to "pause on all exceptions" and "pause on uncaught exceptions".

If it has the ability to "pause on all rejections" and "pause on unhandled rejections" then that would give me the error visibility I want.

[medikoo]

@Raynos

You don't forget done same as you don't forget to use forEach on array. You use it, or don't use it, that's it.

The only mistake you can do, is to use then where you actually should use done, but I don't remember doing such mistake on my side. Once you get it how works it's not the mistake you would do.

.. and there's no or, all other solutions are unnecessary complex and imperfect.

[Raynos]

@medikoo in that case I would just never use then and only use done. that also solves the problem. That way a promise becomes lazy until you call .done().

Of course you still have the issue of 3rd party code authored by not yourself using then() or hiding / swallowing exceptions or forgetting to use .done(). Just like you have the problem of 3rd party code in the node community throwing asynchronous exceptions.

The only difference is expectations. Within the node community everyone knows they can't throw. Is there a decent standard of expectations around error handling set for the promises community?

[medikoo]

in that case I would just never use then and only use done. that also solves the problem. That way a promise becomes lazy until you call .done().

then is important as well. It just should not be overused, and not treated as first-choice function. Best way to approach it, is to start learning promises from done and after having that sound, move to then as a more sophisticated option when mapping is needed.

Is there a decent standard of expectations around error handling set for the promises community?

Expectations I have is that errors handling is transparent, and that we have full control over it (as in every other JS API). No better solution than done have been proposed so far.

[stefanpenner]

@medikoo you have described my exact point. People unfamiliar with some semantics pay a huge price, developer ergonomics is very important, especially as new users begin to use promises. In addition, even experienced individuals will make mistakes. My above suggestion provides visibility for such mistakes.

[spion]

How about a lint tool that warns of ExpressionStatements that consist of (AnyExpression).<then|spread>(x); and tells you to use .done at the end of the chain? That should be able to capture most common cases where people forget to use .done...

[Raynos]

@spion return x.then(...) is an expression without .done() but not a bug.

[medikoo]

return x.then(...) is perfectly valid, but ; x.then(...); is clearly wrong, and indeed lint tools can be programmed to warn about that.

[spion]

@Raynos thats not an ExpressionStatement, its a ReturnStatement. (I'm using the AST node names that Esprima is using, don't know whats the correct name - sorry)

[spion]

Here is a toy I quickly hacked together to play with - thenlint - also avaialble via npm install -g thenlint

Works with globs (including double-star glob) so it should be relatively easy to check large codebases...

Edit: now it also supports basic detection of broken chains (forgetting to return from within a function expression passed to .then)

[Hixie]

FWIW, I think this is a mistake. If you pass the wrong arguments to a method, the promise should never be created, IMHO. It should just throw like the rest of the API.

[cscott]

@Hixie can you be a bit more specific about your complaints?

I think @domenic's general rule is correct. As with all things human, there are probably some places where it is best to bend the rule to some degree. In my personal projects the line lies at "boneheaded errors" -- that is, obvious faults can trigger immediate throws, but anything which is data-dependent and might not be immediately obvious should use the promised exception handling.

So, @Hixie: do you have a specific promise API method that you think should immediately throw in some situation? I think you need to outline a specific improvement to move the discussion forward constructively.

[petkaantonov]

@cscott It's impossible to tell in the callee function how the passed arguments were obtained. You cannot tell in Object.keys() whether the caller is being deliberately stupid and calling Object.keys(undefined) or having a subtle once in a blue moon bug where complex dynamic interactions end up reading an undefined property of some object somewhere and passing the result to Object.keys - to Object.keys both cases look excactly the same.

[cscott]

@petkaantonov Exactly, you have to take it case by case, based on your particular application and API and what errors you think might be common -- like almost any case where someone might be tempted to bend a "rule". It's not worth discussing generalities -- is there a specific method in the promise API you should should throw early?

[Raynos]

:+1: @Hixie

a function should throw all programmer errors.

All operational errors for a promise returning function must be returned as a rejected promise.

see http://www.joyent.com/developers/node/design/errors for definition of "Programmer error" and "Operational error"

[Hixie]

Well for instance if you ever call createImageBitmap() with a zero width, or with an ImageData object that has been neutered via postMessage(), or if you pass it the "null" value, or a string, or some a Location object, then IMHO it shouldn't return a promise, it should just throw.

But if you call it with a Blob that contains data that, when the user agent gets around to decoding it, turns out to have unexpected data corruption, that should be reported asynchronously by rejecting the promise (or calling the "failure" callback, or whatever mechanism the API uses).

That allows you to pass the promise on to another consumer without worrying about the fact that if a logic error occurs, you'll be leaking your internals all over their code. ("Why the heck am I being sent a promise that's rejected with TypeError?!?!")

[cscott]

@Hixie you don't ever worry when you call a function F synchronously that it might throw a TypeError and "leak internals all over" the caller?

[Hixie]

No, because then it just aborts the whole program. As it should.

[Raynos]

@Hixie

:+1: if a low level API wants to abort the program due to a programmer bug / error (i.e. passing in null or a number or whatever invalid) it should have the freedom to choose to do that with a thrown exception.

Now note it's a separate discussion whether any given DOM / HTML api / interface should even throw for those errors instead of handle them differently, that's a per API discussion or WHATWG organization discussion, not a promises discussion.

[erights]

@Raynos No one is proposing that a normally-promise-returning function written in JavaScript be prevented from throwing. Indeed, no one has proposed any mechanism that would enable such prevention, or even any means to distinguish which functions qualify. Nor is anyone likely to.

The issue is only one of advice and best practices, for JavaScript programmers, and of language bindings for webidl programmers. I agree that the default language binding for webidl that describes a promise-returning function should be that the function only rejects rather than throwing. But if there's a well motivated counter-example, we could introduce a webidl way of saying that a given function is an exceptional case. In fact, we have one already: Just declare the function as returning any, and document in the prose that it is expected to return a promise when it doesn't throw.

[Raynos]

@erights

that makes sense. In that case it's a phrasing issue, as in maybe show both options and then make a case / advice / best practice why the throwing option is not recommended and we recommend promise-returning functions should be rejected.

This way one separates neutral options / schools of thoughts from the best practices that are recommended.

There is also a question whether the people behind TC39 should recommend these best practices or whether the people behind webidl should. As far as I understand one is a javascript language organization and the other is a browser vendor organization.

[getify]

Apologize if I'm jumping in unnecessarily, but I'm confused. There's a variety of ways that the built-in Promise functions (which are "promise-returning") already do in fact throw, per my reading of the spec.

What exactly is being argued? That there should be more of that, or less of it?

For example:

var x = new Promise(2); // throws TypeError

var t = { then:function(s){ s(); } };
Promise.resolve(3).then.call(t,success,failure); // throws TypeError

[domenic]

Yes, if you call on an incompatible this, then it doesn't know what promise constructor to use to return a rejected promise, so it throws an error. A failure of the promise machinery is qualitatively different from a failure of an asynchronous operation.

A clearer contrast is that Promise.all(5) will return a rejected Promise, since 5 is not iterable and it knows that it should use Promise as the promise constructor to signal that. But Promise.all.call(4, 5) will throw, since it has no idea how to construct a rejected promise given that it's being told to use 4 as the constructor.

[getify]

@domenic in all fairness, it seems like by that logic, new Promise(5) should return a rejected promise instead of throwing. I'm sure there's some other bigger reason why it's not, but just on the surface I think I can see where the argument is of that precedent being set there.