#115 BasicResourcePool is not fully thread safe

[Marty Rose]

For example:

LinkedList unused = new LinkedList();

is only ever manipulated within a synchronized block, however a LinkedList is not a concurrent data structure!

the node pointers inside the LinkedList are NOT volatile and so modifications made to this list are not necessarily visible across all threads/CPU's.

We have an environment where we have 50+ VM's running on non-NUMA servers (they never ever have an issue); but we have two servers setup that use cores on different CPU dies. We routinely see lost connections, occasionally infinite loops, etc in the VM's running on those servers. You would never see this on a VM running within a single L1/L2/L3/etc memory visibility.

I eventually traced this down to the following unsafe code in BasicResourcePool. I have patched a local version to use java.util.concurrent versions of these datastructures and have been running with no issues (or lost connections) for a few days now.

If interested I can send patches.

unused isn't the only data structure with this issue

it appears all the following have this issue as well. (I have not done an exhaustive search)

HashMap managed = new HashMap();

/* all valid, managed resources currently available for checkout */
LinkedList unused = new LinkedList();

/* resources which have been invalidated somehow, but which are */
/* still checked out and in use. */
HashSet excluded = new HashSet();

Map formerResources = new WeakHashMap();

Set idleCheckResources = new HashSet();

[Steve Waldman]

So thanks for what is an interesting issue and report.

My understanding is that it is a responsibility of a JVM, whenever a Thread enters or leaves a monitor, to ensure that reads and writes are consistent, regardless of any caching. That is, what one Thread did before leaving a monitor must be visible by another Thread which acquires it.

So when a Thread enters modifies a java.util.LinkedList, if that list is unprotected, it would be perfectly possible that an internal pointer change would not be seen by some other Thread (unless the internal pointer was marked volatile).

However, if the Thread must obtain a monitor before modifying that pointer, and if any other Thread must obtain the same monitor before reading it, there should be no possibility of a dirty read.

If this understanding were incorrect, than there would be concurrency problems all over c3p0. but i do think it is correct. See 17.4.4 "synchronization order" in the JLS http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html

Perhaps in your unusual environment, your JVM is not respecting the specified behavior. But my inclination is to call this "closed, not a bug", because a compliant Java implementation always would, and trying to write concurrent code without the invariant that changes across synchronization boundaries are consistent would be very hard.

Am I wrong?

[Steve Waldman]

closed, not a bug.