commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christoph Kutzinski <ku...@gmx.de>
Subject Re: [pool] New DBCP deadlock reported (DBCP-44)
Date Sun, 02 Mar 2008 13:09:16 GMT
Hi Phil,

Phil Steitz wrote:
> Thanks, Christoph
> 
> On Sat, Mar 1, 2008 at 4:21 AM, Christoph Kutzinski <kutzi@gmx.de> wrote:
>> As the 'inversed' locking order in evict() is obviously the source of
>>  the problems, I would try to avoid calling any _factory methods while
>>  holding the 'this' lock.
> 
> +1
> evict and addObject currently violate this and I think both should be fixed.
> 
>>  I thinking about something like this:
>>
>>  public void evict() {
>>    CursorableLinkedList tmpPool;
>>    Collection connsToEvict = new ArrayList();
>>    synchronized(this) {
>>      tmpPool = new CursorableLinkedList(_pool);
>>    }
>>
>>    // checking for conns to evict, calling methods on _factory and adding
>>    // conns to evict to connsToEvict
>>
>>    synchronized(this) {
>>      // remove conns in connsToEvict from _pool
>>      // ignoring conns that are (currently) not in pool
>>    }
>>   }
>>
>>  Obviuosly, this doesn't work with the evictionCursor as it is
>>  implemented now, so it would require some work on that one.
>>
> 
> I think something like this might work if we can solve:
> 1)  Maintaining the cycling behavior of the evictionCursor -  will be
> tricky, but should be doable.

I haven't looked into the implementation, but I think this shouldn't be 
to hard. Especially if one would do only a 'best effort' approach, i.e. 
it would be possible that pooled objects are skipped or are repeatedly 
checked, a simple index into the _pool should be ok. Or am I missing 
something?


>  2)  "ignoring conns that are
> (currently) not in pool" in the second synch block - hard to do
> efficiently, but again should be doable

What problems do you see here? I'm not sure why this shouldn't be 
possible to do efficiently

> 3) when to do _factory.destroyObject - can't do this is the
> mid-section because borrowObject could jump in and grab the destroyed
> object.  Need some way to prevent that, maybe by locking / marking the
> ObjectTimeStampPair.

That could be done after the second synchronized block, if we remember 
only the successful removed objects in connsToEvict (i.e. removing the 
ones which couldn't be removed from _pool)

But that have made me think about a (probably) major flaw in this approach:
It is probably not ok to access a pooled object from within the evictor 
thread while it is possibly concurrently accessed from a thread which 
borrowed it from the pool (and AFAIS we cannot prevent this scenario 
with this approach).
So I also thought about marking the ObjectTimeStampPair as either being 
currently 'inEviction' or 'borrowed' or both (volatile boolean fields in 
ObjectTimeStampPair). But I fear that this would be vulnerable to 
various race conditions.

But I think that it might work if we would protect the pooled objects 
via proxy objects (ObjectTimeStampPair could be reused for this) and 
only access all factory methods via this one.

Sketch:
static class PooledObjectProxy implements Comparable {
   Object value;
   long tstamp;
   boolean borrowed = false;

   public synchronized setBorrowed(boolean borrowed) ...

   public synchronized boolean validateObject() {
    if(borrowed) {
      return true;
    } else {
      return _factory.validateObject( value );
    }
   }

   public synchronized void activateObject() {
    if(borrowed) {
      return;
    } else {
      _factory.activateObject( value );
    }
   }

   public synchronized void destroyObject() {
    if(borrowed) {
      // TODO: should not happen: throw Exception?
    } else {
      _factory.destroyObject( value );
    }
   }
}

public Object borrowObject() {
   // find PooledObjectProxy
   ...
   pooledObjectProxy.setBorrowed(true);
}

public void returnObject(Object obj) {
   ...
   pooledObjectProxy.setBorrowed(false);
}

However, I still think this might me vulnerable to race conditions, 
maybe we also need an 'evicted' boolean in PooledObjectProxy to prevent 
borrowing objects which are already designated to be evicted.
Of course this approach has also its performance downside, because every 
access to PooledObjectProxy has to be synchronized on it, but it is 
probably still better than the alternatives (at least I hope so).

Christoph




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


Mime
View raw message