Use ListOrderedSet for thread context listeners.

[doychin]

• Guarantees only one instance per listener in the collection
• Guarantees predictable execution order
• Added commons-collections4 as dependency to openejb-core. It was already there as transitive dependency from openjpa.

Signed-off-by: Doychin Bondzhev doychin@dsoft-bg.com

[SvetlinZarev]

Hi,

Why not CopyOnWriteArraySet from the standard java library ? Same features, but without the additional dependency (ok, the contains() operations is O(n), but it's not used in ThreadContext anyway).

An issue is that now all ThreadContextListeners must properly implement hashcode() and equals(), otherwise that change cannot guarantee that there will be only one instance per listener. But a quick search through the hierarchy reveals that none of the implementations override hashcode/equals. So from my PoV the change as is does not do anything.

Kind regards,
Svetlin

[doychin]

Few months a go it was CopyOnWriteArrayList. Then I found that using list for this purpose does not work because there is no way to protect that list from ending with multiple instances of the same listener or some listeners were never removed from the collection even when they are not needed anymore.

This happens in some test cases where tests are run one after another in the same VM and test start depending on the order of execution - the presence of a listener breaks a test.

So this structure must be kept in a state that is predictable. That's why I changed it to Set using CopyOnWriteArraySet.

Few days a go @dblevins reverted my change because for some odd reason there is some CXF code that somehow depends on the order of execution of the listeners in this collection. Set's can't guarantee predictable order of execution.

So we need a solution that guarantees both - same order of execution based on the order of add/remove from the collection and no more then one instance of a listener in the collection.

In this case none of the listeners implements equals and hash code. I made sure that those listeners that are never removed from the list are initialized only once so same instance is "added" every time.

The rest are removed during undeploy/stop of the container.

I could use ListOrderedSet from collections 3 but it is not generic so I added v4 of collections to the mix which is already used by openjpa so it is not new dependency.

[SvetlinZarev]

My point was that ListOrderedSet is the same thing as CopyOnWriteArraySet:

• Both implement Set
• Yet both preserve the insertion order

But ListOrderedSet:

• is an additional dependency
• uses twice the memory compared to CopyOnWriteArraySet
• generates twice the garbage when it's modified

In the end, functionality wise, it does not matter which of them is being used, as they do the same thing, but CopyOnWriteArraySet is the superior in terms of resource usage and not being an external dependency.

I admit that I didn't check the previous version, but if CopyOnWriteArraySet is not ok, then ListOrderedSet should not be OK as well.

My guess is that David did not notice that although CopyOnWriteArraySet implements Set, it's actually a CopyOnWriteArrayList underneath and behaves in the very same way, except that it does not accept duplicates. That's because "Set listeners" from the original change-set is misleading.Using Set as the type, you implicitly add documentation to the field that the order is not important, because sets do not have an order. So IMO using the actual type instead of the interface would be better in that case, as it makes it clear that the order of the elements is important.

[doychin]

Yes, you are right. I missed that part that CopyOnWriteArraySet is not real Set. It preserves the iteration order.