Disabling promise-swallowing-ness of Route `model` hooks

#1

Hey yall

Promises are a lovely abstraction embraced by much of Ember API, but it’s well-known that the promise-swallowing behavior within promise callbacks can be cumbersome to debug since rather than breaking right where an exception is thrown, the exception is caught and then treated as a rejecting promise.

The way this most commonly bites in an Ember setting is within the Route class’s various model hooks: beforeModel, model, and afterModel; any exceptions thrown, including ReferenceErrors and TypeErrors, get “swallowed” and treated as rejecting promises due to the (somewhat surprising to some) fact that these hooks are called within one giant internal promise chain. Even though the error that gets passed to rejection handlers often has enough information for you to debug and see where the error is occurring (via the error’s stack property among others), it’s a much much better experience to actually break right where the error happens so that you can inspect variables, etc.

So anyway, I’m strongly considering the idea of making those Route hooks not swallow errors synchronously thrown from those hooks. I spiked on this a bit earlier and here’s a demo of it working: http://jsbin.com/ucanam/1556/edit

Here’s the router.js PR for this: https://github.com/tildeio/router.js/pull/56

Note that I’m not changing anything about how errors are treated within promises in general; just within Route hooks.

I highly doubt people willfully were synchronously throwing exceptions from model hooks with the intent of them being treated as rejecting promises; the very few that did would have to change their code to return Ember.RSVP.reject(error) instead of throw error.

What do people think of this? Do any of you use the throw error behavior described above?

#2

-1

the following gist would be enough to globally detect unhandled exception. (For the non-async handler scenario) Outcomes with when applying this gist: False positives are possible, strange errors are exposed, purple debug button works, and correct error propagation semantics continue to exist.

#3

The main problem with that gist is that you don’t stop right where the error occurs, which means you lose all inspectable state at the point where the exception was thrown (so you can’t inspect variables leading up to the exception). There’s a huge ergonomic difference between stopping right where the error happened, right in the stack frame, with inspectable state intact vs landing the debugger in this onerror function where they have no inspectable state and are forced to 1) go through the steps to trace down where the error might have happened, 2) set proper breakpoints, possibly conditional breakpoints of the the thrown exception occurs in an often-called function, 3) have enough debugger-fu to understand how to deal with steps 1 and 2. It sucks for experienced developers and its untenable for noobs.

Keep in mind that I’m not suggesting we change promise semantics to catch errors thrown from resolve handlers and treat them as rejecting promises; what I’m saying is that it doesn’t really make sense for hooks the we expose as public API to take place within the promise land of swallowed exceptions. We advertise our router as promise-aware, but not so much that the hooks it exposes should be creepishly subject to rules you would only expect of resolve handlers.

To illustrate:

App.IndexRoute = Ember.Route.extend({
  model: function() {
    // throw "blah"; -> break immediately, right here
    // null.crap; -> TypeError breaks immediately, right here
    // foo.bar; -> ReferenceError breaks immediately, right here
    // return Ember.RSVP.reject("blah"); -> break in onerror from gist
    // return $.getJSON('wat.json'); -> if rejects, breaks in onerror from gist
    // return new Ember.RSVP.Promise(function(res) { throw "blah"; });
    // (above line:) breaks in onerror from gist
  }
});
#4

tentative +1

That the these hooks are implemented with promises seems like an implementation detail, if there’s an error in my code then if possible I’d like to break into that as opposed to having to manually unwind the stack myself.

#5

-1 I’m with @stefan.

It’s not a question of intentionally throwing errors from route handlers. It’s a question of being able to react when I accidentally throw an exception from a route handler. I don’t want to get different behavior if that error happens on the first run loop (throwing an exception directly) vs if the exception happens deeper down my own promise chain (returning a rejected promise). The whole point of promise encapsulation is that you don’t need to keep worrying about that distinction everywhere.

You really can’t gracefully do promises “half way”. If route handlers can return promises, they should have one consistent behavior for all the ways those route handlers can fail.

@machty, I don’t see the “stop right where the error occurs” distinction. In either case, if you want to stop instantly as the exception is being thrown, you can do that with “pause on all exceptions” mode. That doesn’t care whether there’s an exception handler or not.

@rlivsey, unfortunately in Javascript it’s really never just an implementation detail if the things higher in your callstack are promise-based, using a runloop, exposing an API for unhandled exceptions, etc. All of the abstractions leak, and you really can’t write solid code without being aware of those things.

Having worked with multiple promise libraries for quite a long time, there’s no fantastic fully-automatic solution to unintended exception swallowing. But in a case like the router, it should not be hard to guarantee that exceptions always get reported, including full stack information.

#6

I’m making a distinction between the Ember Router being “promise-aware” vs. the Ember Router treating everything its internal API touches as taking place within the promise land. It does not follow that just because you can return a promise from the Ember Router, that therefore all hooks that are promise-aware ought to take place within exception-swallowing land. You’re giving the user no opportunity to opt in to this advanced concept that is lamentably crappy to debug.

I’d happily been touting the blue stop sign “Pause on all exceptions” “solution” for a long time but it’s still a miserable debugging experience; debugging page-refresh routing issues are particularly annoying since there so many browser-sniffing caught exceptions at startup, so you end up having to find an intelligent place to put a breakpoint, sometimes having to rely on conditional breakpoints, then switch on the blue stop sign, yadda yadda yadda; it’s an embarrassingly miserable developing experience, everyone gets bitten by it all the time, and pragmatism aside, I think the API is just incorrect/inconsistent.

#7

Will the error message still bubble up through the routes if one of my hooks throws an exception? I may not be reading the diff correctly, but it looks to me like this change would break that behavior.

At a minimum, I think error handlers need to still work, and Ember.onerror needs to be able to trap these exceptions. If those both hold true, I can see the benefit of the change.

#8

error actions are still very much alive; they fire any time a promise returned from one of these hooks rejects. But error won’t (can’t) fire in the case of exceptions thrown directly from the hook because, as I’m suggesting, there won’t be any wrapping try/catch to catch them. Here’s a rewrite of the code sample I wrote above but with a focus on when error would fire:

App.IndexRoute = Ember.Route.extend({
  model: function() {
    // throw "blah"; -> break immediately; no `error` action
    // null.crap;    -> break immediately; no `error` action
    // foo.bar;      -> break immediately; no `error` action
    // return Ember.RSVP.reject("blah"); -> fire `error` action
    // return $.getJSON('wat.json'); -> if rejects, fire `error` action
    // return new Ember.RSVP.Promise(function(res) { throw "blah"; });
    // (above line:) fire `error` action
  }
});

You bring up a good point about Ember.onerror; I’d similarly realized this was an issue only to find that it’s already prevalent throughout Ember any time you have a 3rd party callback or use Ember.run.later; I’m gonna start a separate thread to discuss this but I think it might just make sense to have Ember.run internally wrap its callback in handleErrors.

Proposal: wrap all Ember.run's in `Ember.handleErrors`
#9

Here’s the separate thread for handleErrors: Proposal: wrap all Ember.run’s in `Ember.handleErrors`

#10

Yeah, that’s precisely what I was afraid of. This makes the error handling really brittle. A user may intend to always return promises, and expect that the error action is going to notify of anything unexpected going wrong. But their bug may strike before they’re able to return a promise.

This creates a painful distinction, especially when you consider the way code can evolve over time. You can refactor an underlying dependency that subtly changes the run loop timing of your route handler, and leaves you exposed to throwing immediate exceptions when you weren’t before.

This also seems like it leaves out users who aren’t opting in to promise-based route hooks. Shouldn’t they be able to automatically show error-recovery routes too, when something breaks? And I mean without manually wrapping every single one of their own callbacks in try/catch, because that’s super brittle and easy to forget one.

#11

A major part of a transition is rendering all the new templates on destination routes, but since all rendering happens on the ‘render’ queue, if someone TypeErrors in didInsertElement, your app will break and be left in an invalid state, at least in present-day Ember; are you saying that these hooks too should take place in error-swallowing land and then cause the transition to reject?

The error (and on beta loading) actions exist to facilitate a lovely API for handling async, not just all errors ever to possibly happen in the course of a transition. If experience shows in a given production app that synchronous errors within hooks are actually a major source of error, then the user can opt-in by using promises or just plain ol try-catch. Again, think of didInsertElement and every other Ember hook we expose that we don’t overbearingly seal proof.

#12

So, I’m sympathetic to what you guys are saying, but I don’t think the existing browser tooling justifies trying to catch everything / intelligently respond to every error that comes along. I’m well aware of all the debugger tricks you can use to pinpoint the source of an error, but still, it’s just straight up still too painful to force this API on people. It bites everyone, all the time, in subtle ways, and it all screams of a highly premature decision to promisify all the things that even I was guilty of at first.

The original version of the PR I had made it possible to configure whether the hooks fired within a promise try/catch or not; Yehuda advised against it so I removed it; perhaps we should add it back in? I’m still not convinced that the sync-error-swallowing-and-treat-as-rejected-promises actually makes your app safer (there is still much work to do) and absolutely unconvinced that it’s worth the dev pain, but I’d be fine with exposing it as an enable-able pro-tip tool, but I think we are seriously damaging people’s dev experience and turning them away from Ember as it stands.

#13

Yeah, this isn’t really an Ember problem so much as a Javascript problem. So there’s no fantastic solution.

I see your point about error not being able to detect every possible failure.

I don’t think you need a new switch – Ember.onerror can already be the switch. People who aren’t setting onerror get no try/catch in handleErrors. People who want it explicitly opt-in by registering onerror.

So if you just wrap the route hooks in handleErrors, I think the behavior works well. By default, errors are untrapped. But people who want to trap them instead can.

#14