Guava: Provide custom exception handling in EventBus
_Original issue created by toellrich on 2011-10-20 at 12:05 PM_
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
gissuebot
All 61 comments
_Original comment posted by ray.j.greenwell on 2011-11-01 at 12:34 AM_
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)" ?
gissuebot
on 31 Oct 2014
_Original comment posted by toellrich on 2011-11-01 at 04:12 AM_
In our case they would all be transactional and therefore wouldn't need to be notified explicitly because of the rollback.
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2011-11-15 at 04:49 PM_
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?
gissuebot
on 31 Oct 2014
_Original comment posted by toellrich on 2011-11-15 at 08:24 PM_
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)
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2011-11-15 at 08:43 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by toellrich on 2011-11-15 at 08:57 PM_
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!
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2011-11-17 at 01:47 AM_
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."
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2011-11-21 at 10:51 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2011-11-21 at 10:56 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2011-12-05 at 11:25 PM_
_(No comment entered for this change.)_
Labels: Type-Enhancement
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2011-12-09 at 06:28 PM_
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:
- Distribute the events in arbitrary order, as we do today.
- Take note of whether any subscribers failed. Skip any failed subscribers, as we do today.
- 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?
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2011-12-10 at 04:22 PM_
_(No comment entered for this change.)_
Labels: Package-EventBus
gissuebot
on 31 Oct 2014
_Original comment posted by toellrich on 2011-12-11 at 01:32 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by cgdecker on 2011-12-11 at 02:08 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by toellrich on 2011-12-12 at 02:45 AM_
As I said, it's probably for the best if I stick to another event bus implementation.
gissuebot
on 31 Oct 2014
_Original comment posted by toellrich on 2011-12-16 at 09:20 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-01-25 at 09:43 PM_
_Issue #868 has been merged into this issue._
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-01-25 at 09:46 PM_
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?
gissuebot
on 31 Oct 2014
_Original comment posted by ewjmulder on 2012-01-26 at 08:28 AM_
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. :)"
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-01-27 at 01:34 AM_
Sorry, I didn't parse that correctly originally. It's totally accurate.
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-01-31 at 07:21 PM_
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.
Status: Accepted
Owner: [email protected]
Labels: Milestone-Release12
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-01-31 at 07:28 PM_
Query: why ExceptionEvent vs. ThrowableEvent? Or is it just more readable that way?
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-01 at 06:27 AM_
Just jotted that quickly without thinking.
Though, "ThrowableEvent" does kind of sound like the event itself can be thrown.
gissuebot
on 31 Oct 2014
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-06 at 07:18 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-02-06 at 07:23 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-06 at 07:23 PM_
(Yes, I realize that DeadEvent is arguably a violation of the principle I describe above. I'm thinking about how to fix that.)
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-02-08 at 09:15 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by fabian.zeindl on 2012-02-11 at 10:32 AM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-11 at 08:31 PM_
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).
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-02-12 at 05:32 PM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-13 at 04:28 AM_
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.
gissuebot
on 31 Oct 2014
_Original comment posted by fabian.zeindl on 2012-02-15 at 10:05 AM_
How do you do the logging, is there some way to get these into my default-logger, at least?
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-15 at 02:31 PM_
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.
gissuebot
on 31 Oct 2014
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-02-15 at 02:38 PM_
https://github.com/google/guava/blob/master/guava/src/com/google/common/eventbus/EventBus.java#L319
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-02-15 at 03:38 PM_
(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.)
gissuebot
on 31 Oct 2014
_Original comment posted by wasserman.louis on 2012-03-02 at 07:08 PM_
Are we sure we'll have worked out these details to our satisfaction by release 12?
gissuebot
on 31 Oct 2014
_Original comment posted by [email protected] on 2012-03-23 at 09:27 PM_
Rolling EventBus issues over to r13.
Labels: -Milestone-Release12, Milestone-Release13
gissuebot
on 1 Nov 2014
_Original comment posted by wasserman.louis on 2012-05-02 at 04:57 PM_
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."
gissuebot
on 1 Nov 2014
_Original comment posted by [email protected] on 2012-05-30 at 07:41 PM_
_(No comment entered for this change.)_
Labels: -Type-Enhancement, Type-Enhancement-Temp
gissuebot
on 1 Nov 2014
_Original comment posted by [email protected] on 2012-05-30 at 07:45 PM_
_(No comment entered for this change.)_
Labels: -Type-Enhancement-Temp, Type-Enhancement
gissuebot
on 1 Nov 2014
_Original comment posted by wasserman.louis on 2012-06-28 at 09:49 AM_
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.)
Status: Research
Labels: -Milestone-Release13
gissuebot
on 1 Nov 2014
_Original comment posted by ashwin.jayaprakash on 2012-07-01 at 06:50 PM_
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.
gissuebot
on 1 Nov 2014
_Original comment posted by ashwin.jayaprakash on 2012-07-02 at 05:34 PM_
Oh I see that in R13 the "EventBus.dispatch" method is package-private (https://github.com/google/guava/blob/master/guava/src/com/google/common/eventbus/EventBus.java#L300)
gissuebot
on 1 Nov 2014
_Original comment posted by wasserman.louis on 2012-09-05 at 02:07 PM_
_(No comment entered for this change.)_
gissuebot
on 1 Nov 2014
_Original comment posted by wasserman.louis on 2012-09-05 at 02:08 PM_
_Issue #1132 has been merged into this issue._
gissuebot
on 1 Nov 2014
_Original comment posted by sudarshan89 on 2012-09-07 at 08:19 PM_
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 ?
gissuebot
on 1 Nov 2014
_Original comment posted by [email protected] on 2012-09-07 at 08:35 PM_
@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);
}
}
gissuebot
on 1 Nov 2014
_Original comment posted by [email protected] on 2013-03-07 at 02:08 PM_
Do you guys have any update regarding this issue? Any ETAs?
gissuebot
on 1 Nov 2014
_Original comment posted by cgdecker on 2013-03-18 at 03:49 AM_
Sorry, no ETAs at this time, but I will try to get back to these EventBus issues and see what can be done when I get a chance.
gissuebot
on 1 Nov 2014
_Original comment posted by b.k.oxley on 2013-07-23 at 03:05 AM_
Something as simple as this would be appreciated:
public class XEventBus extends EventBus {
public static final class ExceptionEvent {
public final Object event;
public final InvocationTargetException exception;
public ExceptionEvent(final Object event, final InvocationTargetException exception) {
this.event = event;
this.exception = exception;
}
}
@Override
void dispatch(final Object event, final EventHandler wrapper) {
try {
wrapper.handleEvent(event);
} catch (final InvocationTargetException e) {
post(new ExceptionEvent(event, e));
}
}
}
gissuebot
on 1 Nov 2014
_Original comment posted by b.k.oxley on 2013-07-23 at 03:10 AM_
Left out a detail - need an initial dead event handler to avoid looping when there is not exception event handler. Could either be special cased in the ITE catch clause, or provide EB with a default dead event handler that similar to the current ITE catch clause.
gissuebot
on 1 Nov 2014
_Original comment posted by liska.jakub on 2013-10-18 at 07:12 AM_
It's not a problem to extend EventBus and make it throw exceptions. I did that a year ago. I remember that there were a few obstacles because the class was not designed for being extended, but it worked... There are disadvantages though...
gissuebot
on 1 Nov 2014
_Original comment posted by peter.prikryl on 2013-11-07 at 08:42 AM_
easy WORKAROUND to re-throw exception:
package com.google.common.eventbus;
import java.lang.reflect.InvocationTargetException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class RDSEventBus extends EventBus {
private static final Logger LOG = LoggerFactory.getLogger(RDSEventBus.class);
void dispatch(Object event, EventHandler wrapper) {
try {
wrapper.handleEvent(event);
} catch (InvocationTargetException e) {
LOG.error("Could not dispatch event: {} to handler {}", event, wrapper, e);
throw new IllegalStateException(e.getCause());
}
}
}
gissuebot
on 1 Nov 2014
_Original comment posted by [email protected] on 2013-11-07 at 06:59 PM_
In Guava 16.0, you'll be able to register a SubscriberExceptionHandler to provide custom handling for exceptions thrown by subscribers: https://github.com/google/guava/blob/master/guava/src/com/google/common/eventbus/EventBus.java#L190
Note that you _cannot_ use this handler to make post() throw an exception when a subscriber throws an exception... that still goes against the fundamental design of the event bus.
Other than that, you can do pretty much whatever you want. The SubscriberExceptionHandler is given all the information on the context of the exception. You can even use it to post an exception event of some type back to the EventBus if that's what you want (you'll want to make sure that your subscriber methods that handle the exception event don't throw exceptions themselves though).
Status: Fixed
Owner: [email protected]
Labels: Milestone-Release16
gissuebot
on 1 Nov 2014
_Original comment posted by [email protected] on 2013-11-07 at 07:01 PM_
@peter.prikryl: Note that by using package-private parts of the eventbus package, you're going to be broken in Guava 16.
gissuebot
on 1 Nov 2014
_Original comment posted by sebastian.davids on 2014-01-06 at 10:42 PM_
Please update the package documentation and the wiki with an usage example.
https://code.google.com/p/guava-libraries/wiki/EventBusExplained
https://google.github.io/guava/apidocs/com/google/common/eventbus/package-summary.html
gissuebot
on 1 Nov 2014
_Original comment posted by mehmetsen80 on 2014-08-16 at 07:01 PM_
Well, I have still same issues but in a different scenario
When I run in spring sts, there is no issue but when I run in the server with java -jar project.jar it gives the same SEVERE: Could not dispatch event: error
The below didn't work for me..
package edu.uams.event;
import java.awt.EventQueue;
import java.lang.reflect.InvocationTargetException;
import java.util.concurrent.Executor;
import org.apache.log4j.Logger;
import com.google.common.eventbus.AsyncEventBus;
import com.google.common.eventbus.EventHandler;
import com.google.common.eventbus.SubscriberExceptionHandler;
import edu.uams.domain.TirEvent;
import edu.uams.pacs.IncomingFileMonitor;
public class AysncTraumaEventBus extends AsyncEventBus {
private final static Logger logger = Logger.getLogger(AysncTraumaEventBus.class);
private String name = null;
public AysncTraumaEventBus(Executor executor,
SubscriberExceptionHandler subscriberExceptionHandler) {
super(executor, subscriberExceptionHandler);
logger.info("AysncTraumaEventBus created.");
}
public AysncTraumaEventBus(String name, Executor executor) {
super(name,executor);
this.name=name;
logger.info("AysncTraumaEventBus created. Name:"+this.name);
}
@Override
public void register(Object object) {
super.register(object);
}
@Override
public void unregister(Object object) {
super.unregister(object);
}
@Override
public void dispatch(Object event, EventHandler wrapper) {
try {
logger.info("Let's dispatch Aysnchroneous Trauma Event:"+ ((TirEvent) event).getResultMessage());
wrapper.handleEvent(event);
} catch (InvocationTargetException e) {
// My logger
logger.error("Could not dispatch event: " + event + " to handler " + wrapper+" e:"+e.getMessage());
logger.info("Lets try to disptach again!");
super.post(new ExceptionEvent(event, e));
}
}
public static final class ExceptionEvent {
public final Object event;
public final InvocationTargetException exception;
public ExceptionEvent(final Object event, final InvocationTargetException exception) {
this.event = event;
this.exception = exception;
}
}
}
Somehow the EventHandler can't invoke the target event..
wrapper.handleEvent(event);
When you look the wrapper (EventHandler):
public void handleEvent(Object event) throws InvocationTargetException {
checkNotNull(event);
try {
method.invoke(target, new Object[] { event });
} catch (IllegalArgumentException e) {
throw new Error("Method rejected target/argument: " + event, e);
} catch (IllegalAccessException e) {
throw new Error("Method became inaccessible: " + event, e);
} catch (InvocationTargetException e) {
if (e.getCause() instanceof Error) {
throw (Error) e.getCause();
}
throw e;
}
}
You see that method.invoke(target, new Object[] { event }); throws the InvocationTargetException from the Method.class
public Object invoke(Object obj, Object... args)
throws IllegalAccessException, IllegalArgumentException,
InvocationTargetException
{
if (!override) {
if (!Reflection.quickCheckMemberAccess(clazz, modifiers)) {
Class caller = Reflection.getCallerClass(1);
checkAccess(caller, clazz, obj, modifiers);
}
}
MethodAccessor ma = methodAccessor; // read volatile
if (ma == null) {
ma = acquireMethodAccessor();
}
return ma.invoke(obj, args);
}
Maybe it is related with Reflection thing.. I don't know, but EventBus just doesn't know what target to invoke..
If it doesn't know the target doesn't it mean that @Subscribe does not work as expected? So it means then it can't find the subscribed method?
gissuebot
on 1 Nov 2014
_Original comment posted by mehmetsen80 on 2014-08-16 at 07:48 PM_
What the interesting part is that the same jar file along with EventBus can run fine in STS Run but when I run java from commandline as java -jar project.jar it doesn't dispatch the event.. still confused
gissuebot
on 1 Nov 2014
Is there a reason an EventBus can't have both a custom identifier and a custom exception handler?
immibis
on 20 Jan 2015
Related issues
gissuebot
·
5Comments
Bocete
·
3Comments
terence1984
·
3Comments
oliviercailloux
·
4Comments
gissuebot
·
3Comments
Most helpful comment
Is there a reason an EventBus can't have both a custom identifier and a custom exception handler?