commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phil Steitz (JIRA)" <>
Subject [jira] Commented: (POOL-108) GenericObjectPool does per-resource work (e.g. validate) in a synchronized context
Date Mon, 03 Dec 2007 06:43:43 GMT


Phil Steitz commented on POOL-108:

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.


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.


> GenericObjectPool does per-resource work (e.g. validate) in a synchronized context
> ----------------------------------------------------------------------------------
>                 Key: POOL-108
>                 URL:
>             Project: Commons Pool
>          Issue Type: Improvement
>    Affects Versions: 1.3
>            Reporter: Matthew Moore
>             Fix For: 1.4
>         Attachments: GOP.patch
> 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.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message