commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: [pool][dbcp] POOL-229 (move AbandonedObjectPool to pool)
Date Fri, 24 Aug 2012 16:57:32 GMT
On 8/24/12 8:39 AM, Mark Thomas wrote:
> On 24/08/2012 16:22, Phil Steitz wrote:
>> On 8/24/12 7:42 AM, Mark Thomas wrote:
>>> On 22/08/2012 13:51, Phil Steitz wrote:
>>>> Any feedback on the patch?  Should I go ahead and apply the [pool]
>>>> patch?
>>> Sorry for the very late review. Some comments/questions on the pool2
>>> part (I haven't looked at DBCP):
>>>
>>> - Why not add abandoned support to GKOP as well as GOP?
>> Could definitely be done, but GKOP is tricky because of the dual
>> parameter set (per key / global) and starvation prevention stuff
>> already there.  I don't see the need to add it right away, but am OK
>> with the idea and might eventually get to it.
> Fair enough.
>
>>> - I wasn't convinced at first about separating standard and abandoned
>>>   config. It is growing on me but I'd be interested in hearing your
>>>   reasons for this.
>> I was in exactly the same place.  I actually coded an initial
>> version combining it all, looked at it and thought it was simpler to
>> keep them separate.  I do not have strong feelings either way.  I
>> chose to keep them separate because a) it was a smaller change and
>> b) I liked the encapsulation of abandoned object cleanup parameters
>> (and not having to look at them if not using this).
> Fair enough. Works for me since I was thinking pretty much the same.
>
>>> - What is the reasoning behind the tests:
>>>   - getNumIdle() < 2
>>>   - getNumActive() > getMaxTotal() - 3
>>>   in borrowObject()? It would be good to document these reasons in a
>>>   comment.
>> Hmm.  Archaeology ;)
>> Seriously, I suspect the original reasoning behind this test remains
>> valid for activation within borrowObject - you don't want to take
>> the hit of doing an abandoned cleanup if you are flush with
>> instances.  Note that there is a flag that controls whether
>> borrowObject does cleanup at all (actually was in the original).
> The patch is missing the AbandonedConfig file which I suspect would have
> some Javadoc that would help here.

Oops!  Forgot the svn add there.
>
>>> - I think an explicit state of PooledObjectState.ABANDONED would be
>>>   clearer / easier to work with (see below).
>> You are probably right.
>>> - There are various timing issues/questions in removeAbandoned()
>>>   - what is the lock on allObjects meant to achieve?
>> It was intended to ensure that remove was formed using a consistent
>> snapshot.  Given the way ConcurrentHashMap works, this is not
>> correct.  So there is no value in the lock.
>>>   - an object may be returned and re-borrowed after being added to
>>>     "remove" and before it is invalidated
>> Right.  That is why this check is there
>>  if (pooledObject.getState() != PooledObjectState.ALLOCATED) {
>>                     continue;
>> }
> You can't differentiate between still allocated (and abandoned) and
> return and re-allocated.

Good catch.  Shows the need for some more / better tests.  I will
see if I can code some tests that fail with the current patch and
succeed with a correct impl along the lines of what you describe below.
>
>>> - abandonedConfig should be in the config section not the JMX section
>> Good catch.  Will add that.
>>>
>>> My instinct is that a checkAbandoned() method on PooledObject that
>>> changes the state to PooledObjectState.ABANDONED and returns true if it
>>> does this is likely to be an easier starting point for a thread-safe
>>> solution rather than separating the timing check and the state change.
>> Can you explain more what you mean here?
> The current patch does not perform the following actions in an atomic
> manner:
> - check to see if object is abandoned
> - mark the object as abandoned / invalidate it
>
> Currently these steps are separate. All sorts of things could happen
> between the two steps. With the aim of avoiding the sort of timing bugs
> that gave me a headache with POOLv1, I would much prefer to see a method
> (probably on PooledObject) that checked the last used time and changed
> the state atomically.

Great idea.  I was trying to use the PooledObject's monitor and
external logic to do this; but as you point out, I made at least one
mistake.  I agree this brings back memories of our fun with latches
in 1.5.x.  I will take a crack at a correct impl using the ideas
above and commit.

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


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


Mime
View raw message