Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Jakarta Commons Developers List" <dev@commons.apache.org>
Received-SPF: pass (nike.apache.org: domain of phil.steitz@gmail.com
 designates 209.85.142.186 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references;
        b=poRntR4eHA+m/NNwE1dub3GvnrpC65jkvUGhkA+pdoLTv7T7n+yG+vVjz3n2NOkY+xuHS40uyrgjgRcyUbJvO5l2QOhnkEkYkmBM6X4Vb99mpANqMHwoJjXYhzWL8XQon6C9IDNbNMB7/Uf8DF2YbcEDOfJN2vBm05nWWaGPpIQ=
Message-ID: <8a81b4af0803022235r14deed0bo75c58c689ae586c0@mail.gmail.com>
Date: Sun, 2 Mar 2008 23:35:00 -0700
From: "Phil Steitz" <phil.steitz@gmail.com>
To: "Jakarta Commons Developers List" <dev@commons.apache.org>
Subject: Re: [pool] New DBCP deadlock reported (DBCP-44)
In-Reply-To: <47CAA6FC.20801@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <8a81b4af0802290513r2199b2c8v4c94b8e691e8743f@mail.gmail.com>
	 <47C93C1E.7030309@gmx.de>
	 <8a81b4af0803011705t40aed931t248dbbd9e2882761@mail.gmail.com>
	 <47CAA6FC.20801@gmx.de>

On Sun, Mar 2, 2008 at 6:09 AM, Christoph Kutzinski <kutzi@gmx.de> wrote:
> 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?
>
No, you are not missing anything.  This should not be hard.
>
>
>  >  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
>
This gets the the issue you have identified below.
>
>  > 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.
>
What race conditions, exactly?
Not having thought this through all the way, I was thinking about an
integer property that could take one of four values
"BORROWED" - a ghost in the pool snapshot
"IDLE"  - in the pool, OK to borrow
"DEAD" - in the pool, marked for eviction
"VALIDATING" - in the pool, being validated
If we need to synchronize, similarly to AtomicInteger, we could define
a synchronized compareAndSet method that borrowObject, returnObject,
and evict could use to control access.  Synchronization would, as you
point out, cost something, but there should not be much contention for
these locks.  I need to work through the various race conditions,
though, to convince myself that a volatile field properly managed
would not suffice.

Thanks again for your thoughts on this.

Phil

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