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
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.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
