You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by se...@apache.org on 2011/12/13 19:40:49 UTC
svn commit: r1213845 - in
/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl:
GenericKeyedObjectPoolFactory.java GenericObjectPoolFactory.java
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]
Modified:
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPoolFactory.java
commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPoolFactory.java
Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPoolFactory.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPoolFactory.java?rev=1213845&r1=1213844&r2=1213845&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPoolFactory.java (original)
+++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericKeyedObjectPoolFactory.java Tue Dec 13 18:40:48 2011
@@ -24,7 +24,9 @@ import org.apache.commons.pool2.KeyedPoo
/**
* A factory for creating {@link GenericKeyedObjectPool} instances from a
* provided {@link GenericKeyedObjectPoolConfig configuration}.
- *
+ * <p>
+ * This class is intended to be thread-safe.
+ *
* @param <K> The type of keys maintained by the built pool.
* @param <T> Type of element pooled in the built pool.
*
@@ -33,8 +35,8 @@ import org.apache.commons.pool2.KeyedPoo
public class GenericKeyedObjectPoolFactory<K,T>
implements KeyedObjectPoolFactory<K,T> {
- private KeyedPoolableObjectFactory<K,T> factory;
- private GenericKeyedObjectPoolConfig<K, T> config;
+ private final KeyedPoolableObjectFactory<K,T> factory;
+ private final GenericKeyedObjectPoolConfig<K, T> config;
/**
* Create a new GenericKeyedObjectPoolFactory.
@@ -56,10 +58,6 @@ public class GenericKeyedObjectPoolFacto
return factory;
}
- public void setFactory(KeyedPoolableObjectFactory<K,T> factory) {
- this.factory = factory;
- }
-
/**
* Obtain the configuration currently used by the factory allowing the
* current settings to be viewed and changed.
@@ -70,17 +68,6 @@ public class GenericKeyedObjectPoolFacto
return config;
}
-
- /**
- * Replace the configuration settings used by this factory.
- *
- * @param config The new settings which will be passed by value
- */
- public void setConfig(GenericKeyedObjectPoolConfig<K, T> config) {
- this.config = config.clone();
- }
-
-
/**
* Create a new GenericKeyedObjectPool with the currently configured
* properties.
Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPoolFactory.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPoolFactory.java?rev=1213845&r1=1213844&r2=1213845&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPoolFactory.java (original)
+++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPoolFactory.java Tue Dec 13 18:40:48 2011
@@ -24,7 +24,9 @@ import org.apache.commons.pool2.Poolable
/**
* A factory for creating {@link GenericObjectPool} instances from a provided
* {@link GenericObjectPoolConfig configuration}.
- *
+ * <p>
+ * This class is intended to be thread-safe.
+ *
* @param <T> Type of element pooled in the built pool.
*
* @since Pool 1.0
@@ -32,8 +34,8 @@ import org.apache.commons.pool2.Poolable
public class GenericObjectPoolFactory<T> implements ObjectPoolFactory<T> {
- private PoolableObjectFactory<T> factory;
- private GenericObjectPoolConfig<T> config;
+ private final PoolableObjectFactory<T> factory;
+ private final GenericObjectPoolConfig<T> config;
/**
@@ -55,10 +57,6 @@ public class GenericObjectPoolFactory<T>
return factory;
}
- public void setFactory(PoolableObjectFactory<T> factory) {
- this.factory = factory;
- }
-
/**
* Obtain the configuration currently used by the factory allowing the
* current settings to be viewed and changed.
@@ -71,16 +69,6 @@ public class GenericObjectPoolFactory<T>
/**
- * Replace the configuration settings used by this factory.
- *
- * @param config The new settings which will be passed by value
- */
- public void setConfig(GenericObjectPoolConfig<T> config) {
- this.config = config.clone();
- }
-
-
- /**
* Create a new GenericKeyedObjectPool with the currently configured
* properties.
*
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
Re: svn commit: r1213845 - in /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl:
GenericKeyedObjectPoolFactory.java GenericObjectPoolFactory.java
Posted by Mark Thomas <ma...@apache.org>.
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