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

Reducer errors swallowed silently after epics' outputs #263

Closed
haf opened this Issue Jun 21, 2017 · 21 comments

Comments

Projects
None yet
6 participants
@haf

haf commented Jun 21, 2017

const reducer = (state = {}, action) => {
  switch (action.type) {
    case 'THING':
      return { ...state, thing: action.nonexistent.childprop }
    default:
      return state;
  }
};
export function sampleEpic(action$: Observable, store: Store) {
  return action$.ofType(ANYTHING)
    .concatMap(action =>
      fetch('/api/anything', { ... })
    .mergeMap(response => {
      switch (response.status) {
        case 409:
          return Observable.of(pleaseNo(email));
        case 201:
          return Observable.of({ type: 'THING' }, { type: 'NEVER_DISPATCHED' });
        default:
          throw new Error(`Unknown response code ${response.code}`);
      }
    });
}

The THING action causes a crash that is hidden from the user (me for the last 2 hours) so that it's very hard to see where things went wrong.

Adding catch to the end of the epic doesn't make a difference.

All middleware a successfully run.

@BerkeleyTrue

This comment has been minimized.

Collaborator

BerkeleyTrue commented Jun 21, 2017

I can't see how redux-observable would catch errors thrown in your reducer. RO does not contain your reducer or any other redux aspect other than the epics themselves. Also, there aren't cases where RxJS would swallow errors like promises would. The default is always to throw errors if unhandled by the user.

I'm also failing to see how the THING action would crash as there are no syntax errors present

 return { ...state, thing: action.nonexistent }

The result of this line of code is a state with the thing key set to undefined.

@haf

This comment has been minimized.

haf commented Jun 21, 2017

Perhaps it's a redux bug then?

.childprop in sample now.

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 21, 2017

I'm digging into this. Right now my findings are that RxJS did not swallow this error in 5.0.3 but started to in 5.1.0. https://jsbin.com/gehasuy/edit?html,js,output

It's not yet clear if this is a bug in Rx or if we were relying on buggy behavior that was fixed in 5.1.0. My initial (biased) reaction is that Rx should not be swallowing this error condition but it's possible I'm missing something.

Will continue digging. If anyone else wants to do so as well, you just need to pause your debugging and step through to see what happens.


It seems to be somehow related to merging into an observable of multiple items Observable.of({ type: 'SECOND' }, { type: 'THIRD' }). Only a single item and it correctly propagates the errors.... Observable.of({ type: 'SECOND' }). 🤔

@jayphelps jayphelps added bug confirmed and removed blocked labels Jun 21, 2017

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 21, 2017

EDIT: this particular situation no longer gets swallowed, but some other cases still do.


Minimum reproduction:

https://jsbin.com/dukixu/edit?html,js,output

const input$ = new Rx.Subject();

input$
  .mergeMap(() => Rx.Observable.of(1, 2))
  .subscribe(d => {
    throw new Error('some error');
  });

input$.next();

Also appears related to Subject usage (but could be a red herring)

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 21, 2017

Similar thing was reported and fixed ReactiveX/rxjs#2565 but it did not fix this issue for us. Discussing here: ReactiveX/rxjs#2618

@jayphelps

This comment has been minimized.

Member

jayphelps commented Aug 16, 2017

This PR is supposed to fix it: ReactiveX/rxjs#2796

@shakyShane

This comment has been minimized.

shakyShane commented Sep 19, 2017

This is still a bug I'm afraid even on the latest RxJS (5.4.3)

https://jsbin.com/xevuwecojo/1/edit?html,js,console

@jayphelps

This comment has been minimized.

Member

jayphelps commented Sep 19, 2017

The fix in that PR has not been released (5.4.3 came out before it).

@jayphelps

This comment has been minimized.

Member

jayphelps commented Sep 28, 2017

I can confirm that PR only fixed some cases, not all of them. ReactiveX/rxjs#2813 is tracking the latest on this issue.

You can work around this, if you want, by using something like this:

const rootEpic = (...args) =>
  combineEpics(epic1, epic2, ...etc)(...args)
    .do({
      error: e => setTimeout(() => { throw e; })
    });

The stack trace might not be ideal, depending on your browser, but you'll at least see the error. You could use console.error instead of setTimeout + throw but console.error isn't supported in older browsers!

@maxkostow

This comment has been minimized.

Contributor

maxkostow commented Oct 5, 2017

For anyone looking to implement the workaround, combineEpics actually returns a function so you will have to wrap it with something like

function combineEpicsWithError(...epics) {
    const combinedEpics = combineEpics(...epics)
    return function epicWithError(...args) {
        return combinedEpics(...args).do({
            error: e => {
                setTimeout(() => {
                    throw e
                }, 0)
            },
        })
    }
}

@jayphelps is it too late to recover from the error at this point? If you have one misbehaving epic, but the rest are totally fine, is it possible to not error the observable?

@jayphelps

This comment has been minimized.

Member

jayphelps commented Oct 6, 2017

@maxkostow whoops you're right, I just fixed my example. Thanks! 😄

is it too late to recover from the error at this point? If you have one misbehaving epic, but the rest are totally fine, is it possible to not error the observable?

If you apply catch like combineEpics(...epics)(...args).catch(...) it's too late--all your other epics have already been unsubscribed so while you could restart them (by returning the source observable which is the second argument) they would have lost any local state. Usually that's not a big deal because the only state in your epics themselves should be transient state; like the state debounceTime maintains, etc. All else should be in the redux store.

If instead you want to protect each of them from terminating the others, you can call each epic individually and add catch on each resulting stream.

Here's an example of that, but it may be hard to grok since it uses a lot of function composition:

const combineAndIsolateEpics = (...epics) => (...args) => {
  const isolatedEpics = epics.map(epic =>
    (...args) =>
      epic(...args)
        .catch((e, source) => {
          console.error(`${epic.name} terminated with error: ${e.message}, restarting it...`);
          return source;
        })
  );

  return combineEpics(...isolatedEpics)(...args);
};

const rootEpic = combineAndIsolateEpics(epic1, epic2);

Now if any individual epic errors out, we'll log the error message and restart it, but the other epics will continue with no knowledge that it happened and maintain any state they had.

We've talked about whether this (or something else) should actually be how it works by default in #94, but haven't reached a solid consensus yet. But we should at least document these patterns probably... 😞

There are potential issues with restarting epics after they error out, if the devs aren't strictly aware of the implications. e.g. what if the redux store is in a different state than the epic expects? Does the epic emit anything at start up? What if the error happens infinitely so it's erroring and restarting synchronously, locking up the browser? etc

@maxkostow

This comment has been minimized.

Contributor

maxkostow commented Oct 6, 2017

Good to know. I got confused because I ran into the swallowing issue (ReactiveX/rxjs#2813) and conflated it with killing the epic. I ended up catching each epic and having it rethrow async and terminate.

Jerry-Hong pushed a commit to Jerry-Hong/redux-observable that referenced this issue Dec 4, 2017

Jerry-Hong pushed a commit to Jerry-Hong/redux-observable that referenced this issue Dec 4, 2017

@Jerry-Hong Jerry-Hong referenced this issue Dec 4, 2017

Merged

Always log reducers exception #263 #379

2 of 2 tasks complete

@jayphelps jayphelps closed this in 56c1903 Dec 5, 2017

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 5, 2017

Reopening. We merged a workaround where we logged all errors from reducers but I'm concerned about it or any other similar solution. See the discussion in #379.

@jayphelps jayphelps reopened this Dec 5, 2017

@jayphelps

This comment has been minimized.

Member

jayphelps commented Dec 22, 2017

This, fingers crossed, should be fixed upstream now in rxjs 5.5.6 that was just released. Please let me know if you have cases where it did not (include reproduction) https://github.com/ReactiveX/rxjs/blob/stable/CHANGELOG.md#556-2017-12-21

The fix itself was simple, but was suuuuper not easy to find and even harder to prove it fixed the problem in a minimum way. The bright side is that all the related confusing crap is removed in master in preparation for v6, which changes error rethrowing to align with the latest Observable spec.

@jayphelps jayphelps closed this Dec 22, 2017

@BerkeleyTrue

This comment has been minimized.

Collaborator

BerkeleyTrue commented Dec 22, 2017

@jayphelps nice work! 🤞

@anton164

This comment has been minimized.

anton164 commented Jun 6, 2018

@jayphelps Unsure how this is fixed in latest stable release? I'm using redux-observable 0.18.0 and 5.5.6 of RxJs, but this is still causing all of the errors from my reducers to be swallowed. Do you know of a workaround for that?

I need them to bubble up so that I can log via window.onerror (or in some other way).

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 6, 2018

Unsure how this is fixed in latest stable release? I'm using redux-observable 0.18.0 and 5.5.6 of RxJs, but this is still causing all of the errors from my reducers to be swallowed

It's fixed in the sense that the errors log to the console via console.error, so they are not swallowed. But it is true that they won't reach window.onerror.

This is no longer an issue in the latest alpha and beta releases of redux-observable because it requires rxjs v6 which no longer synchronously rethrows errors instead throwing them async on a new callstack, similar to Promises--except they are reported through window.onerror whereas Promise errors are only reported on window.unhandledrejection.

As far as making a fix for 0.18.0, I'm open to releasing 0.19.0 if there's some way to better rethrow the error? We could rethrow it async setTimeout(() => { throw e; }). While not ideal because now we're changing the timing of the error, it might the lesser of two evils since it would now report in window.onerror.

I'm open to alternatives 👍

@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 6, 2018

Technically we could remove the error catching entirely because the fix I did upstream in rxjs https://github.com/ReactiveX/rxjs/blob/stable/CHANGELOG.md#556-2017-12-21 fixed the actual swallowing issue, the console.error was just a workaround. The only issue with removing it is that anyone who has not yet updated their rxjs version high enough to pick up the fix will have swallowed errors again. But perhaps that's indeed the correct solution, advising people to upgrade their rxjs instead.

@anton164

This comment has been minimized.

anton164 commented Jun 6, 2018

Thanks for the quick response!

I think removing the error catching should be fine (if it no longer has any purpose). If you outline in the 0.19.0 release that it plays nicer with RxJS 5.5.6 I think you're covered.

With 0.18.0, as you point out, there is no way to catch the errors because of the try/catch/console.error in createEpicMiddleware so logging errors is currently impossible so to me as a user of redux-observable it is much worse.

While I'm aiming to upgrade to 6.0.0 and 1.0.0 later on, that is a bigger leap.

jayphelps added a commit that referenced this issue Jun 6, 2018

fix(errors): errors from reducers are no longer caught and logged, in…
…stead are rethrown. related #263#issuecomment-395109222

BREAKING CHANGE: For 0.19.0 errors from reducers are no longer caught and console.error logged, instead they are just rethrown as before. This was a temporary workaround for a bug in rxjs where it would silently swallow errors. That bug has been fixed in 5.5.6+, so it is highly recommended you use _at least_ rxjs@5.5.6+ with this version of redux-observable. However, redux-observable is close to reaching 1.0.0-final which will require rxjs v6 and redux v4, if you'd like to start upgrading to it now you can use redux-observable@next (as of this writing 1.0.0-beta.1)
@jayphelps

This comment has been minimized.

Member

jayphelps commented Jun 6, 2018

released discussed change as redux-observable@0.19.0

@anton164

This comment has been minimized.

anton164 commented Jun 7, 2018

Awesome, thanks a lot for your responsiveness and help @jayphelps! ❤️ 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment