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 2011/12/13 20:32:08 UTC
Re: svn commit: r1213845 - in /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl:
GenericKeyedObjectPoolFactory.java GenericObjectPoolFactory.java
On 13/12/2011 18:40, sebb@apache.org wrote:
> Author: sebb
> Date: Tue Dec 13 18:40:48 2011
> New Revision: 1213845
>
> URL: http://svn.apache.org/viewvc?rev=1213845&view=rev
> Log:
> Make factories thread-safe.
> [No existing tests actually used the setter methods which have now been removed]
This commit removes functionality and doesn't achieve the stated
objective since the config object is accessible and not thread-safe.
I am close to vetoing this commit.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1213845 - in /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl:
GenericKeyedObjectPoolFactory.java GenericObjectPoolFactory.java
Posted by sebb <se...@gmail.com>.
On 13 December 2011 19:59, sebb <se...@gmail.com> wrote:
> On 13 December 2011 19:32, Mark Thomas <ma...@apache.org> wrote:
>> On 13/12/2011 18:40, sebb@apache.org wrote:
>>> Author: sebb
>>> Date: Tue Dec 13 18:40:48 2011
>>> New Revision: 1213845
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1213845&view=rev
>>> Log:
>>> Make factories thread-safe.
>>> [No existing tests actually used the setter methods which have now been removed]
>>
>> This commit removes functionality and doesn't achieve the stated
>> objective since the config object is accessible and not thread-safe.
>
> Oops - the config object is cloned prior to being stored by the ctor
> (and was cloned by the setter) but is not cloned by the getter.
> My bad; did not notice that.
>
> That can of course be fixed, either by cloning in the get or by
> removing the getter.
>
> Even if the setter is restored, I think the getter needs to clone the
> config to ensure thread-safety.
>
> Is it useful to be able to change the factory after creation?
> And are there any configuration items that it does not make sense to change?
>
> If so, we need unit tests.
>
>> I am close to vetoing this commit.
>
> Fine, but the factory classes still need to be fixed.
I've fixed the getters.
Just done a check of DBCP 1.4, and that calls
GenericKeyedObjectPoolFactory (does not call GenericObjectPoolFactory)
The main code creates the factory but then uses it through the
KeyedObjectPoolFactory interface, so is clearly not interested in
setters/getters.
I will start a separate thread about factory thread safety.
>> 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: svn commit: r1213845 - in /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl:
GenericKeyedObjectPoolFactory.java GenericObjectPoolFactory.java
Posted by sebb <se...@gmail.com>.
On 13 December 2011 19:32, Mark Thomas <ma...@apache.org> wrote:
> On 13/12/2011 18:40, sebb@apache.org wrote:
>> Author: sebb
>> Date: Tue Dec 13 18:40:48 2011
>> New Revision: 1213845
>>
>> URL: http://svn.apache.org/viewvc?rev=1213845&view=rev
>> Log:
>> Make factories thread-safe.
>> [No existing tests actually used the setter methods which have now been removed]
>
> This commit removes functionality and doesn't achieve the stated
> objective since the config object is accessible and not thread-safe.
Oops - the config object is cloned prior to being stored by the ctor
(and was cloned by the setter) but is not cloned by the getter.
My bad; did not notice that.
That can of course be fixed, either by cloning in the get or by
removing the getter.
Even if the setter is restored, I think the getter needs to clone the
config to ensure thread-safety.
Is it useful to be able to change the factory after creation?
And are there any configuration items that it does not make sense to change?
If so, we need unit tests.
> I am close to vetoing this commit.
Fine, but the factory classes still need to be fixed.
> 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