[
https://issues.apache.org/jira/browse/POOL-108?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12547740
]
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.
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/[EMAIL PROTECTED]/msg68616.html
> GenericObjectPool does per-resource work (e.g. validate) in a synchronized
> context
> ----------------------------------------------------------------------------------
>
> Key: POOL-108
> URL: https://issues.apache.org/jira/browse/POOL-108
> 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.
