bugfix: exception raised by error handler terminates the async msg dispatcher

[yaronyam]

Exception in async event handler followed by exception in error handler prevents further asyc messages to be dispatched. Submitting bugfix + UT that covers the scenario

[bennidi]

Can you move the fix into the handlePublicationError method itself, please? Make another PR and I will accept it. Thanks for your contribution.

[yaronyam]

handlePublicationError() is called in synchronized context as well. As of now, error handler can deliberately throw exception to signal message consumption failure to the publisher of a synchronized message. If the fix is moved to handlePublicationError() - synchronized publishers won't get a direct indication of message handling failures. In my use case, HTTP request handlers dispatch synchronized messages and need to return a different response in case of a message consumption failure. Arguably, such design may raise questions whether a message bus is appropriate here, but the use of a message bus still provides component decoupling (though the publisher is aware of consumption failures) and a generic implementation for both sync and async scenarios. WDYT?

[bennidi]

I don't have the time to look into the code right now, but I believe that handlePublicationError() is not meant to throw any exception that bubbles up back to the caller. The preferred (and only!) way of handling errors is via the error handler(s). And what exactly do you mean by "synchronized" publishers? You refer to synchronous dispatch I believe? Maybe it would be necessary to use MessagePublication as that object to communicate errors (in sync and aysnc scenarios). Maybe you can investigate that idea a bit further?

[yaronyam]

Yes, by "synchronized" I ment synchronous dispatch. sorry about the ambiguity. I'll replace this PR with a PR that moves the fix to handlePublicationError() as suggested. later I'll raise another PR to reflect message consumption failure to the publisher. That PR will include:

• adding 2 methods to IMessagePublication interface: boolean isError(); List getErrors()
• methods publishing sync messages - IPublicationCommand.now(), MBassador.publish() - will return IMessagePublication just as the async publishing methods

makes sense?

[bennidi]

Yes, that sounds very reasonable :+1: Thank you!

[yaronyam]

This PR is updated with the fix moved to handlePublicationError() and ready to be merged as far as I'm concerned

[bennidi]

I just reviewed the code from the PR and came up with a solution that would support this use case and actually improve the design of the current code base. I will try to sum it up and maybe you like to implement it.

1. The IMessageDispatcher Interface can be simplified by removing the message from its signature (message is contained in publication, i.e. can simply be replaced)
2. The HandlerInvocation is not the place where exceptions should be handled. The signature can be changed to indicate that they throw exceptions. Handlers are just called and if an exception occurs it is dealt with in the calling code.
3. The place where exceptions are handled is in the dispatcher. It can create the publication error and pass it to the MessagePublication.
4. The MessagePublication becomes context aware and as such gains access to the error handlers. It will then also store the errors handled by it. Done!

I hope I could make myself clear with those descriptions. Do you feel like implementing this?

[yaronyam]

Sorry, for the late reply. Yes, I'll re-implement this if there's a better way. So if my understanding is correct, ReflectiveHandlerInvocation should catch and throw rather than catch and handle, MessageDispatcher#dispatch() will catch and handle. But things get complicated with async msg handlers. AsynchronousHandlerInvocation cannot let the exception bubble up from the invoke() method since it's thrown from a different thread. If you have any other insights please let me know

[bennidi]

You are totally right. The proposed design would only work for synchronous handlers. Well, then the publication must be passed down the chain, like you already did. But I would relocate all error handling code to the message publication itself. Handlers just catch and "redirect" so to speak.

[yaronyam]

Not sure I got it. Looking at method net.engio.mbassy.bus.MBassador#publish as a use case: invocation of createMessagePublication() might throw exception and - IMessagePublication publication - might be null so it cannot handle the error but the PublicationError still needs to be created and passed to the error handlers. Created a gist to clarify this: https://gist.github.com/yaronyam/96f9457a6dab3e1c88f4

[bwzhang2011]

@bennidi, any update with such issue ?

[bennidi]

I just made a release with the discussed code adaptations. You now have access to the MessagePublication and can check for errors. I hope the feature makes you happy.