Hi Phil,
Phil Steitz wrote:
Thanks, Christoph
On Sat, Mar 1, 2008 at 4:21 AM, Christoph Kutzinski <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
