Removing a ConnectionEventListener from a PooledConnection during its connectionClosed() callback causes other ConnectionEventListener callbacks to be missed

• Key: DERBY-3401
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Knut Anders Hatlen
• Reporter: Daniel John Debrunner
• Component/s: JDBC
• Affects Version/s: 10.4.1.3
• Fix Version/s: 10.4.2.0, 10.5.1.1

Description

A ConnectionEventListener should be able to remove itself as a listener during a callback without affecting any other callbacks.

DataSourceTest.subtestPooledRemoveListenerOnClose() tests the scenario but calls to it will be commented out in the fixture testPooledReuseOnClose() (using this bug number).

Issue is that such a remove will modify the eventListener Vector in EmbedPooledConnection while it is being enumerated over.
An idea for a fix would be to first change the Vector over to a new-style collection, such as an implementation of List, then work off a copy of the collection when calling the callbacks. I don't think eventListener needs to be a synchronized collection, its access should be already synchronized on EmbedPooledConnection.

I imagine that a similar issue exists for adding a new callback during callback processing, fixing this bug would fix that issue as well though no tests have been written for the add case.

Comments

[Knut Anders Hatlen]

I think this is also a problem for the statement event listeners (JDBC 4.0 feature).

[Knut Anders Hatlen]

I agree that copying the collection sounds like a reasonable approach. Strictly speaking, we only need a copy if there are at least two listeners in the list, but it is perhaps not worth the extra complexity to avoid copying 1-element lists. I'll try it and see how complex it gets.

Alternatively, we could have a copy-on-write scheme so that we only pay the extra price if we actually modify the list of listeners (this assumes that the listeners are triggered more frequently than they are modified). For the statement event listeners, we could get this more or less for free with java.util.concurrent.CopyOnWriteArrayList (new in Java 1.5) since that code is compiled with the 1.6 compiler.

[Knut Anders Hatlen]

Here's a test and a fix for the statement event listeners. In the current trunk, adding or removing listeners from a listener causes ConcurrentModificationException (both client and embedded). Since this was JDBC 4.0 code, I went for the simple solution and just replaced Vector/ArrayList with CopyOnWriteArrayList and removed the synchronization.

All the regression tests passed.

[Kristian Waagan]

I had a look at ' d3401-statement_events.diff'. I think it looks good, but I have one question.

Any reason why you allow null objects into the list of listeners in ClientPooledConnection40 and ClientXAConnection40?
Can't you get an NPE when you iterate through the list of listeners and invoke the callback method?

I ran the new test(s) successfully, but modifying it to add a null listener causes failures.

+1 to commit if you plan to address the NPE elsewhere or in a followup patch.

[Knut Anders Hatlen]

Thanks for looking at the patch Kristian. Since the different handling of null in client versus embedded is an existing issue, I filed a new bug (DERBY-3695) that I plan to address separately. You're quite right that it causes a NullPointerException on the client.

[Knut Anders Hatlen]

Committed d3401-statement_events.diff with revision 660959.

[Knut Anders Hatlen]

Attaching a patch which fixes the problem with the ConnectionEventListeners, enables the test case for removing a listener and adds a test case for adding a listener.

The fix makes the PC update a variable each time it starts iterating over the list of listeners and each time it has finished iterating. That way, (add/remove)ConnectionEventListener knows if the list is currently being iterated over. If it is being iterated over, the list is cloned and the add/remove operation is performed on the clone. That way, the iterator is not affected and continues to work just as if no listener has been added or removed.

The logic is somewhat more complex than the code for the statement event listeners, as we don't have CopyOnWriteArrayList available in Java 1.4. However, it is less costly, as it only copies the list in the cases where modifying the list directly would have caused problems (CopyOnWriteArrayList always copies the list when it's modified).

All the regression tests passed with this patch.

[Kristian Waagan]

I looked at the patch 'd3401-connection_events.diff' and it looks good to me.
suites.All (Sun JDK 1.5) and derbyall (Sun JDK 1.6) passed without failures.

+1 to commit

[Knut Anders Hatlen]

Thanks for reviewing the patch, Kristian.
Committed revision 663641.

I'll probably merge it to 10.4 in a couple of days, so I'm leaving the issue open for now.

[Knut Anders Hatlen]

Merged to 10.4 and committed revision 664655.