-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Why shouldn't promise-returning functions throw? #24
Comments
It forces duplicate error handling logic to both wrap a function in |
Yes, WebIDL definitely needs revision to accomodate promises into its overload resolution algorithm. |
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? |
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. |
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 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.) |
This is the exact same argument that people have made for not allowing
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 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 |
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?) |
Yeah, definitely, in the same way that |
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 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
If 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. |
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 Currently in node & browsers you can add a single application error reporter to 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 |
Depending on exactly what you mean, yes. var result = getWidget().then(frobWidget).then(babWidget).then(blarghWidget);
widget.catch(function(err){...}); |
In RSVP we have something somewhat like 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. |
@stefanpenner I don't mind false positives. Handling rejections asynchronously is not a use case I cater to. Is something like |
@Raynos Ya its pretty simple. poke around -> https://github.com/tildeio/rsvp.js/blob/master/lib/rsvp/promise.js |
@stefanpenner I mean can we add it to native promises where we can't hook into the machinery. |
@Raynos ah, no. |
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 |
@medikoo the method described also catches the places where you forgot to |
@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 |
@medikoo if you have used this approach you would realize it is quite valuable. The only false positive is asynchronously handled rejections where Also I fully understand how |
@stefanpenner I use |
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.. |
@stefanpenner I don't see how you get that. I stated that all my errors (mistakes) are exposed naturally, as I use |
ajax(request).then(function(response){
// forgotten return
response.then(function(){
// oops typo
});
}).done() done didn't help. |
@stefanpenner This example doesn't make much sense. If someone writes Still, as you used |
Then use maybes or monads or never throw exceptions which all have deterministic error behaviour.
|
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 The issue with invalid argument errors is rarely about simple typos. Data that can propagate through multiple promise-returning functions should never cause synchronous errors because that forces programmers to do checks at every single point as demonstrated above (or give up and use both try/catch and JavaScript is a dynamic language. Functions can and (unfortunately) often do return values of different types. Other functions take those returned values as input arguments. Add several layers of this as in the example I gave. For a good measure, also make some of those functions dynamic, e.g. replace acquireThings with Now lets compare what happens when promises are always returned. The only remaining errors that throw are either synchronous code: var res = preFrobMassage(data); But at least that code has just a single error channel (exceptions), not two And local programmer errors: var res = preFrobMasage(data); // typo Which are most definitely programmer errors local to this code. Only two errors that are local programming errors become a promise: using an undefined variable name: var res = preFrobMassage(dataa); and non-existant object property accessors: var res = preFrobMassage(obj.dataa); The first can be statically checked in strict mode easily, which leaves just the second one. Now my question is, are there any issues with this programmer error turning into a Promise? If so, what are they? |
Not validating input that is external from your process is a bug. Once you have correctly converted this external input into a known shape with known types then you have removed the runtime errors and are back to typo errors.
This is a difference between failing hard and failing softly. Without debugging tools like Also we may decide that I know need to add extra error handling because the callsite uses the error value in the promise to communicate programmer errors which are bugs and must be reported on and fixed. I'd much rather have that be a thrown exception and have it use the bug reporting mechanism that is used for every other bug in my application. |
@Raynos Its not just the external data. For modules, its also the entire API surface - every single function that can be called by external code must have type checks. Its the worst possible solution to the lack of static types problem - piling layers upon layers of typechecks that check data over and over again. Its also horrible for module evolution - if you decide to expose new functions which weren't previously exposed, better make sure you add type checks to them. Even if we set that set aside and assume that argument type errors must use another channel, you simply cannot do that with promises. Because even if the function throws, once you're inside a then callback promises themselves will uphold the contract and convert the thrown error into a rejection. That is how they're designed to work. And if you try to combine both approaches, you get potential resource leaks and other fun problems, and I find it hard to imagine anything worse than that. |
@spion programmer errors are as @Hixie very clearly explained a result of using an API in a bogus way. It's either result of typo, lack of programmer knowledge how API works, or in general badly configured algorithm that leads to given bogus call. There are no other reasons.
If you have used node.js you know it's not an issue as you try to put it. You can setup long callback chain and any programmer error in a middle will throw in uncaught way, by design they're not propagated as errors to callbacks. It's expected, it's a programmer error which you want to be exposed by all means, immediately. I don't remember anybody proposing a change to that node.js behavior, instead they provided us with this very informative guide http://www.joyent.com/developers/node/design/errors |
On Wed, 21 May 2014, C. Scott Ananian wrote:
You should never call the API with 0 as the value. It's not a dynamic data
That's handled at the WebIDL level. I'm arguing that all the errors that
I don't understand why this would be a rejection. It's a bad use of the
This seems to be the same as an incomplete image. It's an image in an
Sure, lots of things can be dynamic. But if you ever get to this point (I mean, a method call could be "dynamic" too, as in: var promise = windowfoo; ...where "foo" is a string. But that will still throw synchronously if
Then the programmer should have verified the type before trying to convert
It is. For example, if we didn't check if the image was fully loaded
It's for consistency with the other branches of this factory method. Ian Hickson U+1047E )..,--....,'``. fL |
Let's separate out the "zero width/height" aspect, because I think that is a fundamental error (not related to Promises). Zero-width Your other comments seem to indicate that we do have a real disagreement here about predictability of errors and execution. Unless I misunderstand you, you are claiming that there is no condition in which Have you read http://blog.ometer.com/2011/07/24/callbacks-synchronous-and-asynchronous/ and http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony yet? Certainly one method to avoid releasing zalgo is to always make every exception synchronous and never reject any promise, but that only works if (a) no one is every expected to catch exceptions in normal use of the API, and (b) every possible exception can be thrown synchronously (which implies always blocking until every possible thing that could go wrong has had the chance to go wrong). Or do you disbelieve in zalgo? |
There's nothing in the <img> case that should ever reject, correct. You can statically determine before you call the factory method whether or not you've satisfied the contract that the factory method expects, so calling the factory method should never fail. But the <img> case is boring. A more instructive case would be Blob. Here the contract is "pass a non-empty source rectangle and a valid pointer to slow-to-read bytes". You can do that and still end up with a problem, because the slow-to-read bytes might turn out to be a corrupt image or an unsupported format. So there there would be some errors that throw (e.g. the Blob is closed when you pass it in), and some errors that reject (e.g. the Blob contains corrupt data). |
We have gone round and round in circles, only to come back to the same conclusion we had dozens of messages ago:
We can't get to all (1) or all (2) (though obviously the symmetry would be nice if we could). As such, the arguments for "predictability of error handling" are a tad moot. May I suggest to handle (3) we debate more explicitly these two options:
Debating (1) and (2) themselves is pointless. They're givens. Long-proven. Let's make progress on (3). |
If we agree that WebIDL should not convert TypeErrors it throws during argument checking into a rejected promise that it returns instead of throwing, and that WebIDL will allow methods that return promises to throw synchronously, then I'm happy. |
@Hixie you didn't answer my question about whether you've actually grokked the various arguments about "zalgo" and agree/disagree with them. By mixing synchronous and asynchronous exceptions you seem to be rejecting the basic premise of Havoc and Isaac's essays. Assuming that you are doing that knowingly, would you care to elaborate on your reasoning? |
cscott: I totally agree that an API, when called within its contract, should always either be synchronous or asynchronous in its uses of callbacks. I'm only arguing about the case of calling outside the contract. I'm saying that should fail early, because at that point it's not the API you're dealing with, in much the same way that if you typo the name of the method it's not the API (and it'll return early). It's not confusing to authors that typoing the name of a promise-returning method throws instead of returning a rejected promise. It's similarly not confusing to authors that passing the wrong number of arguments throws instead of returning a rejected promise. IMHO, it's actually more confusing if an error of this kind ends up reporting the argument problem in a callback/promise. The blog posts you cite aren't talking about this kind of out-of-contract error, so they don't really apply here. (Actually, promise callbacks will always be async, so the bad pattern that these blog posts refer to can't actually happen, as far as I can tell, when using promises.) |
Once again, there is no clear boundary between "violating the API contract" and "data error". Passing Typoing throws sync because we don't have syntax affordance in JS for saying "I'm calling this async" - sync and async calls look identical. If we did have such, a typo would definitely result in a rejected promise, not an exception, for all the reasons already explained. As @cscott said, several people have written well-reasoned arguments for keeping all exception handling async for async functions. What do you think is wrong with their arguments? |
The bad pattern is when the API throws a synchronous exception when all the handlers are expecting asynchrony (or vice-versa [*] ). The bad pattern is when you have to write separate synchronous and asynchronous handlers because the API is not consistent. I agree that synchronous throws for what Java would call "exceptions that an ordinary program is not expected to catch" are unobjectionable; we seem to differ only on what that means. You seem to feel that the API should be written such that no reasonable program triggers the exception paths, and thus most exceptions are synchronous. You've mentioned that the Blob case is one where you could see a promise rejection (if the slow-to-read Blob contains corrupt data). I understand the implementation logic of this, but semantically it seems the result is we treat unexpected data in Blobs different than unexpected data in What other cases are there in the [*] Although we can have consistent handling by either making all "conditions that a reasonable application might want to catch" synchronous or asynchronous, the latter is straightforward to implement while the former requires unacceptable blocking of the main thread if we apply it consistently. |
I disagree that there's no clear boundary. It's a boundary that the DOM API has been exposing for years, in the form of exceptions on one side, and 'error' events on the other. My earlier comments on this page are my attempt to argue against the case for using rejected promises for TypeError and ReferenceError and InvalidStateError and other "contract violating" cases. It's quite possible that this really just comes down to different priorities, and that just as I think the arguments are a compelling case for what I'm arguing, you think the arguments are a compelling case for what you're arguing. If this is the case, then I don't know what I can say that will change this. createImageBitmap() is already specced to use promises (it's just the old-style promises before they were merged into JS; this whole discussion came up because I'm trying to update the spec to match the new promise prose and in doing so was told one thing I should do is stop having any exceptions). So you can tell what I think should be rejections vs what should be exceptions. It's anything that you can't determine will be a problem before calling the factory method. It's worth noting that createImageBitmap() is meant to be an API that takes a valid image source (something you could paint on a canvas synchronously) and turn it into a ImageBitmap object. Only Blobs are the exception to this; drawImage() doesn't accept a Blob. An argument could certainly be made that createImageBitmap() should be trying to be something else, but that's a different argument than the one I'm trying to have here. (If you do think that, please file a bug http://whatwg.org/newbug .) |
If you actually read the material you linked about zalgo you will see that it says do not call a callback both synchronously and asynchronously. It says nothing about throwing exceptions. Throwing an exception is not zalgo, please do not use this argument incorrectly. |
Exceptions which are "conditions that a reasonable application might want to catch" are simply another form of return value, and are indeed callbacks in pre-promise node. I am not using the argument incorrectly. And my experience programming with promises involved a lot of network synchronization code, where exceptions are usually thrown to indicate various forms of transient network errors which are (in the real internet) not uncommon, and completely reasonable to try to catch. And yes, in the pre-promise-refactoring node-style-callback version of our code, we were littered with Bad Things where synchronous exceptions were not properly turned into node-style invocations of an error callback, resulting in an unhandled exception, loss of the flow of control, and our cloud synchronization "just stopping". That was bad, and there were dozens of possible places where network errors of one sort or another could occur. So yes, where isaac says "callback" it is entirely proper to read "error callback" (aka, "thrown exception"), and I am certainly biased by my experience to make any error path which could possibly be triggered by network instability into a promise rejection. (And this includes zero-size images, unexpected image types, etc.) ps. See also the |
@cscott it looks you missed the point of those articles. It doesn't say that programmatical errors should not throw immediately. It's strictly about how the result of given (initialized) operation should be returned. In node.js you can make nearly every asynchronous function to either throw immediately or invoke it's callback async way. Do you suggest that node.js API exposes the zalgo? :) |
What this thread discusses is: Do (in promise returning APIs) programmatical errors should be thrown immediately or should be propagated into promise rejections? What we also try to discuss here: On given API example, what should be considered a programmatical error? This is two separate matters. Concerning second one, If given API method is expected to create an object of specific instance an async way, and input arguments for that are bogus (taking into account all ECMAScript normalization conventions). So it's immediately obvious (by definition, in deterministic way) that such object can't be created. It is in my opinion a programmatical error, I see a very clear boundary here. |
ReferenceErrors caused by typos are different in one important way. To get an actual ReferenceError to be thrown, the code that invokes the particular promise function has to be locally incorrect. You have to either use some sort of reflection (treat the method name as another argument), e.g. obj[methodName](args, ...) or actually have a real, local typo/error obj.incorrrectMethodName(args, ...);
// or
objj.incorrectMethodName(args, ...);
// etc But argument data can come from anywhere. It may come from a database, from the network, from another repository. Without a type system or without littering the code with layers upon layers of checks its impossible to say anything certain about the data without tracing its entire path. This is very different from a local typo. Finally, the main point here is that it doesn't really matter. Promises already convert all exceptions thrown in the body of a callback passed to Infact, I gave an example that leaks resources when you throw synchronously from a promise-returning function although the local code is correct. Can you provide an example that shows how throwing to get "fail fast" behavior works in the context of promises? Because my hunch is that it doesn't, at all. |
The "fail fast" behaviour I'm talking about works exactly the way that the ReferenceError errors work. Does ReferenceError "not work, at all"? |
@spion my assumption is that the "fail early" crowd really wants to ensure the thrown exception is uncaught and thus triggers the default uncaught exception handler, which will put a nice big red message on console. As others have pointed out, unhandled promise rejections will do the same thing... and this notion of "uncatchable exception" isn't really a Thing in javascript in the first place. |
@Hixie it certainly doesn't cause promise code to fail fast if the ReferenceError happens within a callback passed to then: function test(arg) {
var p1 = promiseReturningFunction(arg).then(function(val) {
// The next line throws ReferenceError: wrongVariable is not defined
return wrongVariable.someMethod(val);
// This error will not bubble up to the uncaught exception handler
// It will be converted to a rejection, and (if not handled) will end
// up in whatever deals with unhandled promise rejections
});
// this code continues to execute. "fail fast" can't work across async boundaries.
var p2 = otherFn(arg).then(function(val) {
return rightVariable.rightMethod(rightArgument);
});
// Same for the rest of the code that doesn't depend on p1
var p3 = p2.then(...)
// Finally we get a rejection here:
return Promise.all(p1, p3)
} Does this code fail fast? I don't think it does. |
Your callback there only has one line of code, so "fail early" is the same as "fail late". Also you see to have no rejection handler, so there's not much to compare here. |
Why would he need a rejection handler here? Unhandled rejections are logged in big red text to the console anyway |
Same as uncaught exceptions. Right. |
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.
The text was updated successfully, but these errors were encountered: