First cut committed in r1379533. Thanks in advance for review.
Some additional comments, including some decisions made in the
patch, inline below.
On 8/24/12 9:57 AM, Phil Steitz wrote:
> 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 stuck with GOP-only to start. This resulted in abandonement
properties added to GOP instead of BGOP. If / when GKOP gets this,
these properties will have to move up.
>>
>>>> - 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.
I also added "isAbandonedConfig" to flag that abandoned object
removal is configured.
>>
>>>> - 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.
Hopefully javadoc is now clear. The starvation criteria is used
when abandoned object removal is configured to fire on borrowObject,
but not when the evictor does it. The rationale there is that
slowing down borrowObject is worse than slowing down the evictor, so
only impose starvation criteria there.
> 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.
Exposed everything but the logwriter.
>>>> 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.
Hopefully what I committed is now thread safe. I ended up adding
two states - ABANDONED and RETURNING. I had to add the second one
because when abandoned object removal is configured, it is possible
to mess with checked out objects as they are returning. I added
test cases in TestAbandonedObjectPool that force races that fail
without these states and the protection they provide.
One final thing I needed to change. In both returnObject and
invalidateObject, the pre-patch code throws ISE if the returning or
invalidating object is not in allObjects. I had to change that to
no-op instead if abandoned object removal is configured to allow for
the situation where a client returns or invalidates an object that
has already been removed abandoned. Arguably, we should still throw
ISE in this case; and control is weakened as a result of this change.
Phil
>
> Phil
>> Mark
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>> For additional commands, e-mail: dev-h...@commons.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
