You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Mark Thomas <ma...@apache.org> on 2020/09/03 09:44:52 UTC
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
On 31/08/2020 01:05, Phil Steitz wrote:
<snip/>
> 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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Bernd Eckenfels <ec...@zusammenkunft.net>.
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 <ph...@gmail.com>
Gesendet: Friday, September 4, 2020 1:06:23 AM
An: dev@commons.apache.org <de...@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 <ma...@apache.org>
> Gesendet: Thursday, September 3, 2020 11:44:52 AM
> An: dev@commons.apache.org <de...@commons.apache.org>
> Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections?
>
> On 31/08/2020 01:05, Phil Steitz wrote:
>
> <snip/>
>
>> 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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Phil Steitz <ph...@gmail.com>.
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 <ma...@apache.org>
> Gesendet: Thursday, September 3, 2020 11:44:52 AM
> An: dev@commons.apache.org <de...@commons.apache.org>
> Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections?
>
> On 31/08/2020 01:05, Phil Steitz wrote:
>
> <snip/>
>
>> 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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Bernd Eckenfels <ec...@zusammenkunft.net>.
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 <ma...@apache.org>
Gesendet: Thursday, September 3, 2020 11:44:52 AM
An: dev@commons.apache.org <de...@commons.apache.org>
Betreff: Re: [dbcp][pool] Use abort instead of close for abandoned connections?
On 31/08/2020 01:05, Phil Steitz wrote:
<snip/>
> 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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
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 <ga...@gmail.com> wrote:
> On Wed, Sep 23, 2020 at 6:31 PM Phil Steitz <ph...@gmail.com> 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 <ph...@gmail.com>
>> wrote:
>> >
>> >> On 9/14/20 10:10 AM, Gary Gregory wrote:
>> >>> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <ph...@gmail.com>
>> >> 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 <garydgregory@gmail.com
>> >
>> >> 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 <ga...@gmail.com>
>> >> wrote:
>> >>>>>>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <phil.steitz@gmail.com
>> >
>> >> wrote:
>> >>>>>>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
>> >>>>>>>>> On 31/08/2020 01:05, Phil Steitz wrote:
>> >>>>>>>>>
>> >>>>>>>>> <snip/>
>> >>>>>>>>>
>> >>>>>>>>>> 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-unsubscribe@commons.apache.org
>> >>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>> >>>>>>>>>
>> >> ---------------------------------------------------------------------
>> >>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> >>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>> >>>>>>>>
>> >>>>>
>> ---------------------------------------------------------------------
>> >>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> >>>>> For additional commands, e-mail: dev-help@commons.apache.org
>> >>>>>
>> >>>> ---------------------------------------------------------------------
>> >>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> >>>> For additional commands, e-mail: dev-help@commons.apache.org
>> >>>>
>> >>> ---------------------------------------------------------------------
>> >>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> >>> For additional commands, e-mail: dev-help@commons.apache.org
>> >>>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> >> For additional commands, e-mail: dev-help@commons.apache.org
>> >>
>> >>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
On Wed, Sep 23, 2020 at 6:31 PM Phil Steitz <ph...@gmail.com> 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 <ph...@gmail.com>
> wrote:
> >
> >> On 9/14/20 10:10 AM, Gary Gregory wrote:
> >>> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <ph...@gmail.com>
> >> 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 <ga...@gmail.com>
> >> 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 <ga...@gmail.com>
> >> wrote:
> >>>>>>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com>
> >> wrote:
> >>>>>>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
> >>>>>>>>> On 31/08/2020 01:05, Phil Steitz wrote:
> >>>>>>>>>
> >>>>>>>>> <snip/>
> >>>>>>>>>
> >>>>>>>>>> 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-unsubscribe@commons.apache.org
> >>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>>>>>>
> >> ---------------------------------------------------------------------
> >>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>>>>>
> >>>>> ---------------------------------------------------------------------
> >>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>>
> >>>> ---------------------------------------------------------------------
> >>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Phil Steitz <ph...@gmail.com>.
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 <ph...@gmail.com> wrote:
>
>> On 9/14/20 10:10 AM, Gary Gregory wrote:
>>> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <ph...@gmail.com>
>> 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 <ga...@gmail.com>
>> 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 <ga...@gmail.com>
>> wrote:
>>>>>>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com>
>> wrote:
>>>>>>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
>>>>>>>>> On 31/08/2020 01:05, Phil Steitz wrote:
>>>>>>>>>
>>>>>>>>> <snip/>
>>>>>>>>>
>>>>>>>>>> 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-unsubscribe@commons.apache.org
>>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>>>
>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Mark Thomas <ma...@apache.org>.
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 <ph...@gmail.com>
>> 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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
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 <ph...@gmail.com> wrote:
>
> On 9/14/20 10:10 AM, Gary Gregory wrote:
> > On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <ph...@gmail.com>
> 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 <ga...@gmail.com>
> 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 <ga...@gmail.com>
> wrote:
> >>>>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com>
> wrote:
> >>>>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
> >>>>>>> On 31/08/2020 01:05, Phil Steitz wrote:
> >>>>>>>
> >>>>>>> <snip/>
> >>>>>>>
> >>>>>>>> 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-unsubscribe@commons.apache.org
> >>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>>>>
> >>>>>>
> ---------------------------------------------------------------------
> >>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>>>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Phil Steitz <ph...@gmail.com>.
On 9/14/20 10:10 AM, Gary Gregory wrote:
> On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <ph...@gmail.com> 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 <ga...@gmail.com> 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 <ga...@gmail.com> wrote:
>>>>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com> wrote:
>>>>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
>>>>>>> On 31/08/2020 01:05, Phil Steitz wrote:
>>>>>>>
>>>>>>> <snip/>
>>>>>>>
>>>>>>>> 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-unsubscribe@commons.apache.org
>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
On Mon, Sep 14, 2020 at 1:07 PM Phil Steitz <ph...@gmail.com> 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 <ga...@gmail.com> 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 <ga...@gmail.com> wrote:
> >>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com> wrote:
> >>>>
> >>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
> >>>>> On 31/08/2020 01:05, Phil Steitz wrote:
> >>>>>
> >>>>> <snip/>
> >>>>>
> >>>>>> 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-unsubscribe@commons.apache.org
> >>>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>>
> >>>> ---------------------------------------------------------------------
> >>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >>>> For additional commands, e-mail: dev-help@commons.apache.org
> >>>>
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Phil Steitz <ph...@gmail.com>.
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 <ga...@gmail.com> 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 <ga...@gmail.com> wrote:
>>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com> wrote:
>>>>
>>>> On 9/3/20 2:44 AM, Mark Thomas wrote:
>>>>> On 31/08/2020 01:05, Phil Steitz wrote:
>>>>>
>>>>> <snip/>
>>>>>
>>>>>> 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-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
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 <ga...@gmail.com> 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 <ga...@gmail.com> wrote:
>>
>> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com> wrote:
>> >
>> >
>> > On 9/3/20 2:44 AM, Mark Thomas wrote:
>> > > On 31/08/2020 01:05, Phil Steitz wrote:
>> > >
>> > > <snip/>
>> > >
>> > >> 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-unsubscribe@commons.apache.org
>> > > For additional commands, e-mail: dev-help@commons.apache.org
>> > >
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> > For additional commands, e-mail: dev-help@commons.apache.org
>> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
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 <ga...@gmail.com> wrote:
> On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com> wrote:
> >
> >
> > On 9/3/20 2:44 AM, Mark Thomas wrote:
> > > On 31/08/2020 01:05, Phil Steitz wrote:
> > >
> > > <snip/>
> > >
> > >> 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-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
Re: [dbcp][pool] Use abort instead of close for abandoned connections?
Posted by Gary Gregory <ga...@gmail.com>.
On Mon, Sep 7, 2020 at 6:02 PM Phil Steitz <ph...@gmail.com> wrote:
>
>
> On 9/3/20 2:44 AM, Mark Thomas wrote:
> > On 31/08/2020 01:05, Phil Steitz wrote:
> >
> > <snip/>
> >
> >> 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-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [dbcp][pool] Use abort instead of close for abandoned
connections?
Posted by Phil Steitz <ph...@gmail.com>.
On 9/3/20 2:44 AM, Mark Thomas wrote:
> On 31/08/2020 01:05, Phil Steitz wrote:
>
> <snip/>
>
>> 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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org