Yes, not destroying the borrowed object would be an option. I didn't want to go
down that route as there are multiple reasons why a borrowed object is not
valid anymore.
E.g. it could be 'consumed', it could be broken, shouldn't be used do to
maxUsageTime is over, etc.
That's why I opted for calling create(). But of course, that has other
potential issues :/
LieGrue,
strub
> Am 28.11.2018 um 04:21 schrieb Phil Steitz :
>
> On 11/26/18 1:23 PM, Phil Steitz wrote:
>> On 11/26/18 8:29 AM, Phil Steitz wrote:
>>> On 11/26/18 6:19 AM, Mark Struberg wrote:
Hi Phil!
>>>
>>> Let me start by repeating that other than trying to help diagnose bugs and
>>> answer user questions, I don't really work on [pool] any more, so I don't
>>> really have any standing here. You are the RM, so it is completely up to
>>> you and the active Commons committers to decide what to do with the code.
>>> So fine to ignore my comments.
> I think that the right answer here is WONT_FIX. That sounds harsh, but
> it seems to me
> that the obvious solution here is for the user to set maxIdle to at least
> 1.
I thought about that as well!
Setting maxIdle to 1 will make it less likely but a deadlock will STILL
happen.
>>> I am not sure this is right. If maxIdle >0, the fix for POOL-240 (a
>>> similar liveness issue) should kick in and work. Each time a validation or
>>> passivation failure occurs on return, there is a call to ensureIdle that
>>> will create a new instance and add it to the pool if there is capacity to
>>> do so.
So I fear we really need to tackle this. Stackoverflow and our own bug
tracker is full of such reports :(
>>>
>>> I see one additional actual report on GKOP (the other linked issue, which
>>> should be similarly patched if the consensus is to do this for GOP). The
>>> vast majority of the liveness reports that we have gotten over the years in
>>> DBCP and pool are just pool exhausted due to failure to return instances.
>>>
>>> The workaround violating maxIdle will restore liveness, but is likely the
>>> wrong solution here. Again, up to you guys to judge. Note that when you
>>> specify maxIdle equal to 1
>> Crap. Meant 0 there.
>>> you are telling the pool to destroy all returning instances. To use the
>>> pool in this way is silly, IMO. With your patch, new instances are still
>>> created for *every* borrow. The pool and all of its machinery is doing
>>> nothing other than tracking the number of active instances and making sure
>>> life cycle events are fired. If you want the semantics of returning object
>>> with threads waiting to be that the returning object is passivated,
>>> activated and handed directly to a waiter without every hitting the idle
>>> instance pool, that would require rewriting a fair bit of the borrow /
>>> return code. It is an interesting idea and would solve the POOL-240 as
>>> well.
>>>
>>> One final comment. If you stick with something like what is in the code
>>> now, you should make sure to passivate the new instance before putting it
>>> into the pool. I just noticed that ensureIdle does not do that, which I
>>> think is a bug in that method. So if you want to proceed with this fix, I
>>> would recommend
>>>
>>> 1. Move the ensureIdle activations added in POOL-240 into destroy itself.
>>> 2. Add passivation to ensureIdle
>>> 3. Implement corresponding workaround for GKOP
>
> Sorry, all. Looking again and thinking a little more, I think the best fix
> for this is actually to prevent the destroy on return in the first place if
> there are take waiters. The maxIdle invariant gets violated with the
> ensureIdle or direct create() fix anyway and just allowing the returning
> instance to go into the pool avoids the needless destroy / create sequence.
> A simple way to do this is just to recode maxIdle before the destroy test in
> returnObject.
> Instead of
>
> final int maxIdleSave = getMaxIdle()
> explicitly recode it:
>
> // If maxIdle is set to 0 and there are take waiters, bump it to 1
> // to preserve liveness.
> final int maxIdleSave = idleObjects.hasTakeWaiters() ?
> Math.max(1,getMaxIdle()) : getMaxIdle();
>
> Phil
>
>>>
>>> Phil
>>>
>>>
LieGrue,
strub
> Am 23.11.2018 um 16:51 schrieb Phil Steitz:
>
> On 11/23/18 2:57 AM, Mark Struberg wrote:
>> should read: This change (putting a new item back to the idle pool) was
>> needed to prevent a dead-lock
>>
>> *grabbing a fresh coffee* le
> I am sorry I did not look carefully enough at this issue before reviewing
> the change. After reviewing the DBCP ticket (OP likely unrelated, IMO),
> I think that the right answer here is WONT_FIX. That sounds harsh, but it
> seems to me that the obvious solution here is for the user to set maxIdle
> to at least 1. What the fix does is effectively that, without changing
> the setting.
