-
Notifications
You must be signed in to change notification settings - Fork 34
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
Hidden exceptions #9
Comments
I can add exposing exceptions here, but we do not want to throw at this point because other subscriptions will be missed - a subscription should handle any exceptions in its own code that is run on publish. Also, throwing here would mean exposing an exception from a subscription, to the publisher and the publisher should have no knowledge, or care about subscribers. Hope that makes sense. |
Think of the case where I don't have my own code in a try/catch. In that case you are going to handle the exception but as you do nothing with it you "hide it", that's what's dangerous and what can take hours to debug. See this: https://rules.sonarsource.com/csharp/type/Vulnerability/RSPEC-2486 |
This is true, but I don't think throwing the exception here is the better option - for the reasons I mentioned before. This code doesn't care about exceptions raised by the code it calls into, and definetly doesn't want other subscribers to not get messages because a unrelated different subscriber had an error. What do you think of some event that you can subscribe to that exposes any exceptions raised, like a global handler? So you can have something that subscribes to that and gets events raised on any exceptions here? Or, I guess I could expose some configuration that would allow exceptions to be thrown so you could specify the behaviour you want. Which would be your preference on this? |
I would choose the second option, setting if you want to 'false' by default to keep it working the same. |
Added option to throw subscriber exceptions in 1.0.7 |
I think that doing this it's dangerous, you shouldn't do this:
https://github.com/mxjones/RedBus/blob/master/src/Redbus/Redbus/EventBus.cs#L86
The text was updated successfully, but these errors were encountered: