commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Zeigermann <oliver.zeigerm...@gmail.com>
Subject Re: [pool] synchronization issues in Pool
Date Sun, 23 Oct 2005 15:52:32 GMT
Hi Mayur,

that really sounds like a cool tool. You results are also quite
impressive. I actually can not comment on them, but would like to know
if your tool is available anywhere?

If not - and I suppose so - could you also run it on the commons
transaction code where any race condition really would be very
harmful. I already know of one that has been fixed in CVS, but not in
a final release.

Thanks in advance

Oliver

2005/10/23, Mayur Naik <mhn@cs.stanford.edu>:
>
> 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