Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Reusing this thread to note FTR that a new version of POOL is out and that a DBCP PR based of the new POOL is in progress... Gary On Wed, Sep 23, 2020, 19:28 Gary Gregory wrote: > On Wed, Sep 23, 2020 at 6:31 PM Phil Steitz wrote: > >> >> On 9/23/20 2:57 PM, Gary Gregory wrote: >> > This all sounds reasonable but it would be easier to see in a PR. >> >> I will do that once the new version of [pool] is released. Otherwise, I >> would have to add snapshot dependency, which I am sure would cause CI to >> fail. >> > > OK, sounds good, I'll push out a [pool] RC later this week after the > [dbcp] RC. > > Gary > > >> >> Phil >> >> > >> > Gary >> > >> > On Tue, Sep 22, 2020 at 8:30 PM Phil Steitz >> wrote: >> > >> >> On 9/14/20 10:10 AM, Gary Gregory wrote: >> >>> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz >> >> wrote: >> On 9/14/20 9:36 AM, Gary Gregory wrote: >> > This feature is now in Pool master. I will prepare an RC soon if you >> > all think we are good to go so we can then move on to DBCP. >> I am still working on testing this in the DBCP use case. Probably >> best >> to wait a little for others to review and for me to get the DBCP >> change >> tested against current pool sources. I should be able to finish that >> this weekend. >> >> I implemented changes in DBCP, based on recently committed changes in >> >> pool. I made a few decisions that I would appreciate feedback on. >> >> >> >> 1. The JDBC abort method requires an Executor as actual parameter. I >> >> added a FixedThreadPool executor with a max of 3 threads to >> >> PoolableConnectionFactory for this purpose. Given that this operation >> >> might hang sometimes, I thought it best to allow more than a single >> >> thread. I guess it could be configurable, but 3 seemed a reasonable >> >> choice. I copied the custom thread factory used by pool's >> EvictionTimer >> >> to source daemon threads based on ccl for this. I then added a close() >> >> method to PCF that closes the executor and modified BasicDataSource >> >> close to call this. >> >> >> >> 2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF >> >> destroyObject when abandoned mode is passed in rather than just >> >> getObject().abort as destroy invokes close (I wonder if that is a bug?) >> >> >> >> 3. I changed JdbcBridge#abort to remove the checkOpen() check. I was >> >> getting exceptions in my concurrency tests where connections were being >> >> closed but then considered abandoned. This is a legit race that abort >> >> will generally handle fine and I don't see the need to throw. >> >> >> >> Phil >> >> >> >>> Sounds good. >> >>> >> >>> Gary >> >>> >> Phil >> >> > Gary >> > >> > On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory > > >> >> wrote: >> >> FWIW, I like the name "DestroyMode" because it matches the >> "destroy" >> >> in the method name. >> >> Gary >> >> >> >> On Mon, Sep 7, 2020, 19:08 Gary Gregory >> >> wrote: >> >>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz > > >> >> wrote: >> On 9/3/20 2:44 AM, Mark Thomas wrote: >> > On 31/08/2020 01:05, Phil Steitz wrote: >> > >> > >> > >> >> If others agree it is a good idea for dbcp, I can do it. I can >> >> see the >> >> argument that its better to stay with close() even for >> abandoned >> >> and I >> >> have not been able to get the deadlock to happen, so I would >> like >> >> to >> >> wait a bit and allow others to weigh in. Similarly for pool, I >> >> would >> >> like to get more community feedback before adding to the >> >> interface. >> > Hmm. >> > >> > On one hand if the driver deadlocks I don't see how that can be >> >> anything >> > other than a driver bug. If multiple code paths obtain multiple >> >> locks >> > then those code paths must always obtain those locks in the same >> >> order. >> > Anything else is a bug that is likely to result in a deadlock >> in a >> > multithreaded environment. >> > >> > On the other hand, it could be argued that the situation only >> >> arises >> > when an application doesn't correctly return connections to the >> >> pool >> > and/or keeps them for too long and/or doesn't configure the pool >> > correctly for their usage pattern. >> > >> > The approach of adding >> > >> > PooledObjectFactory.destroyObject(PooledObject,CloseMode) >> > >> > where CloseMode is an Enum with two values looks reasonable to >> me. >> I have started to work on the [pool] changes for this. I want to >> >> check >> two things before completing the PR: >> >> 1. "Close" is a [dbcp] concept which does not make sense for all >> >> pool >> factories, so I am going to name the enum "DestroyMode" and the >> two >> modes, "Default" and "Abandoned".
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On Wed, Sep 23, 2020 at 6:31 PM Phil Steitz wrote: > > On 9/23/20 2:57 PM, Gary Gregory wrote: > > This all sounds reasonable but it would be easier to see in a PR. > > I will do that once the new version of [pool] is released. Otherwise, I > would have to add snapshot dependency, which I am sure would cause CI to > fail. > OK, sounds good, I'll push out a [pool] RC later this week after the [dbcp] RC. Gary > > Phil > > > > > Gary > > > > On Tue, Sep 22, 2020 at 8:30 PM Phil Steitz > wrote: > > > >> On 9/14/20 10:10 AM, Gary Gregory wrote: > >>> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz > >> wrote: > On 9/14/20 9:36 AM, Gary Gregory wrote: > > This feature is now in Pool master. I will prepare an RC soon if you > > all think we are good to go so we can then move on to DBCP. > I am still working on testing this in the DBCP use case. Probably best > to wait a little for others to review and for me to get the DBCP > change > tested against current pool sources. I should be able to finish that > this weekend. > >> I implemented changes in DBCP, based on recently committed changes in > >> pool. I made a few decisions that I would appreciate feedback on. > >> > >> 1. The JDBC abort method requires an Executor as actual parameter. I > >> added a FixedThreadPool executor with a max of 3 threads to > >> PoolableConnectionFactory for this purpose. Given that this operation > >> might hang sometimes, I thought it best to allow more than a single > >> thread. I guess it could be configurable, but 3 seemed a reasonable > >> choice. I copied the custom thread factory used by pool's EvictionTimer > >> to source daemon threads based on ccl for this. I then added a close() > >> method to PCF that closes the executor and modified BasicDataSource > >> close to call this. > >> > >> 2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF > >> destroyObject when abandoned mode is passed in rather than just > >> getObject().abort as destroy invokes close (I wonder if that is a bug?) > >> > >> 3. I changed JdbcBridge#abort to remove the checkOpen() check. I was > >> getting exceptions in my concurrency tests where connections were being > >> closed but then considered abandoned. This is a legit race that abort > >> will generally handle fine and I don't see the need to throw. > >> > >> Phil > >> > >>> Sounds good. > >>> > >>> Gary > >>> > Phil > > > Gary > > > > On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory > >> wrote: > >> FWIW, I like the name "DestroyMode" because it matches the "destroy" > >> in the method name. > >> Gary > >> > >> On Mon, Sep 7, 2020, 19:08 Gary Gregory > >> wrote: > >>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz > >> wrote: > On 9/3/20 2:44 AM, Mark Thomas wrote: > > On 31/08/2020 01:05, Phil Steitz wrote: > > > > > > > >> If others agree it is a good idea for dbcp, I can do it. I can > >> see the > >> argument that its better to stay with close() even for abandoned > >> and I > >> have not been able to get the deadlock to happen, so I would > like > >> to > >> wait a bit and allow others to weigh in. Similarly for pool, I > >> would > >> like to get more community feedback before adding to the > >> interface. > > Hmm. > > > > On one hand if the driver deadlocks I don't see how that can be > >> anything > > other than a driver bug. If multiple code paths obtain multiple > >> locks > > then those code paths must always obtain those locks in the same > >> order. > > Anything else is a bug that is likely to result in a deadlock in > a > > multithreaded environment. > > > > On the other hand, it could be argued that the situation only > >> arises > > when an application doesn't correctly return connections to the > >> pool > > and/or keeps them for too long and/or doesn't configure the pool > > correctly for their usage pattern. > > > > The approach of adding > > > > PooledObjectFactory.destroyObject(PooledObject,CloseMode) > > > > where CloseMode is an Enum with two values looks reasonable to > me. > I have started to work on the [pool] changes for this. I want to > >> check > two things before completing the PR: > > 1. "Close" is a [dbcp] concept which does not make sense for all > >> pool > factories, so I am going to name the enum "DestroyMode" and the > two > modes, "Default" and "Abandoned". That leaves room for other > modes > >> like > "Evicted" or "Invalid" later. > > 2. Speaking of later, technically adding modes will not break > binary > compatibility. Are we going to be OK adding outside a major > >> release? > If the answer is no, I might argue to include the other
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 9/23/20 2:57 PM, Gary Gregory wrote: This all sounds reasonable but it would be easier to see in a PR. I will do that once the new version of [pool] is released. Otherwise, I would have to add snapshot dependency, which I am sure would cause CI to fail. Phil Gary On Tue, Sep 22, 2020 at 8:30 PM Phil Steitz wrote: On 9/14/20 10:10 AM, Gary Gregory wrote: On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz wrote: On 9/14/20 9:36 AM, Gary Gregory wrote: This feature is now in Pool master. I will prepare an RC soon if you all think we are good to go so we can then move on to DBCP. I am still working on testing this in the DBCP use case. Probably best to wait a little for others to review and for me to get the DBCP change tested against current pool sources. I should be able to finish that this weekend. I implemented changes in DBCP, based on recently committed changes in pool. I made a few decisions that I would appreciate feedback on. 1. The JDBC abort method requires an Executor as actual parameter. I added a FixedThreadPool executor with a max of 3 threads to PoolableConnectionFactory for this purpose. Given that this operation might hang sometimes, I thought it best to allow more than a single thread. I guess it could be configurable, but 3 seemed a reasonable choice. I copied the custom thread factory used by pool's EvictionTimer to source daemon threads based on ccl for this. I then added a close() method to PCF that closes the executor and modified BasicDataSource close to call this. 2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF destroyObject when abandoned mode is passed in rather than just getObject().abort as destroy invokes close (I wonder if that is a bug?) 3. I changed JdbcBridge#abort to remove the checkOpen() check. I was getting exceptions in my concurrency tests where connections were being closed but then considered abandoned. This is a legit race that abort will generally handle fine and I don't see the need to throw. Phil Sounds good. Gary Phil Gary On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory wrote: FWIW, I like the name "DestroyMode" because it matches the "destroy" in the method name. Gary On Mon, Sep 7, 2020, 19:08 Gary Gregory wrote: On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: On 9/3/20 2:44 AM, Mark Thomas wrote: On 31/08/2020 01:05, Phil Steitz wrote: If others agree it is a good idea for dbcp, I can do it. I can see the argument that its better to stay with close() even for abandoned and I have not been able to get the deadlock to happen, so I would like to wait a bit and allow others to weigh in. Similarly for pool, I would like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I have started to work on the [pool] changes for this. I want to check two things before completing the PR: 1. "Close" is a [dbcp] concept which does not make sense for all pool factories, so I am going to name the enum "DestroyMode" and the two modes, "Default" and "Abandoned". That leaves room for other modes like "Evicted" or "Invalid" later. 2. Speaking of later, technically adding modes will not break binary compatibility. Are we going to be OK adding outside a major release? If the answer is no, I might argue to include the other natural modes now. Yes, IMO, it is OK to add enum values in a minor release since it does not break binary (or source) compatibility. Gary Phil I do agree that abort() should only be used in the case of abandoned connections. 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 - 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
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
This all sounds reasonable but it would be easier to see in a PR. Gary On Tue, Sep 22, 2020 at 8:30 PM Phil Steitz wrote: > > On 9/14/20 10:10 AM, Gary Gregory wrote: > > On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz > wrote: > >> > >> On 9/14/20 9:36 AM, Gary Gregory wrote: > >>> This feature is now in Pool master. I will prepare an RC soon if you > >>> all think we are good to go so we can then move on to DBCP. > >> I am still working on testing this in the DBCP use case. Probably best > >> to wait a little for others to review and for me to get the DBCP change > >> tested against current pool sources. I should be able to finish that > >> this weekend. > > I implemented changes in DBCP, based on recently committed changes in > pool. I made a few decisions that I would appreciate feedback on. > > 1. The JDBC abort method requires an Executor as actual parameter. I > added a FixedThreadPool executor with a max of 3 threads to > PoolableConnectionFactory for this purpose. Given that this operation > might hang sometimes, I thought it best to allow more than a single > thread. I guess it could be configurable, but 3 seemed a reasonable > choice. I copied the custom thread factory used by pool's EvictionTimer > to source daemon threads based on ccl for this. I then added a close() > method to PCF that closes the executor and modified BasicDataSource > close to call this. > > 2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF > destroyObject when abandoned mode is passed in rather than just > getObject().abort as destroy invokes close (I wonder if that is a bug?) > > 3. I changed JdbcBridge#abort to remove the checkOpen() check. I was > getting exceptions in my concurrency tests where connections were being > closed but then considered abandoned. This is a legit race that abort > will generally handle fine and I don't see the need to throw. > > Phil > > > Sounds good. > > > > Gary > > > >> Phil > >> > >>> Gary > >>> > >>> On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory > wrote: > FWIW, I like the name "DestroyMode" because it matches the "destroy" > in the method name. > > Gary > > On Mon, Sep 7, 2020, 19:08 Gary Gregory > wrote: > > On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz > wrote: > >> On 9/3/20 2:44 AM, Mark Thomas wrote: > >>> On 31/08/2020 01:05, Phil Steitz wrote: > >>> > >>> > >>> > If others agree it is a good idea for dbcp, I can do it. I can > see the > argument that its better to stay with close() even for abandoned > and I > have not been able to get the deadlock to happen, so I would like > to > wait a bit and allow others to weigh in. Similarly for pool, I > would > like to get more community feedback before adding to the > interface. > >>> Hmm. > >>> > >>> On one hand if the driver deadlocks I don't see how that can be > anything > >>> other than a driver bug. If multiple code paths obtain multiple > locks > >>> then those code paths must always obtain those locks in the same > order. > >>> Anything else is a bug that is likely to result in a deadlock in a > >>> multithreaded environment. > >>> > >>> On the other hand, it could be argued that the situation only > arises > >>> when an application doesn't correctly return connections to the > pool > >>> and/or keeps them for too long and/or doesn't configure the pool > >>> correctly for their usage pattern. > >>> > >>> The approach of adding > >>> > >>> PooledObjectFactory.destroyObject(PooledObject,CloseMode) > >>> > >>> where CloseMode is an Enum with two values looks reasonable to me. > >> I have started to work on the [pool] changes for this. I want to > check > >> two things before completing the PR: > >> > >> 1. "Close" is a [dbcp] concept which does not make sense for all > pool > >> factories, so I am going to name the enum "DestroyMode" and the two > >> modes, "Default" and "Abandoned". That leaves room for other modes > like > >> "Evicted" or "Invalid" later. > >> > >> 2. Speaking of later, technically adding modes will not break binary > >> compatibility. Are we going to be OK adding outside a major > release? > >> If the answer is no, I might argue to include the other natural > modes now. > > Yes, IMO, it is OK to add enum values in a minor release since it > does > > not break binary (or source) compatibility. > > > > Gary > > > >> Phil > >> > >>> I do agree that abort() should only be used in the case of > abandoned > >>> connections. > >>> > >>> Mark > >>> > >>> > - > >>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > >>> For additional commands, e-mail: dev-h...@commons.apache.org > >>> > >> >
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 23/09/2020 01:29, Phil Steitz wrote: > > On 9/14/20 10:10 AM, Gary Gregory wrote: >> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz >> wrote: >>> >>> On 9/14/20 9:36 AM, Gary Gregory wrote: This feature is now in Pool master. I will prepare an RC soon if you all think we are good to go so we can then move on to DBCP. >>> I am still working on testing this in the DBCP use case. Probably best >>> to wait a little for others to review and for me to get the DBCP change >>> tested against current pool sources. I should be able to finish that >>> this weekend. > > I implemented changes in DBCP, based on recently committed changes in > pool. I made a few decisions that I would appreciate feedback on. > > 1. The JDBC abort method requires an Executor as actual parameter. I > added a FixedThreadPool executor with a max of 3 threads to > PoolableConnectionFactory for this purpose. Given that this operation > might hang sometimes, I thought it best to allow more than a single > thread. I guess it could be configurable, but 3 seemed a reasonable > choice. I copied the custom thread factory used by pool's EvictionTimer > to source daemon threads based on ccl for this. I then added a close() > method to PCF that closes the executor and modified BasicDataSource > close to call this. > > 2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF > destroyObject when abandoned mode is passed in rather than just > getObject().abort as destroy invokes close (I wonder if that is a bug?) > > 3. I changed JdbcBridge#abort to remove the checkOpen() check. I was > getting exceptions in my concurrency tests where connections were being > closed but then considered abandoned. This is a legit race that abort > will generally handle fine and I don't see the need to throw. All looks reasonable to me. Mark - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 9/14/20 10:10 AM, Gary Gregory wrote: On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz wrote: On 9/14/20 9:36 AM, Gary Gregory wrote: This feature is now in Pool master. I will prepare an RC soon if you all think we are good to go so we can then move on to DBCP. I am still working on testing this in the DBCP use case. Probably best to wait a little for others to review and for me to get the DBCP change tested against current pool sources. I should be able to finish that this weekend. I implemented changes in DBCP, based on recently committed changes in pool. I made a few decisions that I would appreciate feedback on. 1. The JDBC abort method requires an Executor as actual parameter. I added a FixedThreadPool executor with a max of 3 threads to PoolableConnectionFactory for this purpose. Given that this operation might hang sometimes, I thought it best to allow more than a single thread. I guess it could be configurable, but 3 seemed a reasonable choice. I copied the custom thread factory used by pool's EvictionTimer to source daemon threads based on ccl for this. I then added a close() method to PCF that closes the executor and modified BasicDataSource close to call this. 2. I used p.getObject().getInnermostDelegate().abort(executor) in PCF destroyObject when abandoned mode is passed in rather than just getObject().abort as destroy invokes close (I wonder if that is a bug?) 3. I changed JdbcBridge#abort to remove the checkOpen() check. I was getting exceptions in my concurrency tests where connections were being closed but then considered abandoned. This is a legit race that abort will generally handle fine and I don't see the need to throw. Phil Sounds good. Gary Phil Gary On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory wrote: FWIW, I like the name "DestroyMode" because it matches the "destroy" in the method name. Gary On Mon, Sep 7, 2020, 19:08 Gary Gregory wrote: On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: On 9/3/20 2:44 AM, Mark Thomas wrote: On 31/08/2020 01:05, Phil Steitz wrote: If others agree it is a good idea for dbcp, I can do it. I can see the argument that its better to stay with close() even for abandoned and I have not been able to get the deadlock to happen, so I would like to wait a bit and allow others to weigh in. Similarly for pool, I would like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I have started to work on the [pool] changes for this. I want to check two things before completing the PR: 1. "Close" is a [dbcp] concept which does not make sense for all pool factories, so I am going to name the enum "DestroyMode" and the two modes, "Default" and "Abandoned". That leaves room for other modes like "Evicted" or "Invalid" later. 2. Speaking of later, technically adding modes will not break binary compatibility. Are we going to be OK adding outside a major release? If the answer is no, I might argue to include the other natural modes now. Yes, IMO, it is OK to add enum values in a minor release since it does not break binary (or source) compatibility. Gary Phil I do agree that abort() should only be used in the case of abandoned connections. 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 - 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: [dbcp][pool] Use abort instead of close for abandoned connections?
On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz wrote: > > > On 9/14/20 9:36 AM, Gary Gregory wrote: > > This feature is now in Pool master. I will prepare an RC soon if you > > all think we are good to go so we can then move on to DBCP. > > I am still working on testing this in the DBCP use case. Probably best > to wait a little for others to review and for me to get the DBCP change > tested against current pool sources. I should be able to finish that > this weekend. Sounds good. Gary > > Phil > > > > > Gary > > > > On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory wrote: > >> FWIW, I like the name "DestroyMode" because it matches the "destroy" in > >> the method name. > >> > >> Gary > >> > >> On Mon, Sep 7, 2020, 19:08 Gary Gregory wrote: > >>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: > > On 9/3/20 2:44 AM, Mark Thomas wrote: > > On 31/08/2020 01:05, Phil Steitz wrote: > > > > > > > >> If others agree it is a good idea for dbcp, I can do it. I can see the > >> argument that its better to stay with close() even for abandoned and I > >> have not been able to get the deadlock to happen, so I would like to > >> wait a bit and allow others to weigh in. Similarly for pool, I would > >> like to get more community feedback before adding to the interface. > > Hmm. > > > > On one hand if the driver deadlocks I don't see how that can be anything > > other than a driver bug. If multiple code paths obtain multiple locks > > then those code paths must always obtain those locks in the same order. > > Anything else is a bug that is likely to result in a deadlock in a > > multithreaded environment. > > > > On the other hand, it could be argued that the situation only arises > > when an application doesn't correctly return connections to the pool > > and/or keeps them for too long and/or doesn't configure the pool > > correctly for their usage pattern. > > > > The approach of adding > > > > PooledObjectFactory.destroyObject(PooledObject,CloseMode) > > > > where CloseMode is an Enum with two values looks reasonable to me. > > I have started to work on the [pool] changes for this. I want to check > two things before completing the PR: > > 1. "Close" is a [dbcp] concept which does not make sense for all pool > factories, so I am going to name the enum "DestroyMode" and the two > modes, "Default" and "Abandoned". That leaves room for other modes like > "Evicted" or "Invalid" later. > > 2. Speaking of later, technically adding modes will not break binary > compatibility. Are we going to be OK adding outside a major release? > If the answer is no, I might argue to include the other natural modes > now. > >>> Yes, IMO, it is OK to add enum values in a minor release since it does > >>> not break binary (or source) compatibility. > >>> > >>> Gary > >>> > Phil > > > I do agree that abort() should only be used in the case of abandoned > > connections. > > > > 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 > > > - > > 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: [dbcp][pool] Use abort instead of close for abandoned connections?
On 9/14/20 9:36 AM, Gary Gregory wrote: This feature is now in Pool master. I will prepare an RC soon if you all think we are good to go so we can then move on to DBCP. I am still working on testing this in the DBCP use case. Probably best to wait a little for others to review and for me to get the DBCP change tested against current pool sources. I should be able to finish that this weekend. Phil Gary On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory wrote: FWIW, I like the name "DestroyMode" because it matches the "destroy" in the method name. Gary On Mon, Sep 7, 2020, 19:08 Gary Gregory wrote: On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: On 9/3/20 2:44 AM, Mark Thomas wrote: On 31/08/2020 01:05, Phil Steitz wrote: If others agree it is a good idea for dbcp, I can do it. I can see the argument that its better to stay with close() even for abandoned and I have not been able to get the deadlock to happen, so I would like to wait a bit and allow others to weigh in. Similarly for pool, I would like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I have started to work on the [pool] changes for this. I want to check two things before completing the PR: 1. "Close" is a [dbcp] concept which does not make sense for all pool factories, so I am going to name the enum "DestroyMode" and the two modes, "Default" and "Abandoned". That leaves room for other modes like "Evicted" or "Invalid" later. 2. Speaking of later, technically adding modes will not break binary compatibility. Are we going to be OK adding outside a major release? If the answer is no, I might argue to include the other natural modes now. Yes, IMO, it is OK to add enum values in a minor release since it does not break binary (or source) compatibility. Gary Phil I do agree that abort() should only be used in the case of abandoned connections. 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 - 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: [dbcp][pool] Use abort instead of close for abandoned connections?
This feature is now in Pool master. I will prepare an RC soon if you all think we are good to go so we can then move on to DBCP. Gary On Tue, Sep 8, 2020 at 9:16 AM Gary Gregory wrote: > > FWIW, I like the name "DestroyMode" because it matches the "destroy" in the > method name. > > Gary > > On Mon, Sep 7, 2020, 19:08 Gary Gregory wrote: >> >> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: >> > >> > >> > On 9/3/20 2:44 AM, Mark Thomas wrote: >> > > On 31/08/2020 01:05, Phil Steitz wrote: >> > > >> > > >> > > >> > >> If others agree it is a good idea for dbcp, I can do it. I can see the >> > >> argument that its better to stay with close() even for abandoned and I >> > >> have not been able to get the deadlock to happen, so I would like to >> > >> wait a bit and allow others to weigh in. Similarly for pool, I would >> > >> like to get more community feedback before adding to the interface. >> > > Hmm. >> > > >> > > On one hand if the driver deadlocks I don't see how that can be anything >> > > other than a driver bug. If multiple code paths obtain multiple locks >> > > then those code paths must always obtain those locks in the same order. >> > > Anything else is a bug that is likely to result in a deadlock in a >> > > multithreaded environment. >> > > >> > > On the other hand, it could be argued that the situation only arises >> > > when an application doesn't correctly return connections to the pool >> > > and/or keeps them for too long and/or doesn't configure the pool >> > > correctly for their usage pattern. >> > > >> > > The approach of adding >> > > >> > > PooledObjectFactory.destroyObject(PooledObject,CloseMode) >> > > >> > > where CloseMode is an Enum with two values looks reasonable to me. >> > >> > >> > I have started to work on the [pool] changes for this. I want to check >> > two things before completing the PR: >> > >> > 1. "Close" is a [dbcp] concept which does not make sense for all pool >> > factories, so I am going to name the enum "DestroyMode" and the two >> > modes, "Default" and "Abandoned". That leaves room for other modes like >> > "Evicted" or "Invalid" later. >> > >> > 2. Speaking of later, technically adding modes will not break binary >> > compatibility. Are we going to be OK adding outside a major release? >> > If the answer is no, I might argue to include the other natural modes now. >> >> Yes, IMO, it is OK to add enum values in a minor release since it does >> not break binary (or source) compatibility. >> >> Gary >> >> > >> > Phil >> > >> > > >> > > I do agree that abort() should only be used in the case of abandoned >> > > connections. >> > > >> > > 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 >> > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
FWIW, I like the name "DestroyMode" because it matches the "destroy" in the method name. Gary On Mon, Sep 7, 2020, 19:08 Gary Gregory wrote: > On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: > > > > > > On 9/3/20 2:44 AM, Mark Thomas wrote: > > > On 31/08/2020 01:05, Phil Steitz wrote: > > > > > > > > > > > >> If others agree it is a good idea for dbcp, I can do it. I can see > the > > >> argument that its better to stay with close() even for abandoned and I > > >> have not been able to get the deadlock to happen, so I would like to > > >> wait a bit and allow others to weigh in. Similarly for pool, I would > > >> like to get more community feedback before adding to the interface. > > > Hmm. > > > > > > On one hand if the driver deadlocks I don't see how that can be > anything > > > other than a driver bug. If multiple code paths obtain multiple locks > > > then those code paths must always obtain those locks in the same order. > > > Anything else is a bug that is likely to result in a deadlock in a > > > multithreaded environment. > > > > > > On the other hand, it could be argued that the situation only arises > > > when an application doesn't correctly return connections to the pool > > > and/or keeps them for too long and/or doesn't configure the pool > > > correctly for their usage pattern. > > > > > > The approach of adding > > > > > > PooledObjectFactory.destroyObject(PooledObject,CloseMode) > > > > > > where CloseMode is an Enum with two values looks reasonable to me. > > > > > > I have started to work on the [pool] changes for this. I want to check > > two things before completing the PR: > > > > 1. "Close" is a [dbcp] concept which does not make sense for all pool > > factories, so I am going to name the enum "DestroyMode" and the two > > modes, "Default" and "Abandoned". That leaves room for other modes like > > "Evicted" or "Invalid" later. > > > > 2. Speaking of later, technically adding modes will not break binary > > compatibility. Are we going to be OK adding outside a major release? > > If the answer is no, I might argue to include the other natural modes > now. > > Yes, IMO, it is OK to add enum values in a minor release since it does > not break binary (or source) compatibility. > > Gary > > > > > Phil > > > > > > > > I do agree that abort() should only be used in the case of abandoned > > > connections. > > > > > > 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 > > >
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz wrote: > > > On 9/3/20 2:44 AM, Mark Thomas wrote: > > On 31/08/2020 01:05, Phil Steitz wrote: > > > > > > > >> If others agree it is a good idea for dbcp, I can do it. I can see the > >> argument that its better to stay with close() even for abandoned and I > >> have not been able to get the deadlock to happen, so I would like to > >> wait a bit and allow others to weigh in. Similarly for pool, I would > >> like to get more community feedback before adding to the interface. > > Hmm. > > > > On one hand if the driver deadlocks I don't see how that can be anything > > other than a driver bug. If multiple code paths obtain multiple locks > > then those code paths must always obtain those locks in the same order. > > Anything else is a bug that is likely to result in a deadlock in a > > multithreaded environment. > > > > On the other hand, it could be argued that the situation only arises > > when an application doesn't correctly return connections to the pool > > and/or keeps them for too long and/or doesn't configure the pool > > correctly for their usage pattern. > > > > The approach of adding > > > > PooledObjectFactory.destroyObject(PooledObject,CloseMode) > > > > where CloseMode is an Enum with two values looks reasonable to me. > > > I have started to work on the [pool] changes for this. I want to check > two things before completing the PR: > > 1. "Close" is a [dbcp] concept which does not make sense for all pool > factories, so I am going to name the enum "DestroyMode" and the two > modes, "Default" and "Abandoned". That leaves room for other modes like > "Evicted" or "Invalid" later. > > 2. Speaking of later, technically adding modes will not break binary > compatibility. Are we going to be OK adding outside a major release? > If the answer is no, I might argue to include the other natural modes now. Yes, IMO, it is OK to add enum values in a minor release since it does not break binary (or source) compatibility. Gary > > Phil > > > > > I do agree that abort() should only be used in the case of abandoned > > connections. > > > > 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 > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 9/3/20 2:44 AM, Mark Thomas wrote: On 31/08/2020 01:05, Phil Steitz wrote: If others agree it is a good idea for dbcp, I can do it. I can see the argument that its better to stay with close() even for abandoned and I have not been able to get the deadlock to happen, so I would like to wait a bit and allow others to weigh in. Similarly for pool, I would like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I have started to work on the [pool] changes for this. I want to check two things before completing the PR: 1. "Close" is a [dbcp] concept which does not make sense for all pool factories, so I am going to name the enum "DestroyMode" and the two modes, "Default" and "Abandoned". That leaves room for other modes like "Evicted" or "Invalid" later. 2. Speaking of later, technically adding modes will not break binary compatibility. Are we going to be OK adding outside a major release? If the answer is no, I might argue to include the other natural modes now. Phil I do agree that abort() should only be used in the case of abandoned connections. 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
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
The two cases i seen the abort() helped, since it closes the socket without synchronize and the socket close will unblock the read. But you need to be careful not to call any other blocking method first (i remeber rollback, Statement close and even isOpen (if i recall correctly) have been problematic) Gruss Bernd -- http://bernd.eckenfels.net Von: Phil Steitz Gesendet: Friday, September 4, 2020 1:06:23 AM An: dev@commons.apache.org Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections? On 9/3/20 3:02 PM, Bernd Eckenfels wrote: > The issues I have seen are not a/b "deadlocks", they are "just" endless locks > - the close is waiting for a read to finish (since both synchronize on the > connection). If the asumption is a pool timer can in the background cancel a > read - it can't, at least on Oracle thin or jtds. > > The cause can be softened by using read timeouts and/or keepalive, but both > is not the default and can block the pool for minutes, still. The question is would using abort instead of close work better in some of the abandoned connection scenarios. IIUC the contract correctly, at least the pool thread will not be blocked in these scenarios if we move to this for that use case. Note that dbcp/pool can be configured to remove abandoned connections on borrow as well as maintenance, so blocking in the former case blocks the pool client. Phil > > Gruss > Bernd > -- > http://bernd.eckenfels.net > > Von: Mark Thomas > Gesendet: Thursday, September 3, 2020 11:44:52 AM > An: dev@commons.apache.org > Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned > connections? > > On 31/08/2020 01:05, Phil Steitz wrote: > > > >> If others agree it is a good idea for dbcp, I can do it. I can see the >> argument that its better to stay with close() even for abandoned and I >> have not been able to get the deadlock to happen, so I would like to >> wait a bit and allow others to weigh in. Similarly for pool, I would >> like to get more community feedback before adding to the interface. > Hmm. > > On one hand if the driver deadlocks I don't see how that can be anything > other than a driver bug. If multiple code paths obtain multiple locks > then those code paths must always obtain those locks in the same order. > Anything else is a bug that is likely to result in a deadlock in a > multithreaded environment. > > On the other hand, it could be argued that the situation only arises > when an application doesn't correctly return connections to the pool > and/or keeps them for too long and/or doesn't configure the pool > correctly for their usage pattern. > > The approach of adding > > PooledObjectFactory.destroyObject(PooledObject,CloseMode) > > where CloseMode is an Enum with two values looks reasonable to me. > > I do agree that abort() should only be used in the case of abandoned > connections. > > 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
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 9/3/20 3:02 PM, Bernd Eckenfels wrote: The issues I have seen are not a/b "deadlocks", they are "just" endless locks - the close is waiting for a read to finish (since both synchronize on the connection). If the asumption is a pool timer can in the background cancel a read - it can't, at least on Oracle thin or jtds. The cause can be softened by using read timeouts and/or keepalive, but both is not the default and can block the pool for minutes, still. The question is would using abort instead of close work better in some of the abandoned connection scenarios. IIUC the contract correctly, at least the pool thread will not be blocked in these scenarios if we move to this for that use case. Note that dbcp/pool can be configured to remove abandoned connections on borrow as well as maintenance, so blocking in the former case blocks the pool client. Phil Gruss Bernd -- http://bernd.eckenfels.net Von: Mark Thomas Gesendet: Thursday, September 3, 2020 11:44:52 AM An: dev@commons.apache.org Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections? On 31/08/2020 01:05, Phil Steitz wrote: If others agree it is a good idea for dbcp, I can do it. I can see the argument that its better to stay with close() even for abandoned and I have not been able to get the deadlock to happen, so I would like to wait a bit and allow others to weigh in. Similarly for pool, I would like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I do agree that abort() should only be used in the case of abandoned connections. 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
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
The issues I have seen are not a/b "deadlocks", they are "just" endless locks - the close is waiting for a read to finish (since both synchronize on the connection). If the asumption is a pool timer can in the background cancel a read - it can't, at least on Oracle thin or jtds. The cause can be softened by using read timeouts and/or keepalive, but both is not the default and can block the pool for minutes, still. Gruss Bernd -- http://bernd.eckenfels.net Von: Mark Thomas Gesendet: Thursday, September 3, 2020 11:44:52 AM An: dev@commons.apache.org Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections? On 31/08/2020 01:05, Phil Steitz wrote: > If others agree it is a good idea for dbcp, I can do it. I can see the > argument that its better to stay with close() even for abandoned and I > have not been able to get the deadlock to happen, so I would like to > wait a bit and allow others to weigh in. Similarly for pool, I would > like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I do agree that abort() should only be used in the case of abandoned connections. Mark - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 31/08/2020 01:05, Phil Steitz wrote: > If others agree it is a good idea for dbcp, I can do it. I can see the > argument that its better to stay with close() even for abandoned and I > have not been able to get the deadlock to happen, so I would like to > wait a bit and allow others to weigh in. Similarly for pool, I would > like to get more community feedback before adding to the interface. Hmm. On one hand if the driver deadlocks I don't see how that can be anything other than a driver bug. If multiple code paths obtain multiple locks then those code paths must always obtain those locks in the same order. Anything else is a bug that is likely to result in a deadlock in a multithreaded environment. On the other hand, it could be argued that the situation only arises when an application doesn't correctly return connections to the pool and/or keeps them for too long and/or doesn't configure the pool correctly for their usage pattern. The approach of adding PooledObjectFactory.destroyObject(PooledObject,CloseMode) where CloseMode is an Enum with two values looks reasonable to me. I do agree that abort() should only be used in the case of abandoned connections. Mark - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 8/30/20 4:00 PM, Gary Gregory wrote: On Sun, Aug 30, 2020 at 2:30 PM Phil Steitz wrote: On 8/30/20 9:22 AM, Gary Gregory wrote: Hm... would we need the flexibility of passing custom enums? For example, CloseMode could be an interface implemented by various enums in the style of the JRE's NIO public enum StandardOpenOption implements OpenOption? We could have: PooledObjectFactory.destroyObject(PooledObject, DestroyOption*) Remember this is only called by the pool, so I don't see the need for custom enums, but it would be good to be able to add enum values so as you suggested we start with the simplest but then add more granularity as needed. Phil, Do you plan on providing this feature? I'm a bit busy with other components and work ATM (aren't we all!) If others agree it is a good idea for dbcp, I can do it. I can see the argument that its better to stay with close() even for abandoned and I have not been able to get the deadlock to happen, so I would like to wait a bit and allow others to weigh in. Similarly for pool, I would like to get more community feedback before adding to the interface. Phil Gary Phil Where DestroyOption is an empty interface and we provide a StandardDestroyOption enum that implements DestroyOption. ? Gary On Sun, Aug 30, 2020 at 11:49 AM Gary Gregory wrote: So concretely, we would add org.apache.commons.pool2.PooledObjectFactory.destroyObject(PooledObject, CloseMode*) as a default method. [*] New enum ? Gary On Sat, Aug 29, 2020 at 4:02 PM Gary Gregory wrote: On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: On 8/29/20 11:03 AM, Gary Gregory wrote: On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz wrote: A pool-related deadlock was reported recently in [1] to tomcat-user. The OP was using a different pool, but it looks to me like the same deadlock could happen with dbcp. The source is arguably a driver bug, but in [2], the driver maintainer makes the good point that to avoid the problem in [1] (and others like it) it would be better if pool maintainers used abort instead of close to disconnect "stuck" or abandoned connections. That makes sense to me. To implement this in dbcp, we would need to either always use abort instead of close when closing physical connections (a bad idea, IMO, as it could mess up counters due to async execution and add overhead) I agree that changing the happy path from close() to abort() is a bad idea; especially since the Connection#close() Javadoc contract releases database resources while abort() does not or is vague about it (at least in the Java 8 Javadoc). Yes, bad idea to do that, I think. or modify pool to call a different factory method than destroy when triggering close-abandoned. I think the latter makes sense and it could be useful for other pool clients to be able to this kind of thing as well. This means that we would treat abandoned connections like a real resource leak in the sense that (if) abort() leaks database resources, then the DBA would have to deal with that. Yeah, unless the driver does it cleanly. The other thing to note here is that abort is async and is supposed to allow ops in progress to complete, etc. That is kinder to clients but technically allows maxActive to exceed the cap while the abort is in progress. I think formally surfacing an abort path vs. the current close/destroy is OK since it reflects what JDBC allows. If that means trickling the concept down to Pool, then that's OK as well IMO. This could be generically surfaced with a "close mode" enum passed to close/destroy APIs. This is similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html Exactly. I think the abandoned-destroy is legitimately different from the other destroy use cases and I could see other kinds of pools (sockets, jms, etc) benefiting from having the ability to handle destroy differently for abandoned (or other) cases. What I propose is that we make these change to pool: 1. Add a new method to PooledObjectFactory called "abandonObject" with default implementation delegating to destroyObject. That or overload destroyObject with a destroyObject(CloseMode) would be easier to find. Yes, I think that would be better. That raises the interesting question of what are the CloseModes. All we need for this issue is normal vs abandoned, but I can see value in providing fuller context to factories. So if we are doing it, why not something like: invalidated abandoned evicted excess idle failed validation failed activation failed passivation clearing pool I think a YAGNI-style approach would work well here by starting with 2 modes: one to support the current behavior (default) and another to support the new behavior backed by Connection#abort() in DBCP. A third "escape hatch" mode could see passing in a lamba I suppose.
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On Sun, Aug 30, 2020 at 2:30 PM Phil Steitz wrote: > > On 8/30/20 9:22 AM, Gary Gregory wrote: > > Hm... would we need the flexibility of passing custom enums? For example, > > CloseMode could be an interface implemented by various enums in the style > > of the JRE's NIO public enum StandardOpenOption implements OpenOption? > > We could have: > > PooledObjectFactory.destroyObject(PooledObject, DestroyOption*) > > Remember this is only called by the pool, so I don't see the need for > custom enums, but it would be good to be able to add enum values so as > you suggested we start with the simplest but then add more granularity > as needed. > Phil, Do you plan on providing this feature? I'm a bit busy with other components and work ATM (aren't we all!) Gary > > Phil > > > Where DestroyOption is an empty interface and we provide a > > StandardDestroyOption enum that implements DestroyOption. > > ? > > Gary > > > > On Sun, Aug 30, 2020 at 11:49 AM Gary Gregory > > wrote: > > > >> So concretely, we would > >> add > org.apache.commons.pool2.PooledObjectFactory.destroyObject(PooledObject, > >> CloseMode*) as a default method. > >> [*] New enum > >> > >> ? > >> > >> Gary > >> > >> > >> On Sat, Aug 29, 2020 at 4:02 PM Gary Gregory > >> wrote: > >> > >>> On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz > >>> wrote: > >>> > On 8/29/20 11:03 AM, Gary Gregory wrote: > > On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz > wrote: > >> A pool-related deadlock was reported recently in [1] to tomcat-user. > >> The OP was using a different pool, but it looks to me like the same > >> deadlock could happen with dbcp. The source is arguably a driver > bug, > >> but in [2], the driver maintainer makes the good point that to avoid > the > >> problem in [1] (and others like it) it would be better if pool > >> maintainers used abort instead of close to disconnect "stuck" or > >> abandoned connections. That makes sense to me. > >> > >> To implement this in dbcp, we would need to either always use abort > >> instead of close when closing physical connections (a bad idea, IMO, > as > >> it could mess up counters due to async execution and add overhead) > > I agree that changing the happy path from close() to abort() is a bad > idea; > > especially since the Connection#close() Javadoc contract releases > database > > resources while abort() does not or is vague about it (at least in > the > Java > > 8 Javadoc). > Yes, bad idea to do that, I think. > > > > >> or > >> modify pool to call a different factory method than destroy when > >> triggering close-abandoned. I think the latter makes sense and it > could > >> be useful for other pool clients to be able to this kind of thing as > well. > > This means that we would treat abandoned connections like a real > resource > > leak in the sense that (if) abort() leaks database resources, then > the > DBA > > would have to deal with that. > Yeah, unless the driver does it cleanly. The other thing to note here > is that abort is async and is supposed to allow ops in progress to > complete, etc. That is kinder to clients but technically allows > maxActive to exceed the cap while the abort is in progress. > > > I think formally surfacing an abort path vs. the current > close/destroy > is > > OK since it reflects what JDBC allows. If that means trickling the > concept > > down to Pool, then that's OK as well IMO. This could be generically > > surfaced with a "close mode" enum passed to close/destroy APIs. This > is > > similar to what we did in HttpCore's CloseMode(IMMEDIATE vs > GRACEFUL): > > > > https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html > > Exactly. I think the abandoned-destroy is legitimately different from > the other destroy use cases and I could see other kinds of pools > (sockets, jms, etc) benefiting from having the ability to handle > destroy > differently for abandoned (or other) cases. > > > > >> What I propose is that we make these change to pool: > >> > >>1. Add a new method to PooledObjectFactory called "abandonObject" > with > >> default implementation delegating to destroyObject. > >> > > That or overload destroyObject with a destroyObject(CloseMode) would > be > > easier to find. > Yes, I think that would be better. That raises the interesting > question > of what are the CloseModes. All we need for this issue is normal vs > abandoned, but I can see value in providing fuller context to > factories. So if we are doing it, why not something like: > > invalidated > abandoned > evicted > excess idle > failed validation > failed activation > failed
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Hello, A common case we saw are missing keep alive with the oracle driver and a hanging read (due to destroyed connection state of firewall) or vip failover of sql server. Besides close we also tried rollback which is also affected of most drivers synchronized blocks. -- https://Bernd.eckenfels.net Von: Phil Steitz Gesendet: Sunday, August 30, 2020 8:27:04 PM An: dev@commons.apache.org Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections? On 8/29/20 9:58 PM, Bernd Eckenfels wrote: > We have a pool implementation where we had to call abort especially for > revoked long running transactions, since they regularly have been locked up > in a connection wide synchronized in the close. However we only use it for > that case and try to close the objects after the abort anyway. We don’t use > it to close connections when they are only idle or too old - owned by the > pool. Right. I was thinking only of the abandoned case. Interestingly, I have not been able to replicate the deadlock that started me thinking about this. I have always worried though about problems with close hanging or deadlocking in the abandoned use case where what is going on is a transaction is stuck or the abandoned timeout is just set too low. Phil > > It seems to be however still a good idea to run the abort/close in a separate > task as it can also take ages for example when TLS close handshakes hang (we > did try to reduce the connection timeout before but that also can be blocked > before the abort) > > Gruss > Bernd > -- > https://Bernd.eckenfels.net > > Von: Gary Gregory > Gesendet: Saturday, August 29, 2020 10:02:42 PM > An: Commons Developers List > Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned > connections? > > On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: > >> On 8/29/20 11:03 AM, Gary Gregory wrote: >>> On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz >> wrote: >>>> A pool-related deadlock was reported recently in [1] to tomcat-user. >>>> The OP was using a different pool, but it looks to me like the same >>>> deadlock could happen with dbcp. The source is arguably a driver bug, >>>> but in [2], the driver maintainer makes the good point that to avoid the >>>> problem in [1] (and others like it) it would be better if pool >>>> maintainers used abort instead of close to disconnect "stuck" or >>>> abandoned connections. That makes sense to me. >>>> >>>> To implement this in dbcp, we would need to either always use abort >>>> instead of close when closing physical connections (a bad idea, IMO, as >>>> it could mess up counters due to async execution and add overhead) >>> I agree that changing the happy path from close() to abort() is a bad >> idea; >>> especially since the Connection#close() Javadoc contract releases >> database >>> resources while abort() does not or is vague about it (at least in the >> Java >>> 8 Javadoc). >> Yes, bad idea to do that, I think. >> >>> >>>> or >>>> modify pool to call a different factory method than destroy when >>>> triggering close-abandoned. I think the latter makes sense and it could >>>> be useful for other pool clients to be able to this kind of thing as >> well. >>> This means that we would treat abandoned connections like a real resource >>> leak in the sense that (if) abort() leaks database resources, then the >> DBA >>> would have to deal with that. >> Yeah, unless the driver does it cleanly. The other thing to note here >> is that abort is async and is supposed to allow ops in progress to >> complete, etc. That is kinder to clients but technically allows >> maxActive to exceed the cap while the abort is in progress. >> >>> I think formally surfacing an abort path vs. the current close/destroy is >>> OK since it reflects what JDBC allows. If that means trickling the >> concept >>> down to Pool, then that's OK as well IMO. This could be generically >>> surfaced with a "close mode" enum passed to close/destroy APIs. This is >>> similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): >>> >> https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html >> >> Exactly. I think the abandoned-destroy is legitimately different from >> the other destroy use cases and I could see other kinds of pools >> (sockets, jms, etc) benefiting from having the ability to handle destr
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 8/30/20 9:22 AM, Gary Gregory wrote: Hm... would we need the flexibility of passing custom enums? For example, CloseMode could be an interface implemented by various enums in the style of the JRE's NIO public enum StandardOpenOption implements OpenOption? We could have: PooledObjectFactory.destroyObject(PooledObject, DestroyOption*) Remember this is only called by the pool, so I don't see the need for custom enums, but it would be good to be able to add enum values so as you suggested we start with the simplest but then add more granularity as needed. Phil Where DestroyOption is an empty interface and we provide a StandardDestroyOption enum that implements DestroyOption. ? Gary On Sun, Aug 30, 2020 at 11:49 AM Gary Gregory wrote: So concretely, we would add org.apache.commons.pool2.PooledObjectFactory.destroyObject(PooledObject, CloseMode*) as a default method. [*] New enum ? Gary On Sat, Aug 29, 2020 at 4:02 PM Gary Gregory wrote: On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: On 8/29/20 11:03 AM, Gary Gregory wrote: On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz wrote: A pool-related deadlock was reported recently in [1] to tomcat-user. The OP was using a different pool, but it looks to me like the same deadlock could happen with dbcp. The source is arguably a driver bug, but in [2], the driver maintainer makes the good point that to avoid the problem in [1] (and others like it) it would be better if pool maintainers used abort instead of close to disconnect "stuck" or abandoned connections. That makes sense to me. To implement this in dbcp, we would need to either always use abort instead of close when closing physical connections (a bad idea, IMO, as it could mess up counters due to async execution and add overhead) I agree that changing the happy path from close() to abort() is a bad idea; especially since the Connection#close() Javadoc contract releases database resources while abort() does not or is vague about it (at least in the Java 8 Javadoc). Yes, bad idea to do that, I think. or modify pool to call a different factory method than destroy when triggering close-abandoned. I think the latter makes sense and it could be useful for other pool clients to be able to this kind of thing as well. This means that we would treat abandoned connections like a real resource leak in the sense that (if) abort() leaks database resources, then the DBA would have to deal with that. Yeah, unless the driver does it cleanly. The other thing to note here is that abort is async and is supposed to allow ops in progress to complete, etc. That is kinder to clients but technically allows maxActive to exceed the cap while the abort is in progress. I think formally surfacing an abort path vs. the current close/destroy is OK since it reflects what JDBC allows. If that means trickling the concept down to Pool, then that's OK as well IMO. This could be generically surfaced with a "close mode" enum passed to close/destroy APIs. This is similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html Exactly. I think the abandoned-destroy is legitimately different from the other destroy use cases and I could see other kinds of pools (sockets, jms, etc) benefiting from having the ability to handle destroy differently for abandoned (or other) cases. What I propose is that we make these change to pool: 1. Add a new method to PooledObjectFactory called "abandonObject" with default implementation delegating to destroyObject. That or overload destroyObject with a destroyObject(CloseMode) would be easier to find. Yes, I think that would be better. That raises the interesting question of what are the CloseModes. All we need for this issue is normal vs abandoned, but I can see value in providing fuller context to factories. So if we are doing it, why not something like: invalidated abandoned evicted excess idle failed validation failed activation failed passivation clearing pool I think a YAGNI-style approach would work well here by starting with 2 modes: one to support the current behavior (default) and another to support the new behavior backed by Connection#abort() in DBCP. A third "escape hatch" mode could see passing in a lamba I suppose. I really think we should start by just focusing on the one issue at hand before we enlarge the solution. 2c, Gary Phil Gary 2. Change the logic in removeAbandoned to call the new factory method instead of destroyObject And then in dbcp: * Add implementation of abandonObject in PoolableConnectionFactory to call abort. wdyt? Phil [1] https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E [2] https://bugs.mysql.com/bug.php?id=64954
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 8/29/20 9:58 PM, Bernd Eckenfels wrote: We have a pool implementation where we had to call abort especially for revoked long running transactions, since they regularly have been locked up in a connection wide synchronized in the close. However we only use it for that case and try to close the objects after the abort anyway. We don’t use it to close connections when they are only idle or too old - owned by the pool. Right. I was thinking only of the abandoned case. Interestingly, I have not been able to replicate the deadlock that started me thinking about this. I have always worried though about problems with close hanging or deadlocking in the abandoned use case where what is going on is a transaction is stuck or the abandoned timeout is just set too low. Phil It seems to be however still a good idea to run the abort/close in a separate task as it can also take ages for example when TLS close handshakes hang (we did try to reduce the connection timeout before but that also can be blocked before the abort) Gruss Bernd -- https://Bernd.eckenfels.net Von: Gary Gregory Gesendet: Saturday, August 29, 2020 10:02:42 PM An: Commons Developers List Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections? On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: On 8/29/20 11:03 AM, Gary Gregory wrote: On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz wrote: A pool-related deadlock was reported recently in [1] to tomcat-user. The OP was using a different pool, but it looks to me like the same deadlock could happen with dbcp. The source is arguably a driver bug, but in [2], the driver maintainer makes the good point that to avoid the problem in [1] (and others like it) it would be better if pool maintainers used abort instead of close to disconnect "stuck" or abandoned connections. That makes sense to me. To implement this in dbcp, we would need to either always use abort instead of close when closing physical connections (a bad idea, IMO, as it could mess up counters due to async execution and add overhead) I agree that changing the happy path from close() to abort() is a bad idea; especially since the Connection#close() Javadoc contract releases database resources while abort() does not or is vague about it (at least in the Java 8 Javadoc). Yes, bad idea to do that, I think. or modify pool to call a different factory method than destroy when triggering close-abandoned. I think the latter makes sense and it could be useful for other pool clients to be able to this kind of thing as well. This means that we would treat abandoned connections like a real resource leak in the sense that (if) abort() leaks database resources, then the DBA would have to deal with that. Yeah, unless the driver does it cleanly. The other thing to note here is that abort is async and is supposed to allow ops in progress to complete, etc. That is kinder to clients but technically allows maxActive to exceed the cap while the abort is in progress. I think formally surfacing an abort path vs. the current close/destroy is OK since it reflects what JDBC allows. If that means trickling the concept down to Pool, then that's OK as well IMO. This could be generically surfaced with a "close mode" enum passed to close/destroy APIs. This is similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html Exactly. I think the abandoned-destroy is legitimately different from the other destroy use cases and I could see other kinds of pools (sockets, jms, etc) benefiting from having the ability to handle destroy differently for abandoned (or other) cases. What I propose is that we make these change to pool: 1. Add a new method to PooledObjectFactory called "abandonObject" with default implementation delegating to destroyObject. That or overload destroyObject with a destroyObject(CloseMode) would be easier to find. Yes, I think that would be better. That raises the interesting question of what are the CloseModes. All we need for this issue is normal vs abandoned, but I can see value in providing fuller context to factories. So if we are doing it, why not something like: invalidated abandoned evicted excess idle failed validation failed activation failed passivation clearing pool I think a YAGNI-style approach would work well here by starting with 2 modes: one to support the current behavior (default) and another to support the new behavior backed by Connection#abort() in DBCP. A third "escape hatch" mode could see passing in a lamba I suppose. I really think we should start by just focusing on the one issue at hand before we enlarge the solution. 2c, Gary Phil Gary 2. Change the logic in removeAbandoned to call the new factory method instead of des
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Hm... would we need the flexibility of passing custom enums? For example, CloseMode could be an interface implemented by various enums in the style of the JRE's NIO public enum StandardOpenOption implements OpenOption? We could have: PooledObjectFactory.destroyObject(PooledObject, DestroyOption*) Where DestroyOption is an empty interface and we provide a StandardDestroyOption enum that implements DestroyOption. ? Gary On Sun, Aug 30, 2020 at 11:49 AM Gary Gregory wrote: > So concretely, we would > add org.apache.commons.pool2.PooledObjectFactory.destroyObject(PooledObject, > CloseMode*) as a default method. > [*] New enum > > ? > > Gary > > > On Sat, Aug 29, 2020 at 4:02 PM Gary Gregory > wrote: > >> On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz >> wrote: >> >>> >>> On 8/29/20 11:03 AM, Gary Gregory wrote: >>> > On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz >>> wrote: >>> > >>> >> A pool-related deadlock was reported recently in [1] to tomcat-user. >>> >> The OP was using a different pool, but it looks to me like the same >>> >> deadlock could happen with dbcp. The source is arguably a driver bug, >>> >> but in [2], the driver maintainer makes the good point that to avoid >>> the >>> >> problem in [1] (and others like it) it would be better if pool >>> >> maintainers used abort instead of close to disconnect "stuck" or >>> >> abandoned connections. That makes sense to me. >>> >> >>> >> To implement this in dbcp, we would need to either always use abort >>> >> instead of close when closing physical connections (a bad idea, IMO, >>> as >>> >> it could mess up counters due to async execution and add overhead) >>> > >>> > I agree that changing the happy path from close() to abort() is a bad >>> idea; >>> > especially since the Connection#close() Javadoc contract releases >>> database >>> > resources while abort() does not or is vague about it (at least in the >>> Java >>> > 8 Javadoc). >>> >>> Yes, bad idea to do that, I think. >>> >>> > >>> > >>> >> or >>> >> modify pool to call a different factory method than destroy when >>> >> triggering close-abandoned. I think the latter makes sense and it >>> could >>> >> be useful for other pool clients to be able to this kind of thing as >>> well. >>> >> >>> > This means that we would treat abandoned connections like a real >>> resource >>> > leak in the sense that (if) abort() leaks database resources, then the >>> DBA >>> > would have to deal with that. >>> >>> Yeah, unless the driver does it cleanly. The other thing to note here >>> is that abort is async and is supposed to allow ops in progress to >>> complete, etc. That is kinder to clients but technically allows >>> maxActive to exceed the cap while the abort is in progress. >>> >>> > >>> > I think formally surfacing an abort path vs. the current close/destroy >>> is >>> > OK since it reflects what JDBC allows. If that means trickling the >>> concept >>> > down to Pool, then that's OK as well IMO. This could be generically >>> > surfaced with a "close mode" enum passed to close/destroy APIs. This is >>> > similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): >>> > >>> https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html >>> >>> Exactly. I think the abandoned-destroy is legitimately different from >>> the other destroy use cases and I could see other kinds of pools >>> (sockets, jms, etc) benefiting from having the ability to handle destroy >>> differently for abandoned (or other) cases. >>> >>> > >>> > >>> >> What I propose is that we make these change to pool: >>> >> >>> >> 1. Add a new method to PooledObjectFactory called "abandonObject" >>> with >>> >> default implementation delegating to destroyObject. >>> >> >>> > That or overload destroyObject with a destroyObject(CloseMode) would be >>> > easier to find. >>> >>> Yes, I think that would be better. That raises the interesting question >>> of what are the CloseModes. All we need for this issue is normal vs >>> abandoned, but I can see value in providing fuller context to >>> factories. So if we are doing it, why not something like: >>> >>> invalidated >>> abandoned >>> evicted >>> excess idle >>> failed validation >>> failed activation >>> failed passivation >>> clearing pool >>> >> >> I think a YAGNI-style approach would work well here by starting with 2 >> modes: one to support the current behavior (default) and another to support >> the new behavior backed by Connection#abort() in DBCP. >> A third "escape hatch" mode could see passing in a lamba I suppose. I >> really think we should start by just focusing on the one issue at hand >> before we enlarge the solution. >> >> 2c, >> Gary >> >> >>> Phil >>> >>> >>> >>> > >>> > Gary >>> > >>> > >>> >> 2. Change the logic in removeAbandoned to call the new factory >>> method >>> >> instead of destroyObject >>> >> >>> >> And then in dbcp: >>> >> >>> >>* Add implementation of abandonObject in
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
So concretely, we would add org.apache.commons.pool2.PooledObjectFactory.destroyObject(PooledObject, CloseMode*) as a default method. [*] New enum ? Gary On Sat, Aug 29, 2020 at 4:02 PM Gary Gregory wrote: > On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: > >> >> On 8/29/20 11:03 AM, Gary Gregory wrote: >> > On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz >> wrote: >> > >> >> A pool-related deadlock was reported recently in [1] to tomcat-user. >> >> The OP was using a different pool, but it looks to me like the same >> >> deadlock could happen with dbcp. The source is arguably a driver bug, >> >> but in [2], the driver maintainer makes the good point that to avoid >> the >> >> problem in [1] (and others like it) it would be better if pool >> >> maintainers used abort instead of close to disconnect "stuck" or >> >> abandoned connections. That makes sense to me. >> >> >> >> To implement this in dbcp, we would need to either always use abort >> >> instead of close when closing physical connections (a bad idea, IMO, as >> >> it could mess up counters due to async execution and add overhead) >> > >> > I agree that changing the happy path from close() to abort() is a bad >> idea; >> > especially since the Connection#close() Javadoc contract releases >> database >> > resources while abort() does not or is vague about it (at least in the >> Java >> > 8 Javadoc). >> >> Yes, bad idea to do that, I think. >> >> > >> > >> >> or >> >> modify pool to call a different factory method than destroy when >> >> triggering close-abandoned. I think the latter makes sense and it >> could >> >> be useful for other pool clients to be able to this kind of thing as >> well. >> >> >> > This means that we would treat abandoned connections like a real >> resource >> > leak in the sense that (if) abort() leaks database resources, then the >> DBA >> > would have to deal with that. >> >> Yeah, unless the driver does it cleanly. The other thing to note here >> is that abort is async and is supposed to allow ops in progress to >> complete, etc. That is kinder to clients but technically allows >> maxActive to exceed the cap while the abort is in progress. >> >> > >> > I think formally surfacing an abort path vs. the current close/destroy >> is >> > OK since it reflects what JDBC allows. If that means trickling the >> concept >> > down to Pool, then that's OK as well IMO. This could be generically >> > surfaced with a "close mode" enum passed to close/destroy APIs. This is >> > similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): >> > >> https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html >> >> Exactly. I think the abandoned-destroy is legitimately different from >> the other destroy use cases and I could see other kinds of pools >> (sockets, jms, etc) benefiting from having the ability to handle destroy >> differently for abandoned (or other) cases. >> >> > >> > >> >> What I propose is that we make these change to pool: >> >> >> >> 1. Add a new method to PooledObjectFactory called "abandonObject" >> with >> >> default implementation delegating to destroyObject. >> >> >> > That or overload destroyObject with a destroyObject(CloseMode) would be >> > easier to find. >> >> Yes, I think that would be better. That raises the interesting question >> of what are the CloseModes. All we need for this issue is normal vs >> abandoned, but I can see value in providing fuller context to >> factories. So if we are doing it, why not something like: >> >> invalidated >> abandoned >> evicted >> excess idle >> failed validation >> failed activation >> failed passivation >> clearing pool >> > > I think a YAGNI-style approach would work well here by starting with 2 > modes: one to support the current behavior (default) and another to support > the new behavior backed by Connection#abort() in DBCP. > A third "escape hatch" mode could see passing in a lamba I suppose. I > really think we should start by just focusing on the one issue at hand > before we enlarge the solution. > > 2c, > Gary > > >> Phil >> >> >> >> > >> > Gary >> > >> > >> >> 2. Change the logic in removeAbandoned to call the new factory method >> >> instead of destroyObject >> >> >> >> And then in dbcp: >> >> >> >>* Add implementation of abandonObject in PoolableConnectionFactory >> to >> >> call abort. >> >> >> >> wdyt? >> >> >> >> Phil >> >> >> >> [1] >> >> >> >> >> https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E >> >> >> >> [2] https://bugs.mysql.com/bug.php?id=64954 >> >> >> >> >> >> >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >>
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
We have a pool implementation where we had to call abort especially for revoked long running transactions, since they regularly have been locked up in a connection wide synchronized in the close. However we only use it for that case and try to close the objects after the abort anyway. We don’t use it to close connections when they are only idle or too old - owned by the pool. It seems to be however still a good idea to run the abort/close in a separate task as it can also take ages for example when TLS close handshakes hang (we did try to reduce the connection timeout before but that also can be blocked before the abort) Gruss Bernd -- https://Bernd.eckenfels.net Von: Gary Gregory Gesendet: Saturday, August 29, 2020 10:02:42 PM An: Commons Developers List Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections? On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: > > On 8/29/20 11:03 AM, Gary Gregory wrote: > > On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz > wrote: > > > >> A pool-related deadlock was reported recently in [1] to tomcat-user. > >> The OP was using a different pool, but it looks to me like the same > >> deadlock could happen with dbcp. The source is arguably a driver bug, > >> but in [2], the driver maintainer makes the good point that to avoid the > >> problem in [1] (and others like it) it would be better if pool > >> maintainers used abort instead of close to disconnect "stuck" or > >> abandoned connections. That makes sense to me. > >> > >> To implement this in dbcp, we would need to either always use abort > >> instead of close when closing physical connections (a bad idea, IMO, as > >> it could mess up counters due to async execution and add overhead) > > > > I agree that changing the happy path from close() to abort() is a bad > idea; > > especially since the Connection#close() Javadoc contract releases > database > > resources while abort() does not or is vague about it (at least in the > Java > > 8 Javadoc). > > Yes, bad idea to do that, I think. > > > > > > >> or > >> modify pool to call a different factory method than destroy when > >> triggering close-abandoned. I think the latter makes sense and it could > >> be useful for other pool clients to be able to this kind of thing as > well. > >> > > This means that we would treat abandoned connections like a real resource > > leak in the sense that (if) abort() leaks database resources, then the > DBA > > would have to deal with that. > > Yeah, unless the driver does it cleanly. The other thing to note here > is that abort is async and is supposed to allow ops in progress to > complete, etc. That is kinder to clients but technically allows > maxActive to exceed the cap while the abort is in progress. > > > > > I think formally surfacing an abort path vs. the current close/destroy is > > OK since it reflects what JDBC allows. If that means trickling the > concept > > down to Pool, then that's OK as well IMO. This could be generically > > surfaced with a "close mode" enum passed to close/destroy APIs. This is > > similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): > > > https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html > > Exactly. I think the abandoned-destroy is legitimately different from > the other destroy use cases and I could see other kinds of pools > (sockets, jms, etc) benefiting from having the ability to handle destroy > differently for abandoned (or other) cases. > > > > > > >> What I propose is that we make these change to pool: > >> > >> 1. Add a new method to PooledObjectFactory called "abandonObject" with > >> default implementation delegating to destroyObject. > >> > > That or overload destroyObject with a destroyObject(CloseMode) would be > > easier to find. > > Yes, I think that would be better. That raises the interesting question > of what are the CloseModes. All we need for this issue is normal vs > abandoned, but I can see value in providing fuller context to > factories. So if we are doing it, why not something like: > > invalidated > abandoned > evicted > excess idle > failed validation > failed activation > failed passivation > clearing pool > I think a YAGNI-style approach would work well here by starting with 2 modes: one to support the current behavior (default) and another to support the new behavior backed by Connection#abort() in DBCP. A third "escape hatch&quo
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On Sat, Aug 29, 2020 at 2:41 PM Phil Steitz wrote: > > On 8/29/20 11:03 AM, Gary Gregory wrote: > > On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz > wrote: > > > >> A pool-related deadlock was reported recently in [1] to tomcat-user. > >> The OP was using a different pool, but it looks to me like the same > >> deadlock could happen with dbcp. The source is arguably a driver bug, > >> but in [2], the driver maintainer makes the good point that to avoid the > >> problem in [1] (and others like it) it would be better if pool > >> maintainers used abort instead of close to disconnect "stuck" or > >> abandoned connections. That makes sense to me. > >> > >> To implement this in dbcp, we would need to either always use abort > >> instead of close when closing physical connections (a bad idea, IMO, as > >> it could mess up counters due to async execution and add overhead) > > > > I agree that changing the happy path from close() to abort() is a bad > idea; > > especially since the Connection#close() Javadoc contract releases > database > > resources while abort() does not or is vague about it (at least in the > Java > > 8 Javadoc). > > Yes, bad idea to do that, I think. > > > > > > >> or > >> modify pool to call a different factory method than destroy when > >> triggering close-abandoned. I think the latter makes sense and it could > >> be useful for other pool clients to be able to this kind of thing as > well. > >> > > This means that we would treat abandoned connections like a real resource > > leak in the sense that (if) abort() leaks database resources, then the > DBA > > would have to deal with that. > > Yeah, unless the driver does it cleanly. The other thing to note here > is that abort is async and is supposed to allow ops in progress to > complete, etc. That is kinder to clients but technically allows > maxActive to exceed the cap while the abort is in progress. > > > > > I think formally surfacing an abort path vs. the current close/destroy is > > OK since it reflects what JDBC allows. If that means trickling the > concept > > down to Pool, then that's OK as well IMO. This could be generically > > surfaced with a "close mode" enum passed to close/destroy APIs. This is > > similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): > > > https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html > > Exactly. I think the abandoned-destroy is legitimately different from > the other destroy use cases and I could see other kinds of pools > (sockets, jms, etc) benefiting from having the ability to handle destroy > differently for abandoned (or other) cases. > > > > > > >> What I propose is that we make these change to pool: > >> > >> 1. Add a new method to PooledObjectFactory called "abandonObject" with > >> default implementation delegating to destroyObject. > >> > > That or overload destroyObject with a destroyObject(CloseMode) would be > > easier to find. > > Yes, I think that would be better. That raises the interesting question > of what are the CloseModes. All we need for this issue is normal vs > abandoned, but I can see value in providing fuller context to > factories. So if we are doing it, why not something like: > > invalidated > abandoned > evicted > excess idle > failed validation > failed activation > failed passivation > clearing pool > I think a YAGNI-style approach would work well here by starting with 2 modes: one to support the current behavior (default) and another to support the new behavior backed by Connection#abort() in DBCP. A third "escape hatch" mode could see passing in a lamba I suppose. I really think we should start by just focusing on the one issue at hand before we enlarge the solution. 2c, Gary > Phil > > > > > > > Gary > > > > > >> 2. Change the logic in removeAbandoned to call the new factory method > >> instead of destroyObject > >> > >> And then in dbcp: > >> > >>* Add implementation of abandonObject in PoolableConnectionFactory to > >> call abort. > >> > >> wdyt? > >> > >> Phil > >> > >> [1] > >> > >> > https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E > >> > >> [2] https://bugs.mysql.com/bug.php?id=64954 > >> > >> > >> > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 8/29/20 11:03 AM, Gary Gregory wrote: On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz wrote: A pool-related deadlock was reported recently in [1] to tomcat-user. The OP was using a different pool, but it looks to me like the same deadlock could happen with dbcp. The source is arguably a driver bug, but in [2], the driver maintainer makes the good point that to avoid the problem in [1] (and others like it) it would be better if pool maintainers used abort instead of close to disconnect "stuck" or abandoned connections. That makes sense to me. To implement this in dbcp, we would need to either always use abort instead of close when closing physical connections (a bad idea, IMO, as it could mess up counters due to async execution and add overhead) I agree that changing the happy path from close() to abort() is a bad idea; especially since the Connection#close() Javadoc contract releases database resources while abort() does not or is vague about it (at least in the Java 8 Javadoc). Yes, bad idea to do that, I think. or modify pool to call a different factory method than destroy when triggering close-abandoned. I think the latter makes sense and it could be useful for other pool clients to be able to this kind of thing as well. This means that we would treat abandoned connections like a real resource leak in the sense that (if) abort() leaks database resources, then the DBA would have to deal with that. Yeah, unless the driver does it cleanly. The other thing to note here is that abort is async and is supposed to allow ops in progress to complete, etc. That is kinder to clients but technically allows maxActive to exceed the cap while the abort is in progress. I think formally surfacing an abort path vs. the current close/destroy is OK since it reflects what JDBC allows. If that means trickling the concept down to Pool, then that's OK as well IMO. This could be generically surfaced with a "close mode" enum passed to close/destroy APIs. This is similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html Exactly. I think the abandoned-destroy is legitimately different from the other destroy use cases and I could see other kinds of pools (sockets, jms, etc) benefiting from having the ability to handle destroy differently for abandoned (or other) cases. What I propose is that we make these change to pool: 1. Add a new method to PooledObjectFactory called "abandonObject" with default implementation delegating to destroyObject. That or overload destroyObject with a destroyObject(CloseMode) would be easier to find. Yes, I think that would be better. That raises the interesting question of what are the CloseModes. All we need for this issue is normal vs abandoned, but I can see value in providing fuller context to factories. So if we are doing it, why not something like: invalidated abandoned evicted excess idle failed validation failed activation failed passivation clearing pool Phil Gary 2. Change the logic in removeAbandoned to call the new factory method instead of destroyObject And then in dbcp: * Add implementation of abandonObject in PoolableConnectionFactory to call abort. wdyt? Phil [1] https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E [2] https://bugs.mysql.com/bug.php?id=64954 - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On Sat, Aug 29, 2020 at 1:35 PM Phil Steitz wrote: > A pool-related deadlock was reported recently in [1] to tomcat-user. > The OP was using a different pool, but it looks to me like the same > deadlock could happen with dbcp. The source is arguably a driver bug, > but in [2], the driver maintainer makes the good point that to avoid the > problem in [1] (and others like it) it would be better if pool > maintainers used abort instead of close to disconnect "stuck" or > abandoned connections. That makes sense to me. > > To implement this in dbcp, we would need to either always use abort > instead of close when closing physical connections (a bad idea, IMO, as > it could mess up counters due to async execution and add overhead) I agree that changing the happy path from close() to abort() is a bad idea; especially since the Connection#close() Javadoc contract releases database resources while abort() does not or is vague about it (at least in the Java 8 Javadoc). > or > modify pool to call a different factory method than destroy when > triggering close-abandoned. I think the latter makes sense and it could > be useful for other pool clients to be able to this kind of thing as well. > This means that we would treat abandoned connections like a real resource leak in the sense that (if) abort() leaks database resources, then the DBA would have to deal with that. I think formally surfacing an abort path vs. the current close/destroy is OK since it reflects what JDBC allows. If that means trickling the concept down to Pool, then that's OK as well IMO. This could be generically surfaced with a "close mode" enum passed to close/destroy APIs. This is similar to what we did in HttpCore's CloseMode(IMMEDIATE vs GRACEFUL): https://javadoc.io/static/org.apache.httpcomponents.core5/httpcore5/5.0.1/org/apache/hc/core5/io/CloseMode.html > What I propose is that we make these change to pool: > > 1. Add a new method to PooledObjectFactory called "abandonObject" with > default implementation delegating to destroyObject. > That or overload destroyObject with a destroyObject(CloseMode) would be easier to find. Gary > 2. Change the logic in removeAbandoned to call the new factory method > instead of destroyObject > > And then in dbcp: > > * Add implementation of abandonObject in PoolableConnectionFactory to > call abort. > > wdyt? > > Phil > > [1] > > https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E > > [2] https://bugs.mysql.com/bug.php?id=64954 > > >
[dbcp][pool] Use abort instead of close for abandoned connections?
A pool-related deadlock was reported recently in [1] to tomcat-user. The OP was using a different pool, but it looks to me like the same deadlock could happen with dbcp. The source is arguably a driver bug, but in [2], the driver maintainer makes the good point that to avoid the problem in [1] (and others like it) it would be better if pool maintainers used abort instead of close to disconnect "stuck" or abandoned connections. That makes sense to me. To implement this in dbcp, we would need to either always use abort instead of close when closing physical connections (a bad idea, IMO, as it could mess up counters due to async execution and add overhead) or modify pool to call a different factory method than destroy when triggering close-abandoned. I think the latter makes sense and it could be useful for other pool clients to be able to this kind of thing as well. What I propose is that we make these change to pool: 1. Add a new method to PooledObjectFactory called "abandonObject" with default implementation delegating to destroyObject. 2. Change the logic in removeAbandoned to call the new factory method instead of destroyObject And then in dbcp: * Add implementation of abandonObject in PoolableConnectionFactory to call abort. wdyt? Phil [1] https://lists.apache.org/thread.html/r7095d641bbc8db0a526d2d9a18684202347a747cf0e315a4a50d2001%40%3Cusers.tomcat.apache.org%3E [2] https://bugs.mysql.com/bug.php?id=64954
