Provide custom exception handling in EventBus

[GoogleCodeExporter]

I would love to replace our implementation of a synchronous event bus with the 
one from Guava. They do pretty much the same thing except for one notable 
difference: Ours propagates exceptions that happen during event handling since 
event handling must occur in the same database transaction as the code that 
raised the event. Is there any chance to make this configurable 
(flag/subclassing)?

Cheers
Thomas

Original issue reported on code.google.com by toellr...@gmail.com on 20 Oct 2011 at 12:05

[GoogleCodeExporter]

What happens when an Exception is thrown? Are subsequent handlers not notified 
of the event object? That seems unacceptable because there's no way to 
configure the order of handling.

Or would you want a "public Set<Exception> postAll (Object event)" ?

Original comment by ray.j.gr...@gmail.com on 1 Nov 2011 at 12:34

[GoogleCodeExporter]

In our case they would all be transactional and therefore wouldn't need to be 
notified explicitly because of the rollback.

Original comment by toellr...@gmail.com on 1 Nov 2011 at 4:12

[GoogleCodeExporter]

I concur that it's not at all clear how the semantics of this would work, due 
to the lack of ordering on handlers.  Would all the other handlers still be 
called?

Original comment by wasserman.louis on 15 Nov 2011 at 4:49

[GoogleCodeExporter]

I would have the dispatching stop similar to the way SimpleEventBus of the 
CQRS-Framework Axon works 
(http://www.axonframework.org/docs/1.2/event-processing.html)

Original comment by toellr...@gmail.com on 15 Nov 2011 at 8:24

[GoogleCodeExporter]

I think that framework have stronger ordering constraints for its handlers, 
however, and I'm not sure that behavior satisfies the principle of least 
astonishment.

For example, I don't think the JDK defines what order methods are iterated over 
with reflection, in which case there's no way to impose any ordering on several 
handlers in the same class.

Original comment by wasserman.louis on 15 Nov 2011 at 8:43

[GoogleCodeExporter]

If you guys feel my request doesn't "gel" with your idea of an EventBus please 
feel free to ignore it ;)

I can always continue to use our own implementation or start using Axon 
framework. Thanks for your interest!

Original comment by toellr...@gmail.com on 15 Nov 2011 at 8:57

[GoogleCodeExporter]

I didn't say that -- I think it's possible (likely?) that someone smarter than 
I am could figure out a sensible way of doing this.  I'm just trying to figure 
out if there's any approach I can think of that would meet Guava's typical 
standards for "clear and well-defined semantics."

Original comment by wasserman.louis on 17 Nov 2011 at 1:47

[GoogleCodeExporter]

Re: "I don't think the JDK defines what order methods are iterated over with 
reflection", not only does it not define it, but the order even changes between 
JDK 6 and 7.

Original comment by kevinb@google.com on 21 Nov 2011 at 10:51

[GoogleCodeExporter]

Indeed.  I think that's a pretty conclusive argument that there could never be 
a consistent, well-defined ordering on the handler order, assuming we're using 
the reflection-based register(Object) approach.

Original comment by wasserman.louis on 21 Nov 2011 at 10:56

[GoogleCodeExporter]

Original comment by cpov...@google.com on 5 Dec 2011 at 11:25

• Added labels: Type-Enhancement

[GoogleCodeExporter]

Firewalling event producers from any failure in subscribers was quite 
deliberate.  It was based on the notion of events being exchanged by separate 
components or library code, where a failure in one errant consumer shouldn't 
destroy all the other parts.  Propagating exceptions across EventBus also 
allows producers to (perhaps deliberately and maliciously) discover aspects of 
consumer code shape and operation by inspecting stack traces, which makes me 
uncomfortable.

For your application, it sounds like something like this might suffice:
1. Distribute the events in arbitrary order, as we do today.
2. Take note of whether any subscribers failed.  Skip any failed subscribers, 
as we do today.
3. Notify the event producer somehow (return value?) that one or more 
subscribers failed.

I would be hesitant to furnish the producer with a list of failed consumers, or 
to return a count (simply because I can't imagine how one would reasonably 
respond to a count).  A boolean, though, seems reasonable.

Would a feature like this help?

Original comment by cbiffle@google.com on 9 Dec 2011 at 6:28

[GoogleCodeExporter]

Original comment by fry@google.com on 10 Dec 2011 at 4:22

• Added labels: Package-EventBus

[GoogleCodeExporter]

A boolean return value from EventBus.post(Object event) that indicates
whether any handler threw an exception? That would work for me since on
false the event producer can throw a RuntimeException which would lead to a
rollback of the transaction.

Original comment by toellr...@gmail.com on 11 Dec 2011 at 1:32

[GoogleCodeExporter]

How would returning a boolean from post work with an AsyncEventBus? It would 
have to wait for all handlers to finish, which doesn't seem acceptable. I 
suppose post could return a ListenableFuture<Boolean> instead of just boolean, 
though that would create some overhead that seems unnecessary for most uses of 
EventBus.

Original comment by cgdec...@gmail.com on 11 Dec 2011 at 2:08

[GoogleCodeExporter]

As I said, it's probably for the best if I stick to another event bus 
implementation.

Original comment by toellr...@gmail.com on 12 Dec 2011 at 2:45

[GoogleCodeExporter]

After your objections I've rethought the design of the application and changed 
it to no longer use an event bus (it was just a prototype). You may close the 
feature request.

Original comment by toellr...@gmail.com on 16 Dec 2011 at 9:20

[GoogleCodeExporter]

Issue 868 has been merged into this issue.

Original comment by wasserman.louis on 25 Jan 2012 at 9:43

[GoogleCodeExporter]

Issue 868 suggested a much nicer way of doing this: when a handler fails, post 
a new event wrapping the exception, instead of doing a return value or 
something.  Then, you can optionally log such events.

...I can think of one major way this can go wrong: what happens if a subscriber 
to Object fails?  Then it'd post a SubscriberFailedEvent or whatever, then it'd 
receive the SubscriberFailedEvent and fail again, and would do an infinite 
loop.  We might have code that specifically breaks this loop by ignoring 
failures on failures -- whenever a subscriber fails when it processes a 
SubscriberFailedEvent.  I dunno.

Thoughts?

Original comment by wasserman.louis on 25 Jan 2012 at 9:46

[GoogleCodeExporter]

I see these issues are merged, makes sense indeed.
About your last scenario: I mentioned this already in the first post of Issue 
686:
"Only downside might be some extra semantics about the exception event. Namely 
that any exceptions for listeners to exception events are not re-posted. But 
this is not any different from the fact that there is no new event if no-one 
listens to the DeadEvent. :)"

Original comment by ewjmul...@gmail.com on 26 Jan 2012 at 8:28

[GoogleCodeExporter]

Sorry, I didn't parse that correctly originally.  It's totally accurate.

Original comment by wasserman.louis on 27 Jan 2012 at 1:34

[GoogleCodeExporter]

Thanks for articulating this problem to us, everyone!

We are considering introducing an "ExceptionEvent" and having the event bus 
post these events to itself upon subscriber failures.

But, any Throwable resulting from publishing an ExceptionEvent itself will 
probably just be dropped. (Or some anti-recursion policy.)

If you see any problems with this idea, let us know soon.

Original comment by kevinb@google.com on 31 Jan 2012 at 7:21

• Changed title: Publish subscriber exceptions back to the EventBus
• Changed state: Accepted
• Added labels: Milestone-Release12

[GoogleCodeExporter]

Query: why ExceptionEvent vs. ThrowableEvent?  Or is it just more readable that 
way?

Original comment by wasserman.louis on 31 Jan 2012 at 7:28

[GoogleCodeExporter]

Just jotted that quickly without thinking.

Though, "ThrowableEvent" does kind of sound like the event itself can be thrown.

Original comment by kevinb@google.com on 1 Feb 2012 at 6:27

[GoogleCodeExporter]

HandlerThrewEvent?

Original comment by wasserman.louis on 1 Feb 2012 at 6:55

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Louis is right that the event class name should reflect the context, not the 
contents.  For example, DeadEvent is not called EventEvent. :-)  (In fact, this 
is a good general design tip for EventBus-based systems.)

I'd suggest HandlerMethodThrew.  It's long, but not by Java standards. :-)  The 
term "handler method" is the one used in the API reference for a method 
annotated with @Subscribe.  "Threw" is more correct than the obvious 
alternatives, like "failed," because it doesn't imply whether an operation was 
successful — that's up to the handler method's contract.  Finally, you'll 
note I left off "event;" it seems redundant.  DeadEvent is not named that 
because it is an event — it's because an event (other than it) is dead.

My concern about this is still the one I described above: it lets components 
see into each other's state.  Java throwables contain a lot of state, including 
a complete stack trace and (depending on the class) application-specific 
information.  EventBus currently tries hard not to leak information between 
subscribers (beyond things that can be derived through a side-channel anyway, 
like timing).  Allowing a subscriber to see other subscribers' throwables would 
violate this principle.

As a strawman of an alternative, what if callers could specify an exception 
handler for a specific listener at register-time?  Applications that truly want 
all their listeners to see everyone's throwables can just pass the same one to 
every call.  Applications that don't use this behavior would never provide a 
handler.  It also makes the recursion behavior application-controlled: crazy 
applications can register the handler as the handler for the handler.

The extreme version of this would actually introduce an observer interface for 
the exception handler, but that seems inconsistent.

Original comment by cbiffle@google.com on 6 Feb 2012 at 7:18

[GoogleCodeExporter]

I note that in a pinch, the exception handler you propose could be repurposed 
to more or less act the same as HandlerMethodThrew: you just pass a handler 
that posts the exception to the event bus.  I like it, and the increased API 
surface is minimal.

Original comment by wasserman.louis on 6 Feb 2012 at 7:23

[GoogleCodeExporter]

(Yes, I realize that DeadEvent is arguably a violation of the principle I 
describe above.  I'm thinking about how to fix that.)

Original comment by cbiffle@google.com on 6 Feb 2012 at 7:23

[GoogleCodeExporter]

Hrrrrrrm.  I'm not sure what interface for the exception handler is most 
appropriate.

When we're handling an exception, do we pass in the handler object that threw 
the exception on one of its methods?  Do we tell it which handler method threw 
the exception?  Do we pass the handler object as an Object, damaging some of 
the nice type safety guarantees we had?

I'm not sure this is a solved problem yet.

Original comment by wasserman.louis on 8 Feb 2012 at 9:15

[GoogleCodeExporter]

Just another input: I often use EventBus for stuff like CSP / 
threaded-programming, where i have one queue to input commands into the Thread. 
The reason i use EventBus there, is because it saves me from endless if 
(nstanceof) then constructs. I could refactor that to use a proper Interface, 
of course, but more often than not make the the code more complicated that it 
needs to be.

The reason i'm adding this is that in all my EventBus scenarios I have exactly 
one handler that's fired for an Event, which eliminates the "should other 
handlers be fired" as well problem. Dunno, if you can make something out of 
that.

Original comment by fabian.z...@gmail.com on 11 Feb 2012 at 10:32

[GoogleCodeExporter]

Fabian, it sounds like what you want is runtime overload resolution (aka 
pattern matching).  I agree that this is a missing feature in Java.  It's not 
really EventBus's target use case, though.

Louis: my opinion on all of your questions is "no."  I imagine the exception 
handler being just another handler that accepts HandlerMethodThrew; if nothing 
else, this greatly simplifies faking exceptions during testing.  We should 
start with as narrow an interface for HandlerMethodThrew as possible -- we can 
always add more properties later if users require them.  Because the exception 
handler is passed in per-register-call, users can always allocate new ones for 
each object if required (essentially partial application of the full many-input 
exception handler function).

Original comment by cbiffle@google.com on 11 Feb 2012 at 8:31

[GoogleCodeExporter]

Just to be clear: you're saying that if we wanted to tell the exception handler 
which method threw the exception, we'd add that data to HandlerMethodThrew?

I think I like this, I'm just trying to think if there's anything better.

Original comment by wasserman.louis on 12 Feb 2012 at 5:32

[GoogleCodeExporter]

Yes, precisely.  As for whether the exception handler is an EventBus handler 
(with @Subscribe), or a single-method interface that accepts the event, I 
haven't decided.

Original comment by cbiffle@google.com on 13 Feb 2012 at 4:28

[GoogleCodeExporter]

How do you do the logging, is there some way to get these into my 
default-logger, at least?

Original comment by fabian.z...@gmail.com on 15 Feb 2012 at 10:05

[GoogleCodeExporter]

EventBus wouldn't do the logging for you.  To get the exceptions into the 
logger of your choice, you'd provide your own exception handler that would 
forward them.

Original comment by cbiffle@google.com on 15 Feb 2012 at 2:31

[GoogleCodeExporter]

Sure, but how is it done now?

Original comment by fabian.z...@gmail.com on 15 Feb 2012 at 2:34

[GoogleCodeExporter]

http://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/comm
on/eventbus/EventBus.java#319

Original comment by cbiffle@google.com on 15 Feb 2012 at 2:38

[GoogleCodeExporter]

(So, answer: no, there's no way to change the default logging behavior at the 
moment, not unless you can do it using one of the j.u.logging wrappers.)

Original comment by wasserman.louis on 15 Feb 2012 at 3:38

[GoogleCodeExporter]

Are we sure we'll have worked out these details to our satisfaction by release 
12?

Original comment by wasserman.louis on 2 Mar 2012 at 7:08

[GoogleCodeExporter]

Rolling EventBus issues over to r13.

Original comment by cpov...@google.com on 23 Mar 2012 at 9:27

• Added labels: Milestone-Release13
• Removed labels: Milestone-Release12

[GoogleCodeExporter]

FYI, cbiffle, Emily Soldal's "return a Ticket when a listener is registered" 
suggestion from issue 784 also addresses your suggestion of "register exception 
handlers on a per-listener basis."

Original comment by wasserman.louis on 2 May 2012 at 4:57

[GoogleCodeExporter]

Original comment by kevinb@google.com on 30 May 2012 at 7:41

• Added labels: Type-Enhancement-Temp
• Removed labels: Type-Enhancement

[GoogleCodeExporter]

Original comment by kevinb@google.com on 30 May 2012 at 7:45

• Added labels: Type-Enhancement
• Removed labels: Type-Enhancement-Temp

[GoogleCodeExporter]

Deleting the milestone from this and marking as Research like the rest of 
`EventBus`, since I don't think this'll make it out in 13.  (I'm going to start 
putting in some research on the `EventBus` issues shortly, though.)

Original comment by wasserman.louis on 28 Jun 2012 at 9:49

• Changed state: Research
• Removed labels: Milestone-Release13

[GoogleCodeExporter]

At the very least, please make EventHandler "public and final" so that we can 
subclass Eventbus and override the "dispatch(Object event, EventHandler 
wrapper)" method. We can write our own code try-catch block and call 
"wrapper.handleEvent(event)" instead of just logging exceptions. 

Right now EventHandler is package-private and we cannot even override the 
protected method. Seems weird that the method is protected but the parameter is 
package-protected.

Original comment by ashwin.j...@gmail.com on 1 Jul 2012 at 6:50

[GoogleCodeExporter]

Oh I see that in R13 the "EventBus.dispatch" method is package-private 
(http://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/com
mon/eventbus/EventBus.java#300)

Original comment by ashwin.j...@gmail.com on 2 Jul 2012 at 5:34

[GoogleCodeExporter]

Original comment by wasserman.louis on 5 Sep 2012 at 2:07

• Changed title: Provide custom exception handling in EventBus

[GoogleCodeExporter]

Issue 1132 has been merged into this issue.

Original comment by wasserman.louis on 5 Sep 2012 at 2:08

[GoogleCodeExporter]

We plan to use the event bus in production, for which we need to have some 
clarity on the exception handling mechanism. Any timelines by which this could 
be decided ?

Original comment by sudarsha...@gmail.com on 7 Sep 2012 at 8:19

[GoogleCodeExporter]

@sudarshan If you can't wait just copy the code from the eventbus package and 
put it into your own package. I had to do this because of JULI.

Then you just have to change EventBus.java to throw your own custom 
RuntimeException (or use Throwables.propagate() if you don't care what the 
wrapped exception is).

  protected void dispatch(Object event, EventHandler wrapper) {
    try {
      wrapper.handleEvent(event);
    } catch (InvocationTargetException e) {
      // My logger
      logger.error(fatal, "Could not dispatch event: " + event + " to handler " + wrapper, e);
      // My own custom runtime exception
      throw new EventBusException(e);
    }
  }

Original comment by adam.g...@evocatus.com on 7 Sep 2012 at 8:35