GenericObjectPool does per-resource work (e.g. validate) in a synchronized context

• Key: POOL-108
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Matthew Moore
• Affects Version/s: 1.3
• Fix Version/s: 1.4

Description

While using the pool library with DBCP, and load testing with simulated failures, we noticed that a single bad connection can cause multiple threads to block when the test on borrow flag is true. (Using "select 1" as a test query.)

Looking at the code, GenericObjectPool performs activation, passivation, and validate on both borrow and return, inside of synchronized methods. This can be a real problem, since any of these operations could conceivably block, in which case all threads trying to obtain or release a resource will also block for the duration. Some of these concerns are indirectly covered by POOL-93 in 2.0. Looking at revision 594226 I can see this has not yet been addressed.

Narrowing the synchronization scope via synchronized blocks, rather than synchronizing the entire method, would deal with this easily enough. If I get the time I'll work up a patch. This should be addressed - in the context of DBCP it turns test on return / borrow into counter-intuitively dangerous options.

Comments

[Matthew Moore]

Just wanted to add that GenericObjectPool.evict() suffers the same problem, in that the object-wide lock is held during potentially blocking validation operations when testWhileIdle is true.

[Phil Steitz]

Patch to remove excess synchronization in GenericObjectPool.

[Phil Steitz]

Changes and rationale for GOP.patch.

Pool 1.3 added synchronization to address threadsafety issues presented in [1]. The potential race conditions identified all involved threads invoking close() on the pool while other threads were accessing or depending on the pool's closed member or, more dangerously, the _factory. In the 1.2 implementation, close() set _factory to null, so unsynchronized access to _factory could lead to null pointer exceptions. In 1.3, close was modified to no longer set _factory to null and closed was declared volatile. Neither of these eliminates the need to protect the data, but the two together make the identified race conditions both less likely and less damaging. The patch essentially reverts synchronization to the 1.2 level.

Changes:

borrowObject - narrowed the synchronization to the wait loop, moving assertOpen inside the loop. Maintained synchronization of access to pool fields. Control driven by stack variables outside synchronized blocks does not depend on pool state. Access to _factory outside of synchronized context is technically unsafe, since there is a synchronized setFactory method. This method throws IllegalStateException, however, if invoked when there are active objects, so this is not a practial problem. close no longer modifies _factory.

invalidateObject - narrowed scope of synchronization to _numActive decrement and notify. Unsynchronized access to _factory is not a practical issue per comment above.

returnObject - removed synchronization entirely. Only _factory was being protected.

addObjectToPool - took factory methods out of synchronized scope. The decision on whether or not to add the returned object back to the pool is made inside the synchronized block and no change in pool state should cause incorrect behavior for the factory methods outside the block. Also added an isClosed check before adding objects back to the pool.

addObject - moved makeObject outside scope of synchronization and moved the assertOpen inside the synchronized block, catching (and rethrowing after cleanup) IllegalStateException, which could happen if the pool is closed while makeObject is in progress.

[1] http://www.mail-archive.com/[e-mail]/msg68616.html

[Mark Thomas]

I have attached a proposed patch to POOL-93 that includes the GOP fixes here, ports them to GKOP and also includes the sync fixes proposed in POOL-93.

[Phil Steitz]

Pacth (pool-93-markt-v2.patch) from POOL-93, which was applied in r602773, should also fix this issue.