Skip to content
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

How to handle silent error in dispatch? #1060

Closed
feifanzhou opened this issue Nov 20, 2015 · 10 comments
Closed

How to handle silent error in dispatch? #1060

feifanzhou opened this issue Nov 20, 2015 · 10 comments

Comments

@feifanzhou
Copy link

Currently, dispatch may suppress silent errors that break the program. I had this happen here:
https://github.com/rackt/redux/blob/bb9fa19cf2a22d0149dcf134e36404a02d93b2b3/src/createStore.js#L126

Modifying to the following surfaces the issue.
screen shot 2015-11-20 at 12 09 47

I can submit a PR, but what's the recommended way to handle?

@joonhocho
Copy link

You don't need to return a result for forEach loop.
Your code will make listeners to continue running after error.
Other than that, +1.

@feifanzhou
Copy link
Author

Didn't realize it was supposed to stop in case of error … hence why I'm asking about the right approach. My main concern was surfacing the silent errors (which I'd spent way too long trying to debug, without knowing where the error was coming from)

@gaearon
Copy link
Contributor

gaearon commented Nov 23, 2015

I think you might be misunderstanding some details of how try..catch..finally works.
If the listener throws, the error will propagate through the call stack.
There is nothing we need to do in Redux to make it work—this is just JavaScript semantics.

If you have silent errors, it’s probably related to the code outside the dispatch() call.
It is possible it contains a catch handler up the call stack which eats the error.
For example, this may happen inside a Promise then() callback if your Promise polyfill doesn't warn about unhandled rejections.

If you're sure this is a Redux bug, please create a full reproducing example.

@gaearon gaearon closed this as completed Nov 23, 2015
@gaearon
Copy link
Contributor

gaearon commented Nov 23, 2015

Perhaps I misunderstood you.

Are you saying Redux should try to invoke all listeners even if one of them failed? I'm not sure. What if many subscribers error'd? Should we just accumulate the errors? Throw just the first one and silently ignore the others?

Right now our behavior is very simple—we just don't tolerate the errors at all. An app throwing an error in a subscriber is likely broken anyway, and at least we can fail hard early if this happens. If you're hesitant, you can always make a “store enhancer” (a la https://github.com/tappleby/redux-batched-subscribe) to implement the behavior you're describing. You could then publish it as a package! 😉

@feifanzhou
Copy link
Author

If the listener throws, the error will propagate through the call stack

That's what I thought … but the code I posted above did surface an issue with a render method not returning a valid component, which was silently breaking otherwise.

@gaearon
Copy link
Contributor

gaearon commented Nov 24, 2015

If it’s silently breaking, it’s a problem somewhere else, but definitely not in Redux core.
If you can create a counter example showing how Redux makes exceptions disappear silently please do.

@face
Copy link

face commented Aug 13, 2018

I think there is a bug in Chrome (I'm on 68.0.3440.106). My reducer is silently breaking, and I have tracked it to this try..finally: https://github.com/reduxjs/redux/blob/master/src/createStore.js#L184

However, that syntax is fine and should not eat exceptions. If I add } catch(e) { console.error(e); } finally {... to createStore everything works as expected. But, this should not be necessary, hence I suspect the bug is in Chrome.

I'll try and reproduce this in a stand alone example, and then open a bug with Google. However, @gaearon, if you are willing to add the workaround above to redux, let me know and I'll open a PR to redux.

@markerikson
Copy link
Contributor

@face : Dan isn't an active maintainer any more - @timdorr and I are.

If you can reproduce the issue, please let us know.

@face
Copy link

face commented Aug 14, 2018

Thank you @markerikson, working on it, but so far it doesn't happen in a simple example (even with some of the same modules we use).

Though, I should have had a throw, not a console.error above which changes my assumptions. Still strange though as with a throw, stepping through the code nothing else seems to catch it and yet my error disappears. I'll update here when I figure it out.

@face
Copy link

face commented Aug 14, 2018

Ok, sorry for the noise @markerikson caused from my wrong assumption by putting a console.error. That caused me to take another look, and a simple backtrace showed me the issue was in our legacy code. Redux and chrome worked as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants