commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mayur Naik <...@cs.stanford.edu>
Subject [pool] synchronization issues in Pool
Date Sun, 23 Oct 2005 08:04:41 GMT

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


Mime
View raw message