commons-lang
commons-lang copied to clipboard
apache
EventListenerSupport - Allow firing events "quietly"
The current EventListenerSupport class by default fires events in a fail-fast manner. When a listener throws an exception it prevents any further listeners from being called and propagates an InvocationTargetException or UndeclaredThrowableException[^1] out to the caller.
While that logic is appropriate for some applications, there is sometimes a desire to prevent listeners from interfering with one another, particularly in a more decoupled project.
This change allows the caller to use the alternative fireQuietly() method to retrieve the proxy. This proxy catches and hides all InvocationTargetExceptions from the java.lang.reflect.Method.invoke(...) method, which effectively silences exceptions from listener invocations, guaranteeing that all listeners receive the event.
See test below for basic operation:
@Test
public void testQuietInvocationHandling() throws Throwable {
final EventListenerSupport<ExceptionThrowingListener> listenerSupport = EventListenerSupport.create(ExceptionThrowingListener.class);
listenerSupport.addListener(new ExceptionThrowingListener() {
public void declaredError() throws Error {
throw new Error();
}
public void declaredRuntime() throws RuntimeException {
throw new RuntimeException();
}
public void declaredThrowable() throws Throwable {
throw new Throwable();
}
public void declaredIo() throws IOException {
throw new IOException();
}
public void declaredException() throws Exception {
throw new Exception();
}
public void undeclaredRuntime() {
throw new RuntimeException();
}
public void undeclaredNotImplemented() {
throw new NotImplementedException();
}
});
listenerSupport.fireQuietly().declaredError();
listenerSupport.fireQuietly().declaredRuntime();
listenerSupport.fireQuietly().declaredThrowable();
listenerSupport.fireQuietly().declaredIo();
listenerSupport.fireQuietly().declaredException();
listenerSupport.fireQuietly().undeclaredRuntime();
listenerSupport.fireQuietly().undeclaredNotImplemented();
assertThrows(UndeclaredThrowableException.class, () -> listenerSupport.fire().declaredError());
assertThrows(UndeclaredThrowableException.class, () -> listenerSupport.fire().declaredRuntime());
assertThrows(InvocationTargetException.class, () -> listenerSupport.fire().declaredThrowable());
assertThrows(UndeclaredThrowableException.class, () -> listenerSupport.fire().declaredIo());
assertThrows(InvocationTargetException.class, () -> listenerSupport.fire().declaredException());
assertThrows(UndeclaredThrowableException.class, () -> listenerSupport.fire().undeclaredRuntime());
assertThrows(UndeclaredThrowableException.class, () -> listenerSupport.fire().undeclaredNotImplemented());
}
Other considerations...
Technically it would be possible to implement the exception handling logic in different ways:
- The above proposal, which hides all exceptions from the caller.
- Collect all exceptions from the listeners and return those somehow to the caller.
- Throw the first encountered exception, after all listeners have received the event
All of those have downsides, but IMO the current code offers very little in the way of exception handling to begin with - the caller doesnt know which listener produced the exception, and must also be prepared to handle the unusual exception wrapping logic. So the tradeoff is pretty break even. Additionally, 2-3 aren't really "quiet" so if those are offered then might need a new term e.g. fireCompletely or fireAll.
[^1]: I'm not exactly sure why the Proxy wraps an UndeclaredThrowableException around the InvocationTargetException for some exceptions even when they are declared. The docs for java.lang.reflect.Proxy seem to indicate that it wraps only checked exceptions with UndeclaredThrowableException, however in the above code you can see that some checked exceptions are wrapped, even if they are declared, some are not, and unchecked exceptions appear to always be wrapped. This oddness is not specific to this change, and exists in the current implementation, so it's not something that should affect this change.
Hello @theshoeshiner ! At a high level I am ok with the feature but the implementation seems seems potentially wasteful because two proxies are always created.
My thinking is that the use case should be that I know in advance if I want a fail-fast listener support or not. The current implementation provides both fail-fast and fire-all (I don't like the "quietly" name) and the call site decides which one to call and that's what I am questioning. Is that level of flexibility required? If not, then the implementation should not create two proxies.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
02eac5e) 92.11% compared to head (0a53897) 92.13%. Report is 3 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #1156 +/- ##
============================================
+ Coverage 92.11% 92.13% +0.01%
- Complexity 7586 7589 +3
============================================
Files 200 200
Lines 15825 15836 +11
Branches 2890 2891 +1
============================================
+ Hits 14577 14590 +13
+ Misses 674 673 -1
+ Partials 574 573 -1
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@theshoeshiner Always run 'mvn' by itself to catch these build errors before you push.
Hello @theshoeshiner ! At a high level I am ok with the feature but the implementation seems seems potentially wasteful because two proxies are always created.
My thinking is that the use case should be that I know in advance if I want a fail-fast listener support or not. The current implementation provides both fail-fast and fire-all (I don't like the "quietly" name) and the call site decides which one to call and that's what I am questioning. Is that level of flexibility required? If not, then the implementation should not create two proxies.
I agree. That was an unfortunate consequence of limitations of the InvocationHandler API, which doesnt have an easy way to pass extra context to the invoke method.
In my scenario, I do use both the fail-fast and fire-all methods. If we went the single proxy route, this could still be accomplished by having two separate EventListenerSupport instances, but it would be nice if it was possible to do both with a single instance. If we did limit it to a single proxy then ultimately for my scenario I would probably have to just extend the EventListenerSupport class to one containing two proxies (which is kind of what I already do). But that wouldn't help anyone who was already using EventListenerSupport, as they would have to change to use the extended class to get both fire/fireall functions.
@theshoeshiner Thank you for your detailed reply. To be clear, let me ask: Your use case requires the same instance to support both fire-all and fire-fail-fast?
@theshoeshiner The build is still broken. Run 'mvn' by itself before you push.
@garydgregory
Yes essentially. If the same instance could not do both I would have to refactor some stuff.
@theshoeshiner The build is still broken. Run 'mvn' by itself before you push.
My bad. Local build was skipping PMD plugin.
@garydgregory Looks like the issue is that checkstyle allows empty catch blocks, but PMD plugin does not. What's the preferred way to handle an empty catch block? Normally I would log something, but I dont see any logging in commons-lang. The catch block in question is used to catch InvocationTargetExceptions from listener invoke() calls.
Edit: If I go the route of collecting the exceptions this won't be an issue.
I'm not sure how this is all supposed to work, but in general I get very nervous about anything that silently catches and swallows random exceptions. Dispatching to all and collecting their resulting exceptions to bundle and rethrow after every listener has had a chance seems like a better option.
I do like the idea of bundling the exceptions, and this pattern is used in org.apache.commons.io.input.ObservableInputStream, so I could use that as a guide. However It still feels like there should be an optional path for firing events and not being forced to catch exceptions. It would be nice if they could be returned instead of thrown, but Im not sure that's possible given that the proxy must honor the listener interface - will look into it though.
Ultimately if they were bundled, they would probably need to be packaged into some unchecked exception class - e.g. EventListenerExceptionList, and I suppose if my specific use case requires swallowing exceptions I could maybe write my own extension that swallows + logs that.
Will refactor my PR taking this into account see what I can come up with.
You're asking for a different language than Java, or a world where exceptions just don't occur. Everything in Java assumes exceptional conditions are handled by the usual exception mechanisms. Exceptions should not be returned instead of thrown. Exceptions should not be swallowed and ignored.
It's better not to do this than provide an implementation that swallows exceptions. It's better not to do this than provide an implementation that blindly converts checked exceptions to runtime exceptions.
That makes sense. If my use case requires me to log the exception and continue, then that extended implementation need only exist in my code, rather than in commons-lang. However I should point out the current implementation doesn't handle checked or unchecked exceptions in a reasonable manner, due to the weirdness of the Proxy class, nor does it allow for all listeners to receive the event in the case of a checked exception. If those two goals are reasonable and generic enough then I'd like to try and implement them here.
You can also "erase" exceptions. See org.apache.commons.io.function.Erase:
// no throws in signature
foo() {
try {
// Something that throws IOException
} catch (final IOException ex) {
rethrow(ex); // throws IOException
}
}
@SuppressWarnings("unchecked")
public static <T extends Throwable> RuntimeException rethrow(final Throwable throwable) throws T {
throw (T) throwable;
}
You can also "erase" exceptions. See org.apache.commons.io.function.Erase:
Thanks! That may be something I need once I get into the details.
(Just for fun) A special and interesting example of APIs that return exceptions by design is JUnit's assertThrows() family of methods.
yes, I suppose assertThrows is a special case. Cue George Carlin voice...Although, now that I think about it,
I really don't like assertThrows. The JUnit 4 approach try { doSomethingThatThrows(); fail();} catch (Exception ex) (assert on the exception)
was much cleaner and more obvious. I might think differently if Java's lambda syntax was more like Python and less like line noise but that's not the world we live in.
In JUnit's defense, you could argue that assertThrows does not, and should not, throw the underlying exception because it is not "exceptional" - In that the presence of the exception is not something that the caller should be forced to handle in an exceptional manner, in fact just the opposite. The lack of the exception is "exceptional". Of course, still doesn't change the fact that it is indeed returning an Exception object, but you could drop that return and the function of the method would still be the same.
So on the topic at hand - here's a question..
Say we want to ensure all listeners get the event, which I think is a reasonable feature, many other observable APIs have it (including apache commons). We begin by iterating through all listeners and collecting any checked exceptions.
The interface event method signature is: myEvent() throws CheckedException1, CheckedException2
Listener1 throws CheckedException1, Listener2 throws CheckedException2
So, what kind of exception should the proxy invocation handler throw?
The code im using as a bit of a guide, ObservableInputStream, doesn't have to deal with this problem. It only has to deal with IOException, and the IOExceptionList simply extends IOException, which elegantly bundles the underlying instances.
How do we force the caller to handle two different checked exceptions? We could wrap it in an EventHandlerExceptionList and throw that, but that violates the goal of not wrapping the checked exceptions in runtime exceptions. Should we force the caller to provide some kind of "onError" handler?
Reserving the right to change my mind later, but I think that if you want to deliver the message to all listeners, even when the an earlier listener throws an exception, I'd want a custom exception that bundles up all the listener exceptions.
If this is synchronous, I think the dispatcher should probably be ready to handle any error condition that can arise from calling a listener. So maybe it shouldn't be a generic utility at all. The dispatcher should not be in commons lang. Rather it should be in the client project that knows what the listeners are doing, knows what errors can happen and why, and knows how to handle them.
If it's asynchronous, then that's what ExecutorService/Executor/Executors is for, and we shouldn't reinvent that.
, I think the dispatcher should probably be ready to handle any error condition that can arise from calling a listener.
I agree, but I think my intention was that the new fireAll method is the dispatcher handling the exceptions. Which is to say - it doesnt want its context to be interrupted by the exceptions. Choosing to call that method is a specific decision. It's not the default functionality.
What if we allowed an error handler to be added to the EventListenerSupport instance? With the intention being that the fireAll method redirects all exceptions to the handler. The redirection would still be synchronous, but it would prevent the dispatcher from having to handle the exceptions in the same context as the fireAll() call. All exceptions are still "handled", but the dispatcher gets to fire events without a bunch of repetitive and potentially useless catch blocks.
@theshoeshiner Note that the build is still red. Run mvn locally (by itself) before pushing to catch all errors.
An error handler is an interesting idea. My initial reaction is positive, though I need to cogitate on it for a little while. It should be possible to give callers the option of either handling each exception independently or aborting processing completely. Either the error handler would be mandatory non-null or exceptions would bubble up naturally if no error handler was installed.
This still makes more salient the question of whether the events are fired in a given and known order. I suspect the answer is yes, but we should be very explicit about that, and both test and document that. This is important for reproducibility.
Slowly reading through the code I see:
- This is mainly designed for methods that don't throw exceptions like ActionListener
- The order is deterministic and tested though not documented.
Given that I think it's OK and probably a good idea for the fire method to fail fast.
I agree It makes sense for that to be the default behavior. But do you still see room for an optional error handler field, that when specified allows the fire method to redirect all proxied exceptions? Or even a separate fire() method that accepts the handler as a parameter? fireWithHandler()?
Now that I think about it, providing a handler might allow the event provider to also handle return values from listeners, In addition to exceptions. Not sure of a use case for that.
The more I look at this class the less I like it and the more I think we should just ignore it, maybe even deprecate it. First of all, as far as I can tell, despite the name it has nothing to do with event listeners. It simply allows one to sequentially invoke the same method on each a list of objects of the same type. There are other ways to do that, including lambdas and executors. Executors use futures to manage exceptions. Lambdas pretend they don't happen (and are broken by design as a consequence).
Do we really need a third way to address this problem? Is there any use case here that can't be handled by a lambda or a single threaded executor?
Digging through the history, this class dates to 2010, well before lambdas but well post Executors. Unless someone can demonstrate a compelling need in an actual, non-hypothetical project for this functionality, I vote no. The costs of additional public API we'll have to maintain going forward outweigh the benefits.
I would say that sequentially invoking the same method on a set of objects is essentially, or exactly, what most listener APIs do (internally that is), and they rarely use executors, IME. The real difference here is that most providers hard code each listener interaction, despite the fact that it produces redundant code (admittedly reducing redundancy isnt always a worthwhile goal). e.g. there are many methods within org.apache.commons.io.monitor.FileAlterationObserver that are identical except for the method that they call on the listeners. I think this EventListenerSupport class is trying to make that less redundant by encapsulating the iteration and reducing the number of internal methods. As expected, the FileAlterationObserver does not attempt to handle listener exceptions, and populates them back out to the provider, so that matches what this class currently does.
The more I think about it, the more I wonder why the default behavior should ever be to allow the listener to abort the providers process? I suppose it very much depends on the use case. I think both use cases have their place, and I feel pretty certain if I looked around I could find instances of both. A "fireWithHandler" implementation just allows the provider to exercise the second option, without repetitive try/catch blocks. Same principle as reducing the redundant iteration, just expanded to include exception handling.
As far as usefulness, I've used it many times in my current project because as far as I can tell it reliably replaces the pattern, while making it easier to evolve and test the listener API. e.g. because you call the proxy, you dont really have a need for an internal private method to wrap the iteration of the listeners. My specific use case is basically allowing UI components to listen directly to background jobs, and push updates to the user. The UI logic is naturally more difficult to test, and is thus more error prone, but even if it weren't we'd never want UI listeners to cause a critical background process to fail. That entire implementation could probably be replaced by a more loosely coupled publisher API, but for now it's implemented with direct listeners because the updates are instantaneous.
The code reduction and my specific use case certainly might not fall under the category of "compelling", so it may not meet the standard to be included here. I'm good if that's the verdict.
Do we have any estimate of how much this class is used? If there are a lot fo projects that depend on it, that would prove me wrong about its utility. Though given your description and thinking about exceptions in particular, I'm more worried than ever about this class.
The typical event listener/Observer pattern is to handle events as they occur, one at a time. At least that's the abstraction that's surfaced through the API. Each exception will be handled before the next event is processed, or it will shut down the thread if it isn't handled. The pattern is not batch up a bunch of tasks, handle them all, and then deal with errors. In synchronous event listeners, error handling or process termination as a result of one event happens before the next event is handled.
Of course an event can spawn a thread or an asynchronous task, but that is not what EventListenerSupport is doing. EventListenerSupport is synchronous. Events are handled in order. A failure to handle one event can have implications for how or whether to handle the next event.
Conceptually there are two cases here:
- An expected, possible exception, usually checked
- An unexpected exception, usually unchecked.
In the second case, the event listener absolutely should shut down, as should everything else. In the first case the exception needs to be handled. It's just a question of where and when.
What an error listener might do is give devs the option to defer or relocate the normal error handling. It should also give devs the option to continue processing the other events if they're independent, or to abort processing if they're not.
OTOH suppose the events are very dependent — e.g. every event writes a record in a database and the credentials are wrong. I might want one try-catch to wrap and handle SQL exceptions from all invocations. In that case, maybe just don't install an error handler? That is, error handlers are for cases where events are independent, and normal processing for cases where they are dependent.
Conceptually there are two cases here:
An expected, possible exception, usually checked An unexpected exception, usually unchecked. In the second case, the event listener absolutely should shut down, as should everything else. In the first case the exception needs to be handled. It's just a question of where and when.
What an error listener might do is give devs the option to defer or relocate the normal error handling. It should also give devs the option to continue processing the other events if they're independent, or to abort processing if they're not. OTOH suppose the events are very dependent — e.g. every event writes a record in a database and the credentials are wrong. I might want one try-catch to wrap and handle SQL exceptions from all invocations. In that case, maybe just don't install an error handler? That is, error handlers are for cases where events are independent, and normal processing for cases where they are dependent.
Mostly agree. Though given that the main goal of the error listener would be to defer / relocate error handling as opposed to hiding it, is there really any reason to not allow providers the option to also separately handle unchecked exceptions? Setting the error handler (maybe a separate handler for unchecked?) is not much different than a standard catch block for unchecked exceptions, because at that point its up to the dev to make sure it's handled properly. Allowing two separate handlers doesn't seem like too much extra complication given that checked and unchecked exceptions are often handled by separate logic to begin with.
So basically, all exceptions are initially caught by the invoker, with the caveat that they may be re-thrown if there are no handlers, causing the firing to stop.
This implementation has the benefit that it would not affect any current users of this class, since I believe the absence of handlers should result in the class functioning exactly as it does now. Additionally there would still only be one fire() method, so that any current users that want to take advantage simply need to set the handler.
I like "not affect any current users of this class, since I believe the absence of handlers should result in the class functioning exactly as it does now" That makes a lot of sense.
Note that I was talking about unexpected exceptions, not unchecked exceptions. Not quite the same thing. An error handler can manage a runtime exception like any other.
I still do wonder whether we need EventListener at all, now that we have lambdas. FileAlterationObserver does use lambdas these days, for instance. In some ways lambdas aren't as nice as EventListener but they are in the JDK and are much more likely to be familiar to devs. To the extent EventListener is less useful in a post-lambda world, I'm hesitant to add further code, API surface, and maintenance burden to the class.
I think even with the use of lambdas the util class offers the convenience of not having to manage all the listener specifics in regards to error handling and order (Unless im missing some potential bit of code optimization)? That management is still incredibly repetitive if you have many event types.
The FileAlterationObserver uses lambdas for iteration, but I think consumers still cant add listeners via lambdas because the listener interface has multiple methods. I do think lambdas could be taken advantage of if this feature was reimplemented to support only single argument events. So your fire would look like...
listenerutils.fire(new MyEventType(params...))
and your consumers could do stuff like...
provider.listenerutils.addListener(MyEventType.class, event -> {
...
});
provider.listenerutils.addListener(MyEventType.class,this::handleMyEvent);
Basically just replacing the pattern of maintaining a listener interface with maintaining event type classes.
(That's all off the top of my head so I could be wrong about the possibilities)
That would eliminate the proxying, but the util class would still handle the ordering and error handling, much like it does (or doesnt) today. The new implementation uses more up-to-date language features, and It has some limitations of it's own, but in the end its just a different way of accomplishing mostly the same thing, which is just encapsulating your listener management logic. It would definitely be it's own class though, since its API would be completely different than this one.
I think the above logic might work better for my usage, allowing consumers to target specific events, but that still probably doesnt address the question of whether it's useful enough to belong in commons.