[BUGFIX release] mark handled errors and dont reraise #12549

minasmart · 2015-11-03T03:04:23Z

Ember 2.1 introduced a regression where all errors during a transition
were thrown, even when those errors had been handled in a route's error
action. This PR adds a test to identify the regression, and adds some
logic and state to identify errors that have been handled, and not
reraise them, while maintaining the desired behaviour of throwing errors
that aren't handled.

This is especially valuable in any application that tests error
handling. Since all errors were being thrown regardless of handling,
failed assertions were being registered by qunit, and it became
impossible to test application flow that entered the error hook.

Further details about this issue are here: #12547

rwjblue · 2015-11-03T13:00:15Z

packages/ember-routing/lib/system/router.js

@@ -865,6 +877,10 @@ function triggerEvent(handlerInfos, ignoreFailure, args) {
      if (handler.actions[name].apply(handler, args) === true) {
        eventWasHandled = true;
      } else {
+        // Should only hit here if a non-bubbling error action is triggered on a route.
+        if (name === 'error') {
+          markErrorAsHandled(args[0]);


What is args[0] here?

args[0] is the error. It's the first argument to the error action. Should I call it out as a var?

It isn't necessary to make it a variable, but I do not think that the following does what we want:

e1 = new Error('foo'); e2 = new Error('foo'); obj = {}; obj[e1] = true; obj[e2] = false; obj[e1] // => false WAT

rwjblue · 2015-11-03T15:21:17Z

Discussed in slack with @minasmart, I believe we have a path forward.

Also, this will be a [BUGFIX release].

rwjblue · 2015-11-03T18:20:37Z

Looks like this is causing a failure in the node test suite.

You can reproduce via:

ember server

# in another terminal
node bin/run-node-tests.js

minasmart · 2015-11-03T18:55:03Z

Will take a look.

minasmart · 2015-11-03T22:01:50Z

This branch is based off of master, and this test seems to consistently fail on master too.

minasmart · 2015-11-03T22:02:58Z

Actually, that's wrong. It logs out the error but doesn't fail an assertion. Are errors not sent up with throw inside the node process?

rwjblue · 2015-11-03T22:40:35Z

Which test is throwing?

minasmart · 2015-11-04T14:29:21Z

"FastBoot: route error"

The error is being thrown from ember-runtime/ext/rsvp inside the onerrorDefault method. And it looks like that's the desired outcome. It's just that the catch on fastbootVisit isn't completely intercepting the error.

minasmart · 2015-11-04T15:06:20Z

Without understanding a little bit more about how App.visit works, I'm not sure this is within my ability to fix. It gets even weirder. If I wrap the promises in assert.throws there's still an issue. Same if I put a .catch on the RSVP.all which indicates that this is a promise that's being resolved outside the scope of what's invoked in the test. The odd thing is, I can't figure out how to tie it back to the change that I made.

minasmart · 2015-11-04T16:48:43Z

More research, if I just don't throw the error in the catch for node, then all the node tests pass. Which also feels wrong.

rwjblue · 2015-11-04T16:59:51Z

Yes, we definitely need to rethrow unhandled errors.

minasmart · 2015-11-04T19:05:45Z

Remaining error is a sauce/IE 10 error, and I don't know how to check the actual output of the test suite for that one.

rwjblue · 2015-11-04T19:07:16Z

I restarted the sauce labs tests. Likely it is a timeout issue (we have been fighting them for a while).

minasmart · 2015-11-04T19:08:25Z

Let's hope.

rwjblue · 2015-11-04T19:30:18Z

Looks good to me.

@stefanpenner is going to review this when he gets a minute to help ensure we have thought through all the angles on this one...

minasmart · 2015-11-04T19:32:02Z

Great! Thanks again for all your help!

stefanpenner · 2015-11-04T20:49:40Z

Ember 2.1 introduced a regression where all errors during a transition

has someone isolated what code regressed this?

stefanpenner · 2015-11-04T20:50:12Z

ah 854d3be

stefanpenner · 2015-11-04T20:53:44Z

packages/ember-routing/lib/system/router.js

@@ -705,6 +706,24 @@ var EmberRouter = EmberObject.extend(Evented, {
      run.cancel(this._slowTransitionTimer);
    }
    this._slowTransitionTimer = null;
+  },
+
+  _handledErrors: computed(function() {


If we go down this path, it seems simpler to just brand the given error with a hasBeenHandled symbol. No need to keep these data-structures around.

error[hasBeenHandledSymbol] = true

although this wont work with primitive values, i think thats fine.

Directly branding the error seems ok to me (it really isn't a big change for this PR as the basic infrastructure to brand is done), but I believe that we have a few tests in Ember itself that reject("asdf") or throw "asdf" which will need to be changed.

That was my first attempt, but the problem I hit was that there's no insurance that the error isn't a primitive type, and there are several tests where it IS a primitive type. This wouldn't be a difficult change (to fire an assertion of the type isn't an object, and update the tests) but it would probably be a problem for a lot of apps. It would need to be noted/documented/blogged about or something.

stefanpenner · 2015-11-04T20:54:43Z

I am slightly concerned that a "handled error" that is then re-thrown, will be incorrectly branded as handled and will be eaten.

I'll try to carve out some time shortly, to do a deeper dive.
My hunch, is that we have something else sideways forcing us down this specific path.

rwjblue · 2015-11-04T21:00:41Z

@stefanpenner - _markErrorAsHandled is only called in the scenario that an error action was found and does not return true. This means that the error handler has handled the error (return true meaning do not bubble further). We are not re-throwing a handled error.

stefanpenner · 2015-11-04T21:02:25Z

@rwjblue nothing prevents the user from re-throwing, or passing the exception through another promise chain, that may or may not rethrow it later. At which point, it would be totally and unexpectedly ignored.

minasmart · 2015-11-04T21:42:54Z

@stefanpenner another promise chain would resolve outside of the transition, so it shouldn't be a problem there (handled errors are only checked as part of the transition's promise). The only way the error could be re-injected into the same transition would be bubbling up. Unless I'm missing a step where after the error hook gets fired Ember makes an effort to resolve more promises as part of the same transition.

minasmart · 2015-11-04T21:52:58Z

@stefanpenner Also, just checked. If I have an error hook like this:

error(e) {
  throw e;
  return false;
}

The error isn't swallowed.

Similarly, if the error is thrown as part of a separate promise chain, or other asynchronous task like this:

error(e) {
  Ember.run.next(function() {
    throw e;
  });
  return false;
}

The error is still not swallowed. As part of the transition, it's not re-raised, it gets thrown after the promise completes.

minasmart · 2015-11-05T15:29:25Z

Also, @stefanpenner I can write more tests to model the cases you're talking about, if you want more insurance!

rwjblue · 2015-11-11T15:11:25Z

nothing prevents the user from re-throwing, or passing the exception through another promise chain, that may or may not rethrow it later. At which point, it would be totally and unexpectedly ignored.

We are catching and ignoring the error only if their error action hook returns anything but true. If they throw in the action hook or do additional chaining and later throw, it will not be ignored.

Also, @stefanpenner I can write more tests to model the cases you're talking about, if you want more insurance!

@minasmart - A couple tests to show that my above statement is true would definitely be nice.

minasmart · 2015-11-16T16:21:31Z

Should have time to get tests up later this week.

rwjblue · 2015-11-16T16:25:01Z

@minasmart - That would be awesome, thank you!

rwjblue · 2015-11-24T16:19:48Z

@minasmart - Have you been able to make any progress here?

minasmart · 2015-11-24T21:34:53Z

@rwjblue been extremely busy. Will push them up tomorrow.

jkeen · 2015-11-24T21:40:02Z

@minasmart Thanks so much for identifying this problem and fixing it! Had been struggling for days to track down what was going on with some failures until @rwjblue pointed me here.

rwjblue · 2015-11-24T22:25:34Z

@minasmart - I definitely understand that, thanks for keeping at this!

@rwjblue

Ember 2.1 introduced a regression where all errors during a transition were thrown, even when those errors had been handled in a route's error action. This PR adds a test to identify the regression, and adds some logic and state to identify errors that have been handled, and not reraise them, while maintaining the desired behaviour of throwing errors that aren't handled. This is especially valuable in any application that tests error handling. Since all errors were being thrown regardless of handling, failed assertions were being registered by qunit, and it became impossible to test application flow that entered the error hook. Further details about this issue are here: emberjs#12547 Remove leftover debugger This addresses an issue where handled errors are still thrown The added test passes on Ember 2.0. It fails on Ember 2.1 because even though the error is not bubbled, the application throws the handled error. This is contrary to what the documentation indicates. Full details about this issue are here: emberjs#12547 This PR also adds a very naive implementation of how to flag an error as having been handled and prevents reraising, while retaining the functionality of throwing errors that aren't caught, or are bubbled from the application route Make error checking methods a part of the router instance Attach error caching methods to EmberRouter, and use guids for errors Errors used as keys were just getting `.toString()`ed, so multiple `Error`s or hashes would all have the same key. This update uses ember `guidFor` to uniquely identify errors. Thanks for the help @rwjblue! When creating a new promise, resolve it. Don't fork the promise Tests expecting errors to bubble up, should bubble errors

This provides insurance that handled errors aren't swallowed

minasmart · 2015-11-26T18:27:21Z

More tests added! Lots of rethrowing different ways.

rwjblue · 2015-11-26T18:34:27Z

Thank you!

@stefanpenner - Can you re-review? We can add more scenario tests if you still think there are missing scenarios here...

minasmart · 2015-11-26T18:40:43Z

It looks like sauce tests timing out again 😞

stefanpenner · 2015-11-26T18:40:46Z

cal reminder set for tomorrow morning.

swalkinshaw · 2015-12-21T15:17:35Z

@stefanpenner friendly reminder about this :) would love to see it get into 2.3.0

rwjblue · 2015-12-21T15:25:26Z

@swalkinshaw - Agreed.

rwjblue · 2015-12-27T16:19:55Z

Rebased and slightly tweaked in #12752. Planning to land into 2.3.0 and current stable branch (2.2.x).

minasmart changed the title ~~Bugfix mark handled errors and dont reraise~~ [BUGFIX] mark handled errors and dont reraise Nov 3, 2015

rwjblue reviewed Nov 3, 2015
View reviewed changes

minasmart force-pushed the BUGFIX-mark-handled-errors-and-dont-reraise branch from 8fba3c2 to 1217cdb Compare November 3, 2015 15:48

minasmart changed the title ~~[BUGFIX] mark handled errors and dont reraise~~ [BUGFIX release] mark handled errors and dont reraise Nov 3, 2015

rwjblue mentioned this pull request Nov 3, 2015

[BUG] Test identifying Ember 2.1 error-hook bubbling issues #12548

Closed

minasmart force-pushed the BUGFIX-mark-handled-errors-and-dont-reraise branch from 1774576 to 25c14ab Compare November 4, 2015 18:28

stefanpenner reviewed Nov 4, 2015
View reviewed changes

rwjblue mentioned this pull request Nov 6, 2015

[BUG] RSVP's different onerrorDefault behaviour in testing breaks tests #12567

Closed

kevinansfield added a commit to kevinansfield/Ghost that referenced this pull request Nov 6, 2015

test against emberjs/ember.js#12549

0c15eaa

minasmart force-pushed the BUGFIX-mark-handled-errors-and-dont-reraise branch from 25c14ab to dd160a6 Compare November 26, 2015 17:28

Add tests with multiple re-throw stratiegies.

d1bb80b

This provides insurance that handled errors aren't swallowed

rwjblue mentioned this pull request Dec 10, 2015

Errors Thrown in Route Model Hooks Don't Hit Global Handler #12669

Closed

stefanpenner self-assigned this Dec 21, 2015

rwjblue mentioned this pull request Dec 27, 2015

Do not re-raise on errors handled in route error action. #12752

Merged

rwjblue closed this Dec 27, 2015

pixelhandler mentioned this pull request Jan 9, 2016

Test.adapter.exception forces acceptance test to fail (RSVP error fails test) #12791

Closed

[BUGFIX release] mark handled errors and dont reraise #12549

[BUGFIX release] mark handled errors and dont reraise #12549

Conversation

minasmart commented Nov 3, 2015

rwjblue Nov 3, 2015

Choose a reason for hiding this comment

minasmart Nov 3, 2015

Choose a reason for hiding this comment

rwjblue Nov 3, 2015

Choose a reason for hiding this comment

rwjblue commented Nov 3, 2015

rwjblue commented Nov 3, 2015

minasmart commented Nov 3, 2015

minasmart commented Nov 3, 2015

minasmart commented Nov 3, 2015

rwjblue commented Nov 3, 2015

minasmart commented Nov 4, 2015

minasmart commented Nov 4, 2015

minasmart commented Nov 4, 2015

rwjblue commented Nov 4, 2015

minasmart commented Nov 4, 2015

rwjblue commented Nov 4, 2015

minasmart commented Nov 4, 2015

rwjblue commented Nov 4, 2015

minasmart commented Nov 4, 2015

stefanpenner commented Nov 4, 2015

stefanpenner commented Nov 4, 2015

stefanpenner Nov 4, 2015

Choose a reason for hiding this comment

stefanpenner Nov 4, 2015

Choose a reason for hiding this comment

rwjblue Nov 4, 2015

Choose a reason for hiding this comment

minasmart Nov 4, 2015

Choose a reason for hiding this comment

stefanpenner commented Nov 4, 2015

rwjblue commented Nov 4, 2015

stefanpenner commented Nov 4, 2015

minasmart commented Nov 4, 2015

minasmart commented Nov 4, 2015

minasmart commented Nov 5, 2015

rwjblue commented Nov 11, 2015

minasmart commented Nov 16, 2015

rwjblue commented Nov 16, 2015

rwjblue commented Nov 24, 2015

minasmart commented Nov 24, 2015

jkeen commented Nov 24, 2015

rwjblue commented Nov 24, 2015

minasmart commented Nov 26, 2015

rwjblue commented Nov 26, 2015

minasmart commented Nov 26, 2015

stefanpenner commented Nov 26, 2015

swalkinshaw commented Dec 21, 2015

rwjblue commented Dec 21, 2015

rwjblue commented Dec 27, 2015