commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <>
Subject Re: [pool][dbcp] Ongoing saga of setFactory
Date Thu, 16 Jun 2011 23:32:18 GMT
I think 2.0 is the opportunity to do this right. Almost like we were
designing this from scratch.

Making the factory an invariant of the pool sounds good.

Otoh If a setFactory method exists it should be implemented fully. The
throw an exception impl is pretty "smelly".


On Jun 16, 2011, at 13:39, Phil Steitz <> wrote:

> On 6/16/11 10:19 AM, Mark Thomas wrote:
>> On 16/06/2011 17:32, sebb wrote:
>>> On 16 June 2011 17:25, Phil Steitz <> wrote:
>>>> Yesterday I fixed some [dbcp] "problems" caused by the new [pool]
>>>> requirement that setFactory can only be called once.  The quotes are
>>>> because most of the problems were redundant calls to setFactory.
>>>> The reason that we left setFactory in [pool] is that [dbcp]'s
>>>> connection factory constructors call setFactory on the pool passed
>>>> to them.  It is an easy "mistake" to create a pool, then create a
>>>> connection factory and then do pool.setFactory(factory).  I
>>>> eliminated all of these usages from within [dbcp], but I bet a fair
>>>> amount of user code will similarly blow up when people upgrade.
>>> AIUI, the upgrade does require code to be changed because of the package rename?
>> It does, but that is just a search and replace and compilation will fail
>> until it is fixed.
>>> If so, can we not drop the setFactory method and provide it as a
>>> constructor parameter?
>> I don't think so. We have been around this buoy already.
>> To quote Phil's summary:
>> <quote>
>> I think it is more natural to have
>> pool = new GKOP(factory,...) have factory.setPool(this) as a side
>> effect than
>> factory = new KPOF(...,pool...) have pool.setFactory(this) as a side
>> effect
>> </quote>
>> I disagree with Phil on this. Since a pool only has one factory whereas
>> a factory may support multiple pools,
> I had not thought about it that way before.  What is humorous is
> that that logic makes it questionable to pass the pool to the
> factory constructor (or at least just one pool).  In the case of
> [dbcp]'s PoolableConnectionFactory,  it is hard-wired 1-1 to the
> pool because it needs to provide the PoolableConnections that it
> sources a reference to their owning pool.  I guess a generalized PCF
> could source objects for multiple pools, in which case the
> constructor could take a collection of pools.  I now see your
> point.  In any case, from my perspective, the requirement to be able
> to set the factory is understood and I am fine with keeping the code
> as it is, possibly with the small patch below.
> Phil
>> I think it is wrong for for "new
>> GKOP(factory,...)" to have "factory.setPool(this)" as a side-effect. I
>> think "new KPOF(...,pool...)" having "pool.setFactory(this)" as a side
>> effect is the more natural choice (as per the current code).
>>>> I hate to keep backsliding here, but maybe we should to this in GOP
>>>> setFactory:
>>>>            synchronized (factoryLock) {
>>>>                if (this.factory == null) {
>>>>                    this.factory = factory;
>>>> -                } else {
>>>> +                } else if (this.factory != factory) {
>>>>                    throw new IllegalStateException("Factory
>>>> already set");
>>>>                }
>>>>            }
>> I'd be OK with that. If we sort out logging we can log it at INFO as
>> unnecessary.
>> Mark
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:
>> For additional commands, e-mail:
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message