ember-fastboot / ember-cli-fastboot

Server-side rendering for Ember.js apps
http://ember-fastboot.com/
MIT License
851 stars 160 forks source link

transition.abort() causes 500 responses #202

Open arjansingh opened 8 years ago

arjansingh commented 8 years ago

As @rwjblue pointed out in #195, aborting a transition in FastBoot causes a TransitionAborted error to be thrown by the server, resulting in the request failing and the response returning a 500 error.

Here is an example stack trace:

Error: TransitionAborted at handleReject (/code/ember-bugs/dist/fastboot/vendor.js:14132:17)
 at tryCatch (/code/ember-bugs/dist/fastboot/vendor.js:63911:14)
 at invokeCallback (/code/ember-bugs/dist/fastboot/vendor.js:63926:15)
 at publish (/code/ember-bugs/dist/fastboot/vendor.js:63894:9)
 at publishRejection (/code/ember-bugs/dist/fastboot/vendor.js:63829:5)
 at /code/ember-bugs/dist/fastboot/vendor.js:42159:7 at Queue.invokeWithOnError (/code/ember-bugs/dist/fastboot/vendor.js:10447:18)
 at Object.Queue.flush (/code/ember-bugs/dist/fastboot/vendor.js:10502:11)
 at Object.DeferredActionQueues.flush (/code/ember-bugs/dist/fastboot/vendor.js:10310:17)
 at Object.Backburner.end (/code/ember-bugs/dist/fastboot/vendor.js:10665:25)
 at [object Object]._onTimeout (/code/ember-bugs/dist/fastboot/vendor.js:11231:18)
 at Timer.listOnTimeout (timers.js:92:15)

I'll try to look at this when I get some time. If anyone has any thoughts on what might be causing this, please chime in.

jasonmit commented 8 years ago

It believe it's happening here: https://github.com/ember-fastboot/fastboot-express-middleware/blob/master/index.js#L54-L65

Similar to the handling of UnrecognizedURLError, we might want to also call next on TransitionAborted

jasonmit commented 8 years ago

Actually, I'm unsure how we should handle aborted transitions so I closed my related PR. What is the behavior that you are expecting?

tomdale commented 8 years ago

@arjansingh Ping when you get a chance

arjansingh commented 8 years ago

In my opinion the expected behavior on an abort should be to just continue execution of the code.

Aborts are usually followed with a transitionTo, which would result in 307 redirect: https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/app/locations/none.js#L29-L49

If there is no transition then the expected behavior from what I can see is to just render whatever route we are on with the information we have available, like we would for a non-fastbooted page.

Here's the code for abort() for reference: https://github.com/tildeio/router.js/blob/master/lib/router/transition.js#L184-L200

rwjblue commented 8 years ago

Confirm. I do not think an aborted transition should be directly considered an error condition.

Gaurav0 commented 7 years ago

@rwjblue Reproduction without explicit transition.abort: https://github.com/Gaurav0/fastboot-issue-202.git

rwjblue commented 7 years ago

I did a bit of investigation into this today (thanks to @Gaurav0 reproduction), and have isolated the issue.

This error is currently (speaking about Ember 2.10+) only occurring when a .transitionTo / .redirectTo is being done asynchronously in afterModel. Doing the redirect in the redirect or model hooks work without an error.

In afterModel the following would work without error:

afterModel() {
  this.transitionTo('something');
}

And this would not:

afterModel() {
  return Ember.RSVP.Promise.resolve()
    .then(() => {
      this.transitionTo('something');
    });
}

The reason for this, is that we currently only allow a transition to be aborted while it is underway (and we use this.get('router').router.activeTransition to track that here), but once afterModel has ran we immediately clear the activeTransition (see here).

rwjblue commented 7 years ago

I commented on ember-learn/ember-api-docs#128 with a more concrete solution over there.

rtablada commented 2 years ago

Ran into this today still with fastboot.

Was working on an application started using Engage and went to setup redirects to a login page and hit the same error.

I went to recreate the error in a fresh Ember app thinking there were still underlying router.js errors and missing catch assertions in Ember, but the error now seems to be isolated to applications using fastboot.

image

https://github.com/flightplanjs/example-app-2


Some of the above "solutions" and OSS commits skip redirects with fastboot, but this is a bad solution since it would then cause the application to only work in rehydration mode, and would require full boot and client side paint along with potential state leaks of protected routes.

alexraputa commented 2 years ago

@rtablada, Can u try:

async beforeModel(transition: Transition) {
  transition = this.router.transitionTo('login'); // or this.router.replaceWith('login');

  return await super.beforeModel(transition);
}
rtablada commented 2 years ago

@alexraputa still has the same issue.