You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gary Gregory <ga...@gmail.com> on 2018/06/07 15:44:22 UTC
[DBCP] DelegatingConnection, hashCode(), and equals()
Hi All:
I just ran into a case where different instances of subclasses
of DelegatingConnection like PoolGuardConnectionWrapper and ConnectionImpl
are used as keys in a Map (Map<java.sql.Connection, Foo>)
The problem is that when you borrow a Connection out of a pool, you get a
new PoolGuardConnectionWrapper, so that the Map in the eventual call site
grows and grows because the intention is that the Map key should be the
same when you borrow the same underlying Connection later.
If DelegatingConnection implemented hashCode() and equals() to account for
some or all of its instance variables, then one could use
DelegatingConnection instances as keys in a Map with the behavior I expect,
YMMV.
The issue is that implementing hashCode() and equals() where we did not
before could have unexpected side-effects for existing applications.
Thoughts?
Gary
Re: [DBCP] DelegatingConnection, hashCode(), and equals()
Posted by Gary Gregory <ga...@gmail.com>.
The RC is out. Please review it :-)
Gary
On Tue, Jun 12, 2018, 15:49 Gary Gregory <ga...@gmail.com> wrote:
> After fixing prepared statements in the cpdsadapter package, I found a
> cleaner solution that delegates all prepared and callable statement pooling
> to DBCP's cpdsadapter.
>
> Thank you for the feedback.
>
> I think I am ready to create an RC for DBCP 2.4.0.
>
> Gary
>
> On Thu, Jun 7, 2018 at 5:01 PM Gary Gregory <ga...@gmail.com>
> wrote:
>
>> Thanks for your feedback Mark.
>>
>> Looking deeper into DBCP for a cleaner solution, it seems we are missing
>> support for preparing callable statements
>> in org.apache.commons.dbcp2.cpdsadapter.ConnectionImpl
>> and org.apache.commons.dbcp2.cpdsadapter.PooledConnectionImpl. I see APIs
>> implemented for prepareStatement(*) but not prepareCall(*). That's a big
>> hole for our server. I'll look into filling it now...
>>
>> Gary
>>
>> On Thu, Jun 7, 2018 at 1:09 PM, Mark Thomas <ma...@apache.org> wrote:
>>
>>> On 07/06/18 17:18, Matt Sicker wrote:
>>> > This sounds like an honest bug that should be fixed, backwards
>>> > compatibility be damned. I don't see how the old behavior is useful for
>>> > anything other than connection starvation or some other strange
>>> behavior.
>>>
>>> I have completely the opposite view.
>>>
>>> There is nothing in the DBCP API that suggests that a Connection object
>>> provided by the pool will ever be seen by the client again.
>>>
>>> The implementation below sounds like deliberate misuse of the pool.
>>> Clients are not meant to retain references to objects that have been
>>> returned to the pool. To do so is nearly always the cause of all sorts
>>> of problems. I would be very strongly against any change that encouraged
>>> that sort of misuse.
>>>
>>> What is the actual use case here? What is the purpose of retaining this
>>> Map? Maybe we can come up with a better solution and/or an API change
>>> that enables the requirement to be met without having to retain
>>> references to connections after they have been returned to the pool.
>>>
>>> Mark
>>>
>>>
>>> >
>>> > On 7 June 2018 at 10:44, Gary Gregory <ga...@gmail.com> wrote:
>>> >
>>> >> Hi All:
>>> >>
>>> >> I just ran into a case where different instances of subclasses
>>> >> of DelegatingConnection like PoolGuardConnectionWrapper and
>>> ConnectionImpl
>>> >> are used as keys in a Map (Map<java.sql.Connection, Foo>)
>>> >>
>>> >> The problem is that when you borrow a Connection out of a pool, you
>>> get a
>>> >> new PoolGuardConnectionWrapper, so that the Map in the eventual call
>>> site
>>> >> grows and grows because the intention is that the Map key should be
>>> the
>>> >> same when you borrow the same underlying Connection later.
>>> >>
>>> >> If DelegatingConnection implemented hashCode() and equals() to
>>> account for
>>> >> some or all of its instance variables, then one could use
>>> >> DelegatingConnection instances as keys in a Map with the behavior I
>>> expect,
>>> >> YMMV.
>>> >>
>>> >> The issue is that implementing hashCode() and equals() where we did
>>> not
>>> >> before could have unexpected side-effects for existing applications.
>>> >>
>>> >> Thoughts?
>>> >>
>>> >> Gary
>>> >>
>>> >
>>> >
>>> >
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>>
>>
Re: [DBCP] DelegatingConnection, hashCode(), and equals()
Posted by Gary Gregory <ga...@gmail.com>.
After fixing prepared statements in the cpdsadapter package, I found a
cleaner solution that delegates all prepared and callable statement pooling
to DBCP's cpdsadapter.
Thank you for the feedback.
I think I am ready to create an RC for DBCP 2.4.0.
Gary
On Thu, Jun 7, 2018 at 5:01 PM Gary Gregory <ga...@gmail.com> wrote:
> Thanks for your feedback Mark.
>
> Looking deeper into DBCP for a cleaner solution, it seems we are missing
> support for preparing callable statements
> in org.apache.commons.dbcp2.cpdsadapter.ConnectionImpl
> and org.apache.commons.dbcp2.cpdsadapter.PooledConnectionImpl. I see APIs
> implemented for prepareStatement(*) but not prepareCall(*). That's a big
> hole for our server. I'll look into filling it now...
>
> Gary
>
> On Thu, Jun 7, 2018 at 1:09 PM, Mark Thomas <ma...@apache.org> wrote:
>
>> On 07/06/18 17:18, Matt Sicker wrote:
>> > This sounds like an honest bug that should be fixed, backwards
>> > compatibility be damned. I don't see how the old behavior is useful for
>> > anything other than connection starvation or some other strange
>> behavior.
>>
>> I have completely the opposite view.
>>
>> There is nothing in the DBCP API that suggests that a Connection object
>> provided by the pool will ever be seen by the client again.
>>
>> The implementation below sounds like deliberate misuse of the pool.
>> Clients are not meant to retain references to objects that have been
>> returned to the pool. To do so is nearly always the cause of all sorts
>> of problems. I would be very strongly against any change that encouraged
>> that sort of misuse.
>>
>> What is the actual use case here? What is the purpose of retaining this
>> Map? Maybe we can come up with a better solution and/or an API change
>> that enables the requirement to be met without having to retain
>> references to connections after they have been returned to the pool.
>>
>> Mark
>>
>>
>> >
>> > On 7 June 2018 at 10:44, Gary Gregory <ga...@gmail.com> wrote:
>> >
>> >> Hi All:
>> >>
>> >> I just ran into a case where different instances of subclasses
>> >> of DelegatingConnection like PoolGuardConnectionWrapper and
>> ConnectionImpl
>> >> are used as keys in a Map (Map<java.sql.Connection, Foo>)
>> >>
>> >> The problem is that when you borrow a Connection out of a pool, you
>> get a
>> >> new PoolGuardConnectionWrapper, so that the Map in the eventual call
>> site
>> >> grows and grows because the intention is that the Map key should be the
>> >> same when you borrow the same underlying Connection later.
>> >>
>> >> If DelegatingConnection implemented hashCode() and equals() to account
>> for
>> >> some or all of its instance variables, then one could use
>> >> DelegatingConnection instances as keys in a Map with the behavior I
>> expect,
>> >> YMMV.
>> >>
>> >> The issue is that implementing hashCode() and equals() where we did not
>> >> before could have unexpected side-effects for existing applications.
>> >>
>> >> Thoughts?
>> >>
>> >> Gary
>> >>
>> >
>> >
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
Re: [DBCP] DelegatingConnection, hashCode(), and equals()
Posted by Gary Gregory <ga...@gmail.com>.
Thanks for your feedback Mark.
Looking deeper into DBCP for a cleaner solution, it seems we are missing
support for preparing callable statements
in org.apache.commons.dbcp2.cpdsadapter.ConnectionImpl
and org.apache.commons.dbcp2.cpdsadapter.PooledConnectionImpl. I see APIs
implemented for prepareStatement(*) but not prepareCall(*). That's a big
hole for our server. I'll look into filling it now...
Gary
On Thu, Jun 7, 2018 at 1:09 PM, Mark Thomas <ma...@apache.org> wrote:
> On 07/06/18 17:18, Matt Sicker wrote:
> > This sounds like an honest bug that should be fixed, backwards
> > compatibility be damned. I don't see how the old behavior is useful for
> > anything other than connection starvation or some other strange behavior.
>
> I have completely the opposite view.
>
> There is nothing in the DBCP API that suggests that a Connection object
> provided by the pool will ever be seen by the client again.
>
> The implementation below sounds like deliberate misuse of the pool.
> Clients are not meant to retain references to objects that have been
> returned to the pool. To do so is nearly always the cause of all sorts
> of problems. I would be very strongly against any change that encouraged
> that sort of misuse.
>
> What is the actual use case here? What is the purpose of retaining this
> Map? Maybe we can come up with a better solution and/or an API change
> that enables the requirement to be met without having to retain
> references to connections after they have been returned to the pool.
>
> Mark
>
>
> >
> > On 7 June 2018 at 10:44, Gary Gregory <ga...@gmail.com> wrote:
> >
> >> Hi All:
> >>
> >> I just ran into a case where different instances of subclasses
> >> of DelegatingConnection like PoolGuardConnectionWrapper and
> ConnectionImpl
> >> are used as keys in a Map (Map<java.sql.Connection, Foo>)
> >>
> >> The problem is that when you borrow a Connection out of a pool, you get
> a
> >> new PoolGuardConnectionWrapper, so that the Map in the eventual call
> site
> >> grows and grows because the intention is that the Map key should be the
> >> same when you borrow the same underlying Connection later.
> >>
> >> If DelegatingConnection implemented hashCode() and equals() to account
> for
> >> some or all of its instance variables, then one could use
> >> DelegatingConnection instances as keys in a Map with the behavior I
> expect,
> >> YMMV.
> >>
> >> The issue is that implementing hashCode() and equals() where we did not
> >> before could have unexpected side-effects for existing applications.
> >>
> >> Thoughts?
> >>
> >> Gary
> >>
> >
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [DBCP] DelegatingConnection, hashCode(), and equals()
Posted by Mark Thomas <ma...@apache.org>.
On 07/06/18 17:18, Matt Sicker wrote:
> This sounds like an honest bug that should be fixed, backwards
> compatibility be damned. I don't see how the old behavior is useful for
> anything other than connection starvation or some other strange behavior.
I have completely the opposite view.
There is nothing in the DBCP API that suggests that a Connection object
provided by the pool will ever be seen by the client again.
The implementation below sounds like deliberate misuse of the pool.
Clients are not meant to retain references to objects that have been
returned to the pool. To do so is nearly always the cause of all sorts
of problems. I would be very strongly against any change that encouraged
that sort of misuse.
What is the actual use case here? What is the purpose of retaining this
Map? Maybe we can come up with a better solution and/or an API change
that enables the requirement to be met without having to retain
references to connections after they have been returned to the pool.
Mark
>
> On 7 June 2018 at 10:44, Gary Gregory <ga...@gmail.com> wrote:
>
>> Hi All:
>>
>> I just ran into a case where different instances of subclasses
>> of DelegatingConnection like PoolGuardConnectionWrapper and ConnectionImpl
>> are used as keys in a Map (Map<java.sql.Connection, Foo>)
>>
>> The problem is that when you borrow a Connection out of a pool, you get a
>> new PoolGuardConnectionWrapper, so that the Map in the eventual call site
>> grows and grows because the intention is that the Map key should be the
>> same when you borrow the same underlying Connection later.
>>
>> If DelegatingConnection implemented hashCode() and equals() to account for
>> some or all of its instance variables, then one could use
>> DelegatingConnection instances as keys in a Map with the behavior I expect,
>> YMMV.
>>
>> The issue is that implementing hashCode() and equals() where we did not
>> before could have unexpected side-effects for existing applications.
>>
>> Thoughts?
>>
>> Gary
>>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [DBCP] DelegatingConnection, hashCode(), and equals()
Posted by Matt Sicker <bo...@gmail.com>.
This sounds like an honest bug that should be fixed, backwards
compatibility be damned. I don't see how the old behavior is useful for
anything other than connection starvation or some other strange behavior.
On 7 June 2018 at 10:44, Gary Gregory <ga...@gmail.com> wrote:
> Hi All:
>
> I just ran into a case where different instances of subclasses
> of DelegatingConnection like PoolGuardConnectionWrapper and ConnectionImpl
> are used as keys in a Map (Map<java.sql.Connection, Foo>)
>
> The problem is that when you borrow a Connection out of a pool, you get a
> new PoolGuardConnectionWrapper, so that the Map in the eventual call site
> grows and grows because the intention is that the Map key should be the
> same when you borrow the same underlying Connection later.
>
> If DelegatingConnection implemented hashCode() and equals() to account for
> some or all of its instance variables, then one could use
> DelegatingConnection instances as keys in a Map with the behavior I expect,
> YMMV.
>
> The issue is that implementing hashCode() and equals() where we did not
> before could have unexpected side-effects for existing applications.
>
> Thoughts?
>
> Gary
>
--
Matt Sicker <bo...@gmail.com>