Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
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 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. If waiting threads die or time out while the create is happening, there will be an idle instance in the pool and for certain one is being put there on the way to getting checked back out. See comments on POOL-327. If the consensus is to insert this workaround to enable the pool to retain liveness in the use case, it's probably best to use ensureIdle(1, false) (see POOL-240). It could be that just moving the call to ensureIdle inside destroy would be OK. But as stated above, this breaks the maxIdle contract. I see that your original report / use case here is from DBCP, Mark. Was it prepared statements or connections that you were trying to limit to 0 idle? Is there a reason that just using 1 would not work? Phil Am 23.11.2018 um 10:49 schrieb Mark Struberg: This change (putting a new item back to the idle pool was needed to prevent a dead-pool - 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 - To unsubscribe, e-mail:dev-unsubscr...@commons.apache.org For additional commands, e-mail:dev-h...@commons.apache.org - To unsubscribe, e-mail:
Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
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 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 Sorry, looks to me like this is not actually necessary. So I take back my comment that this is a bug in ensureIdle. 3. Implement corresponding workaround for GKOP 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. If waiting threads die or time out while the create is happening, there will be an idle instance in the pool and for certain one is being put there on the way to getting checked back out. See comments on POOL-327. If the consensus is to insert this workaround to enable the pool to retain liveness in the use case, it's probably best to use ensureIdle(1, false) (see POOL-240). It could be that just moving the call to ensureIdle inside destroy would be OK. But as stated above, this breaks the maxIdle contract. I see that your original report / use case here is from DBCP, Mark. Was it prepared statements or connections that you were trying to limit to 0 idle? Is there a reason that just using 1 would not work? Phil Am 23.11.2018 um 10:49 schrieb Mark Struberg: This change (putting a new item back to the idle pool was needed to prevent a dead-pool - 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 - To unsubscribe, e-mail:dev-unsubscr...@commons.apache.org For additional commands, e-mail:dev-h...@commons.apache.org
Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
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 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 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. If waiting threads die or time out while the create is happening, there will be an idle instance in the pool and for certain one is being put there on the way to getting checked back out. See comments on POOL-327. If the consensus is to insert this workaround to enable the pool to retain liveness in the use case, it's probably best to use ensureIdle(1, false) (see POOL-240). It could be that just moving the call to ensureIdle inside destroy would be OK. But as stated above, this breaks the maxIdle contract. I see that your original report / use case here is from DBCP, Mark. Was it prepared statements or connections that you were trying to limit to 0 idle? Is there a reason that just using 1 would not work? Phil Am 23.11.2018 um 10:49 schrieb Mark Struberg: This change (putting a new item back to the idle pool was needed to prevent a dead-pool - 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 - 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
Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
I think we still need to address what happens if null gets returned in create(). This was something I missed. Not sure if it got addressed in the meantime? LieGrue, strub > Am 26.11.2018 um 14:26 schrieb Rob Tompkins : > > > >> On Nov 26, 2018, at 8:16 AM, Mark Struberg wrote: >> >> Hi Gary! >> >> I've added multi-line comments in the middle of code blocks I touched. >> e.g. >> https://github.com/apache/commons-pool/blob/016a1f67263fe1cde1d910dc7002d972811951c5/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java#L919 >> >> I also tried to write extensive commit comments. > > Sounds good. I’ll try to get to starting the release today. > > -Rob > >> >> LieGrue, >> strub >> >>> Am 23.11.2018 um 16:18 schrieb Gary Gregory : >>> >>> On Fri, Nov 23, 2018 at 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* > Am 23.11.2018 um 10:49 schrieb Mark Struberg : > > This change (putting a new item back to the idle pool was needed to prevent a dead-pool >>> >>> Hi Mark, >>> >>> Would you mind adding some comments to the code to help future maintainers? >>> >>> Gary (currently sipping coffee) >>> >>> - 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 >> > > - > 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
Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
> On Nov 26, 2018, at 8:16 AM, Mark Struberg wrote: > > Hi Gary! > > I've added multi-line comments in the middle of code blocks I touched. > e.g. > https://github.com/apache/commons-pool/blob/016a1f67263fe1cde1d910dc7002d972811951c5/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java#L919 > > I also tried to write extensive commit comments. Sounds good. I’ll try to get to starting the release today. -Rob > > LieGrue, > strub > >> Am 23.11.2018 um 16:18 schrieb Gary Gregory : >> >> On Fri, Nov 23, 2018 at 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* >>> Am 23.11.2018 um 10:49 schrieb Mark Struberg : This change (putting a new item back to the idle pool was needed to >>> prevent a dead-pool >>> >> >> Hi Mark, >> >> Would you mind adding some comments to the code to help future maintainers? >> >> Gary (currently sipping coffee) >> >> >>> >>> >>> - >>> 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 > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
Hi Phil! > 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. So I fear we really need to tackle this. Stackoverflow and our own bug tracker is full of such reports :( 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. If > waiting threads die or time out while the create is happening, there will be > an idle instance in the pool and for certain one is being put there on the > way to getting checked back out. See comments on POOL-327. > > If the consensus is to insert this workaround to enable the pool to retain > liveness in the use case, it's probably best to use ensureIdle(1, false) (see > POOL-240). It could be that just moving the call to ensureIdle inside > destroy would be OK. But as stated above, this breaks the maxIdle contract. > > I see that your original report / use case here is from DBCP, Mark. Was it > prepared statements or connections that you were trying to limit to 0 idle? > Is there a reason that just using 1 would not work? > > Phil > > >> >>> Am 23.11.2018 um 10:49 schrieb Mark Struberg : >>> >>> This change (putting a new item back to the idle pool was needed to prevent >>> a dead-pool >>> >>> - >>> 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 > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [VOTE] [CANCEL] Release Apache Commons Pool2 2.6.1
Hi Gary! I've added multi-line comments in the middle of code blocks I touched. e.g. https://github.com/apache/commons-pool/blob/016a1f67263fe1cde1d910dc7002d972811951c5/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java#L919 I also tried to write extensive commit comments. LieGrue, strub > Am 23.11.2018 um 16:18 schrieb Gary Gregory : > > On Fri, Nov 23, 2018 at 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* >> >>> Am 23.11.2018 um 10:49 schrieb Mark Struberg : >>> >>> This change (putting a new item back to the idle pool was needed to >> prevent a dead-pool >> > > Hi Mark, > > Would you mind adding some comments to the code to help future maintainers? > > Gary (currently sipping coffee) > > >> >> >> - >> 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
