BinaryLogClient will always continue even if one of the EventListeners errors out on an event

[sacundim]

I'm attempting to use the library for a project at my job, and I'm running into a problem because the BinaryLogClient will continue dispatching events to every EventListener, even if one (or all) of the listeners throws an exception on a previous event.

I have a proposed refactoring in order to make failure handling more flexible. The core ideas (ignoring backward compatibility for a moment, and imagining an ideal world) are:

1. BinaryLogClient should only have one EventListener and one LifecycleListener.
2. When the one EventListener throws an exception, the BinaryLogClient should abort and disconnect immediately.
3. The multiple listener behavior in the current library should be factored out into a BroadcastEventListener (and a BroadcastLifecycleListener) that handles the registration/unregistration logic, dispatches to the registered listeners, and "swallows" the child listeners' exceptions if desired (the same behavior as the current code).

I have started a fork exploring these ideas (sacundim@8edbda1). In order not to break existing code that uses the library, I've modified the ideas described above in this way:

• BinaryLogClient should continue to behave exactly the same way as it does now.
• I've added a class AbstractBinaryLogClient and moved most of the original logic there.

I still have three issues I'd like to work through, though:

1. My changes so far still break compatibility with existing code, because BinaryLogClient.LifecycleListener has methods that take a BinaryLogClient argument. In my modified code as of now, this needs to be changed to AbstractBinaryLogClient, which means that existing LifecycleListener implementations will no longer compile.
2. I'm still generally just not certain that I have the best factoring.
3. I haven't run any tests yet.

I'd appreciate to have your input into this matter.

[sacundim]

I have made some improvements to my fork that I think are going to simplify things:

sacundim@f39aceb

The way I have things now:

1. Most of the logic that was originally in BinaryLogClient moves to AbstractBinaryLogClient.
2. Instead of invoking event and lifecycle listeners, AbstractBinaryLogClient just invokes protected abstract methods to allow subclasses to decide how to handle these.
3. BinaryLogClient subclasses AbstractBinaryLogClient to implement the same behavior as before.
4. As of now, I've left my BroadcastEventListener and BroadcastLifecycleListener refactoring into BinaryLogClient, but I'm not particularly attached to it now, and would be perfectly happy to restore this to how it was originally.

The idea is that it should be possible to write new subclasses of AbstractBinaryLogClient that, by throwing an Exception, cause the client to abort and disconnect.

My next steps:

1. Install vagrant, get the unit test suite running, and validate I haven't broken anything.
2. Do a pull request
3. Add tests to verify that the AbstractBinaryLogClient aborts gracefully if the onEvent method fails; modify the code if this is not so.
4. Do a pull request

[shyiko]

Howdy,

Can I ask why didn't you go with registering (just an example) FailureIntolerantDelegatingEventListener which calls disconnect() on the client when some event listener (among registered) throws an exception, like so:

Note that implementation below is not thread-safe (for brevity).

public class FailureIntolerantDelegatingEventListener 
    implements BinaryLogClient.EventListener {

    private final BinaryLogClient binaryLogClient;
    private final List<EventListener> eventListeners = 
new LinkedList<EventListener>();

    public FailureIntolerantDelegatingEventListener(
BinaryLogClient binaryLogClient) {
        this.binaryLogClient = binaryLogClient;
    }

    public void registerEventListener(EventListener eventListener) {
        eventListeners.add(eventListener);
    }

    @Override
    public void onEvent(Event event) {
        for (EventListener eventListener : eventListeners) {
            try {
                eventListener.onEvent(event);
            } catch (Exception e) {
                binaryLogClient.disconnect();
                break; // do not notify remaining event listeners
            }
        }
    }

}

// and then somewhere in the code:

...
BinaryLogClient binaryLogClient = ...
FailureIntolerantDelegatingEventListener delegatingEventListener = 
    new FailureIntolerantDelegatingEventListener(binaryLogClient);
delegatingEventListener.registerEventListener(<event listener>);    
delegatingEventListener.registerEventListener(<another event listener>);
...
binaryLogClient.registerEventListener(delegatingEventListener);
binaryLogClient.connect();
...

If, for some reason, you don't like the idea of EventListener being aware of BinaryLogClient and you really want BinaryLogClient itself to take care of any failures, then how about issue-15-proposed-change? It's basically few lines change which would allow to override ("in-place") notification strategy used by BinaryLogClient.

As for the "BinaryLogClient should only have one EventListener and one LifecycleListener", personally I'd rather not do that. Even if we set aside backward compatibility (as stable (API-wise) version is not in Maven Central yet) I would argue that it's too restrictive (=frustrating) (especially when you want to register (temporary) some tracing / monitoring listeners which do not care (or even know) about other registered ones (like in tests)). Moreover, no one is forcing to use binaryLogClient.registerEventListener more than once if you absolutely don't want to :).

Please let me know your thoughts on this.

[sacundim]

Can I ask why didn't you go with registering (just an example) FailureIntolerantDelegatingEventListener which calls disconnect() on the client when some event listener (among registered) throws an exception, like so [...]

A handful of reasons. First of all, well, I didn't think of it, for starters :-P.

Second: design-wise, I thought that BinaryLogClient perhaps has too many responsibilities right now, so I sought to separate two of them (communication with the server vs. managing the listeners).

Third: implicitly swallowing the EventListeners' exceptions strikes me as a biased, dangerous and complex default. By "biased," I mean that it's the behavior we want in some scenarios, like when you have disparate listeners sharing one client, and performing relatively safe actions in response to events (like invalidating in-memory caches). But in other cases, we really do want to fail fast.

For example, in my project I'm trying to record the history of changes to a table that is subject to frequent UPDATE statements. This means for each event, I have to:

1. Insert a row into another database recording whether the event's row data, whether it was an insert, update or delete, and the timestamp of the event;
2. Record the binlog filename and position into another table as the "high water mark";
3. Take care to handle transactions correctly, so that rows and high water marks are only committed on XID events.

If the event listener for example fails to insert into the target, it really makes no sense to continue. Even worse, if a target insert failure is later followed by a success, then I've put the target into an erroneous state. So "swallow by default" strikes me as a dangerous default, in that it makes it easier for careless people to implement listeners like this one incorrectly. (Note that I count myself among the careless.)

It's also more complex to reason about, because just like I can write an EventListener that disconnects the client on failure, somebody can write a LifecycleListener that reconnects the client on a disconnect. So, arguably, your FailureIntolerantDelegatingEventListener idea needs to be modified to unregister the listener in addition to disconnecting the client.

It's your code, however, so your call. I could certainly live with this proposal for now.

If, for some reason, you don't like the idea of EventListener being aware of BinaryLogClient and you really want BinaryLogClient itself to take care of any failures, [...]

I don't like it for this reason: such EventListeners are harder to unit test. For example I wrote an automated test yesterday for my project's EventListener by using the BinaryLogFileReader to feed it some binlog files I generated by running some scripts against a fresh MySQL install.

Though in this case thankfully the FailureIntolerantDelegatingEventListener delegates to a EventListener that can be tested on its own. So again, I could live with that proposal.

[...] then how about issue-15-proposed-change? It's basically few lines change which would allow to override ("in-place") notification strategy used by BinaryLogClient.

To my eye, a subclass that overrode notifyEventListeners to implement the behavior I propose would be violating its superclass' contract. For example, my subclass could simply not honor EventListener registrations.

But again, your code, your call.

As for the "BinaryLogClient should only have one EventListener and one LifecycleListener", personally I'd rather not do that.

That was my first idea, but I changed my mind pretty quickly. The patch for #16 ended up not having this.

Summary: there are three proposals on the table that AFAIK can all be made to work:

1. My pull request Refactor BinaryLogClient into an abstract base class to allow for more flexible use #16
2. No code change; use a FailureIntolerantDelegatingEventListener or similar
3. c025f1e

[shyiko]

implicitly swallowing the EventListeners' exceptions strikes me as a biased, dangerous and complex default

You're probably right. I also tend to agree on "too many responsibilities" part. Let me think it through.

[sacundim]

Well, in the meantime, I've implemented a version of your FailureIntolerantDelegatingEventListener in my project, but with one twist that I mentioned earlier—unsubscribe first, then disconnect. Looking good so far...