commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephen Colebourne <scolebou...@btopenworld.com>
Subject Re: [pool] synchronization issues in Pool
Date Sun, 23 Oct 2005 13:56:31 GMT
This looks interesting. I'll leave the pool comments to a pool 
developer. However, could adding a lot more synchronoization could cause 
other issues with locking and performance?

My main question is can these tests be run against any class? We're 
particularly stuck with 
http://issues.apache.org/bugzilla/show_bug.cgi?id=32573 collections 
LRUMap at present where a bug can easily be reproduced when you don't 
sync, but not at all when you do sync. Yet we have bugs reported! So, is 
your tool an internal project for your PhD, or intended to be for 
general open source usage.

Stephen


Mayur Naik wrote:
> Hello,
> 
> I'm a PhD student in Computer Science at Stanford University,
> evaluating a static race detection tool I'm developing on open source
> Java programs.  I ran it on the 5 implementations of pools in Apache
> Commons Pool, and found some bugs.  The output of the tool is here:
> 
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/softref/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic_keyed/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack_keyed/index.html
> 
> I will explain one race in detail.  Following the [grouped by field]
> link on the first link above and then the [details] link under the
> heading "Races on field org.apache.commons.pool.BaseObjectPool:
> boolean closed" leads you to:
> 
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic/R8_trace.html
> 
> Traversing the [up] links on this page yields at least one pair of
> paths in the call graph along which a common lock is not held and
> hence a race can occur if that pair of paths is executed by different
> threads:
> 
> PATH 1:
> 
> GenericHarness:27
> org.apache.commons.pool.impl.GenericObjectPool:853
> org.apache.commons.pool.BaseObjectPool:77
> 
> leading to a read access of the field 'closed' at
> org.apache.commons.pool.BaseObjectPool:73
> 
> PATH 2:
> 
> GenericHarness:33
> org.apache.commons.pool.impl.GenericObjectPool:901
> 
> leading to a write access of the field 'closed' at
> org.apache.commons.pool.BaseObjectPool:62
> 
> [GenericHarness is a test harness that my tool automatically generates.]
> 
> The above race is due to missing synchronization in method
> returnObject of class GenericObjectPool.  In fact, it can cause a null
> pointer exception:
> 
> thread 1 calls the returnObject method which executes the assertOpen
> method and finds the pool to be open.
> 
> thread 2 calls the close method which sets _factory to null.
> 
> thread 1 executes the addObjectToPool method which deferences _factory.
> 
> Many other races are because parts of some methods implementing the
> pool interfaces (ex. invalidateObject below) that access _factory are
> not synchronized:
> 
> public void invalidateObject(Object obj) throws Exception {
>      assertOpen();
>      try {
>          _factory.destroyObject(obj);
>      }
>      finally {
>          synchronized(this) {
>              _numActive--;
>              notifyAll();
>          }
>      }
> }
> 
> Perhaps, the developer expects the client methods implementing the
> factory interface to be synchronized.  If this is indeed the case, it
> should perhaps be documented somewhere so that unwitting users don't
> implement the factory interface without synchronization.  Even if
> users implement the factory interface with synchronization, there is a
> problem: if one thread executes the invalidateObject method and
> another executes the close method (which sets _factory to null), then
> there can be a null pointer exception similar to the scenario outlined
> above.
> 
> Most of the races I found are harmful (ex. causing null pointer
> exceptions) if the close method is called concurrently with some other
> pool interface method.  Perhaps, it is highly unlikely that a client
> would do that.  However, since the pool interface is intended to be
> thread-safe, I think it's implementations should handle this scenario
> gracefully.
> 
> I have summarized below all possible bugs I found by inspecting the
> above reports generated by my race detection tool.  I would be happy
> if you can confirm/refute these bugs.
> 
> Thanks!
> -- Mayur
> 
> org.apache.commons.pool.impl.GenericObjectPool
> ==============================================
> 
> The returnObject(Object) and addObject() methods must be synchronized.
>  The synchronization inside the body of the method addObjectToPool is
> then unnecessary since all callers of this method are synchronized.
> 
> The entire invalidateObject(Object) method must be synchronized, not
> just parts of it.
> 
> The entire borrowObject() method must be synchronized, not just parts
> of it.  Note that there are dereferences of _factory in the
> unsynchronized portions of this method, so there will be a null
> pointer dereference if it is called concurrently with the close
> method.  Once the entire borrowObject method is synchronized, you can
> also move the assertOpen() call at the start of each iteration of the
> loop in this method to outside the loop.
> 
> org.apache.commons.pool.impl.StackObjectPool
> ============================================
> 
> The entire returnObject(Object) method must be synchronized, not just
> parts of it.
> 
> The getNumIdle() method must be synchronized.
> 
> The getNumActive() method must be synchronized.
> 
> The entire addObject() method must be synchronized, not just parts of it.
> 
> org.apache.commons.pool.impl.SoftReferenceObjectPool
> ====================================================
> 
> The entire returnObject(Object) method must be synchronized, not just
> parts of it.
> 
> The entire addObject() method must be synchronized, not just parts of it.
> 
> The getNumIdle() method must be synchronized.
> 
> org.apache.commons.pool.impl.GenericKeyedObjectPool
> ===================================================
> 
> The entire returnObject(Object,Object) method must be synchronized,
> not just parts of it.
> 
> The entire invalidateObject(Object,Object) method must be
> synchronized, not just parts of it.
> 
> The entire addObject(Object) method must be synchronized, not just parts of it.
> 
> org.apache.commons.pool.impl.StackKeyedObjectPool
> =================================================
> 
> The entire addObject(Object) method must be synchronized, not just parts of it.
> 
> The getNumActive(Object) method must be synchronized.
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message