Re: Our error-handling is broken (Re: Promise slides)

On Wed, Oct 1, 2014 at 1:05 PM, Jan-Ivar Bruaroey <jib@mozilla.com> wrote:

>  In case it's not obvious from the slides, I'll be arguing tomorrow that
> our error-handling is broken and should not be shipped.
>
> This excellent post on promises [1] says why better than I can:
>
> "There are two very important aspects of synchronous functions:
>
> They return values
> They throw exceptions
>
>  Both of these are essentially about composition. That is, you can feed
> the return value of one function straight into another, and keep doing this
> indefinitely. More importantly, if at any point that process fails, one
> function in the composition chain can throw an exception, which then
> bypasses all further compositional layers until it comes into the hands of
> someone who can handle it with a catch."
>
> Now, in an asynchronous world, you can no longer return values: they
> simply aren’t ready in time. Similarly, you can’t throw exceptions, because
> nobody’s there to catch them. So we descend into the so-called “callback
> hell,” where composition of return values involves nested callbacks, and
> composition of errors involves passing them up the chain manually, and oh
> by the way you’d better never throw an exception or else you’ll need to
> introduce something crazy like domains.
>
>
Conveniently, we already agreed months ago that in WebRTC and gUM,
error callbacks are used for all runtime error reporting and exceptions
are used *solely* for programming errors (i.e., the kind of thing that would
be a good candidate for compile-time failures and/or asserts in other
languages). Thus, this argument about exceptions is largely irrelevant,
leaving us with an argument about the relative aesthetic merits of
callback chains versus promises.

-Ekr

I will demonstrate.
>
> First, here's a fiddle [2] showing how promises handle errors correctly:
>
>
> <div id="log"></div>
>
> var div = document.getElementById("log");
> var log = msg => (div.innerHTML = div.innerHTML + msg + "<br>");
>
> new Promise(resolve => resolve())
> .then(() => log("success1"), () => log("fail1"))
> .then(() => {
>   log("success2a");
>   barf;
>   log("success2b");
> }, () => log("fail2"))
> .then(() => log("success3"), () => log("fail3"))
> .then(() => log("success4"), () => log("fail4"))
> .then(() => log("success5"), () => log("fail5"))
> .catch(() => log("failure"));
>
>  This outputs:
>
> success1
> success2a
> fail3
> success4
> success5
>
>  Importantly, fail3 - not fail2 - is called because success2 which barfed
> is actually step3.
>
> Furthermore, success4 and success5 proceed because we "caught" the error
> in fail3! (we'd need to re-throw the error just like we'd do in a try-catch
> clause if we didn't want that).
>
> Now, here's the same test in a fiddle [3] showing how callbacks handle
> errors poorly (spot the bug):
>
>
> <div id="log"></div>
>
>  var div = document.getElementById("log");
> var log = msg => (div.innerHTML = div.innerHTML + msg + "<br>");
>
> try {
>   oldcall(() => {
>     log("success1");
>     oldcall(() => {
>       log("success2a");
>       barf;
>       log("success2b");
>       oldcall(() => {
>         log("success3");
>         oldcall(() => {
>           log("success4");
>           oldcall(() => {
>             log("success5");
>           }, () => log("fail5"));
>         }, () => log("fail4"));
>       }, () => log("fail3"));
>     }, () => log("fail2"));
>   }, () => log("fail1"));
> } catch(e) { log("failure"); }
>
> function oldcall(success, failure) {
>   var succeed = true;
>   setTimeout(succeed? success : failure, 0);
> }
>
>  This outputs:
>
> success1
>  success2a
>
>
> and a "ReferenceError: barf is not defined" in web console.
>
> This is bad because it means the program can't handle the error. The bug?
> try/catch is needed around success2 and - to be safe - around *every*
> callback! This final fiddle [4] shows what's minimally needed to handle
> errors safely in our API, and even this doesn't truly propagate errors,
> e.g. we're just pushing handlers in, not allowing errors to rise up:
>
>
> var div = document.getElementById("log");
> var log = msg => (div.innerHTML = div.innerHTML + msg + "<br>");
>
> try {
>   oldcall(() => { try {
>     log("success1");
>     oldcall(() => { try {
>       log("success2a");
>       barf;
>       log("success2b");
>       oldcall(() => { try {
>         log("success3");
>         oldcall(() => { try {
>           log("success4");
>           oldcall(() => { try {
>             log("success5");
>           } catch(e) { log("failure"); } }, () => log("fail5"));
>         } catch(e) { log("fail5"); } }, () => log("fail4"));
>       } catch(e) { log("fail4"); } }, () => log("fail3"));
>     } catch(e) { log("fail3"); } }, () => log("fail2"));
>   } catch(e) { log("fail2"); } }, () => log("fail1"));
> } catch(e) { log("failure"); }
>
> function oldcall(success, failure) {
>   var succeed = true;
>   setTimeout(succeed? success : failure, 0);
> }
>
> This error-prone boilerplate is reminiscent of antique languages without
> exception-handling, and has this output:
>
> success1
> success2a
> fail3
>
>  This STILL doesn't have the same output as the first fiddle, and I gave
> up since I don't think a version that does is feasible without promises.
>
> Could we at least hide the boilerplate inside oldcall? We could, but this
> would require a third argument (recall that we want fail3 not fail2):
>
>
> function oldcall(success, failure, followingFailure) {
>   try {
>     var succeed = true;
>      setTimeout(succeed? success : failure, 0);
>   } catch (e) { followingFailure(e); }
>  }
>
>  The third argument would have to be optional to be backwards compatible,
> so there would be no enforcement. Even then it would be complicated to use,
> and still wouldn't solve propagation.
>
> The proper way to solve this is with promises.
>
>  .: Jan-Ivar :.
>
> [1]
> http://domenic.me/2012/10/14/youre-missing-the-point-of-promises/#what-is-the-point-of-promises
> [2] Firefox: http://jsfiddle.net/jib1/eyq80vh6 - Others:
> http://jsfiddle.net/jib1/68h59cbf
> [3] Firefox: http://jsfiddle.net/jib1/79qLeb4g - Others:
> http://jsfiddle.net/jib1/qyqypsyv
> [4] Firefox: http://jsfiddle.net/jib1/cy4rvpLb - Others:
> http://jsfiddle.net/jib1/xh5r933a
>
>