commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <>
Subject Re: [pool][dbcp] POOL-229 (move AbandonedObjectPool to pool)
Date Fri, 24 Aug 2012 15:39:37 GMT
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.

>> - 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.

>> - 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
- 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.


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message