Inconsistent synch in InvalidEvictorLender, GenericKeyedObjectPool, GenericObjectPool

• Key: POOL-113
• Type: Bug
• Status: Open
• Priority: Major
• Assignee: Unassigned
• Reporter: Sebb
• Fix Version/s: 2.0

Description

InvalidEvictorLender does not always synchronize referant field.

Patch to follow - please check that the correct synch object is used.

Similar problems apply to GenericKeyedObjectPool:

Inconsistent synchronization of org.apache.commons.pool.impl.GenericKeyedObjectPool._testOnBorrow; locked 66% of time
GenericKeyedObjectPool.java line 913
Inconsistent synchronization of org.apache.commons.pool.impl.GenericKeyedObjectPool._testOnReturn; locked 66% of time
GenericKeyedObjectPool.java line 1096

And to GenericObjectPool:

Inconsistent synchronization of org.apache.commons.pool.impl.GenericObjectPool._testOnBorrow; locked 66% of time
GenericObjectPool.java line 915
Inconsistent synchronization of org.apache.commons.pool.impl.GenericObjectPool._testOnReturn; locked 66% of time
GenericObjectPool.java line 1017

Comments

[Sebb]

The easiest solution for _testOnBorrow and _testOnReturn is probably to make these volatile, in which case the get/set routines no longer need to be synchronized.

[Phil Steitz]

Where did the report above come from? Can you please rerun whatever discovered this against trunk or the 1_4_RELEASE branch to see if the synchronization changes committed to address POOL-93, POOL-108 also fixed this for GenericObjectPool. If yes, we can change the fix version on this to 2.0.

[Sebb]

The report was generated by Findbugs. I've just re-run it against current trunk, and it shows the same errors (amongst others).

If the class is intended for use from multiple threads, then it is vital that all fields that can be updated in one thread and read in another are synchronised on the same object. The Findbugs report shows that some fields can be accessed without synchronisation. The Java memory model only guarantees that such accesses see the most recent value if the accesses to the field are all synch on the same object (or volatile is used). Synchronisation is necessary for visibility as well as mutual exclusion.

In fact in this case, the fields are mainly primitive booleans, so synch is not needed for exclusion when updating. However it is needed for visibility between threads.

[Phil Steitz]

Synchronization fixes committed for GenericObjectPool, GenericKeyedObjectPool, StackKeyedObjectPool in trunk r603451 and 1.4 release branch r603449.

The relevant classes in the 1.4 release branch have been patched address this issue. Leaving open for 2.0