You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2011/06/13 17:22:14 UTC
Re: svn commit: r1135111 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
On 13 June 2011 15:20, <ma...@apache.org> wrote:
> Author: markt
> Date: Mon Jun 13 14:20:56 2011
> New Revision: 1135111
>
> URL: http://svn.apache.org/viewvc?rev=1135111&view=rev
> Log:
> Remove deprecation of setFactory method
>
> Modified:
> commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
>
> Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
> URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1135111&r1=1135110&r2=1135111&view=diff
> ==============================================================================
> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java (original)
> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java Mon Jun 13 14:20:56 2011
> @@ -1479,30 +1479,30 @@ public class GenericObjectPool<T> extend
> }
>
> /**
> - * Sets the {@link PoolableObjectFactory factory} this pool uses to create
> - * new instances. Trying to change the <code>factory</code> while there are
> - * borrowed objects will throw an {@link IllegalStateException}. If there
> - * are instances idle in the pool when this method is invoked, these will be
> - * destroyed using the original factory.
> + * <p>Sets the poolable object factory associated with this pool.</p>
> + *
> + * <p>If this method is called when the factory has previously been set an
> + * IllegalStateException is thrown.</p>
> *
> * @param factory
> * the {@link PoolableObjectFactory} used to create new
> * instances.
> - * @throws IllegalStateException
> - * when the factory cannot be set at this time
> - * @deprecated to be removed in version 2.0
> + * @throws IllegalStateException if the factory has already been set
> */
> @Override
> - @Deprecated
> public void setFactory(PoolableObjectFactory<T> factory)
> throws IllegalStateException {
> - assertOpen();
> - if (0 < getNumActive()) {
> - throw new IllegalStateException("Objects are already active");
> + if (this._factory == null) {
> + synchronized (factoryLock) {
> + if (this._factory == null) {
This is double-checked locking, which won't work reliably unless
_factory is volatile.
But why bother with the double check?
> + this._factory = factory;
> + } else {
> + throw new IllegalStateException("Factory already set");
> + }
> + }
> } else {
> - clear();
> + throw new IllegalStateException("Factory already set");
> }
> - _factory = factory;
> }
>
> /**
> @@ -1998,6 +1998,7 @@ public class GenericObjectPool<T> extend
>
> /** My {@link PoolableObjectFactory}. */
> private PoolableObjectFactory<T> _factory;
Needs to be volatile to ensure visibility to other threads, both for
the locking and for subsequent reads by other threads.
> + private Object factoryLock = new Object();
Locks should be final.
If they are mutable, they can fail as locks.
>
> /**
> * My idle object eviction {@link TimerTask}, if any.
> @@ -2007,7 +2008,7 @@ public class GenericObjectPool<T> extend
> /**
> * All of the objects currently associated with this pool in any state. It
> * excludes objects that have been destroyed. The size of
> - * {@link #_allObjects} will always be less than or equal to {@linl
> + * {@link #_allObjects} will always be less than or equal to {@link
> * #_maxActive}.
> */
> private Map<T, PooledObject<T>> _allObjects = null;
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1135111 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Posted by sebb <se...@gmail.com>.
On 13 June 2011 16:39, Mark Thomas <ma...@apache.org> wrote:
> On 13/06/2011 16:22, sebb wrote:
>> On 13 June 2011 15:20, <ma...@apache.org> wrote:
>>> Author: markt
>>> Date: Mon Jun 13 14:20:56 2011
>>> New Revision: 1135111
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1135111&view=rev
>>> Log:
>>> Remove deprecation of setFactory method
>>>
>>> Modified:
>>> commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
>>>
>>> Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
>>> URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1135111&r1=1135110&r2=1135111&view=diff
>>> ==============================================================================
>>> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java (original)
>>> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java Mon Jun 13 14:20:56 2011
>>> @@ -1479,30 +1479,30 @@ public class GenericObjectPool<T> extend
>>> }
>>>
>>> /**
>>> - * Sets the {@link PoolableObjectFactory factory} this pool uses to create
>>> - * new instances. Trying to change the <code>factory</code> while there are
>>> - * borrowed objects will throw an {@link IllegalStateException}. If there
>>> - * are instances idle in the pool when this method is invoked, these will be
>>> - * destroyed using the original factory.
>>> + * <p>Sets the poolable object factory associated with this pool.</p>
>>> + *
>>> + * <p>If this method is called when the factory has previously been set an
>>> + * IllegalStateException is thrown.</p>
>>> *
>>> * @param factory
>>> * the {@link PoolableObjectFactory} used to create new
>>> * instances.
>>> - * @throws IllegalStateException
>>> - * when the factory cannot be set at this time
>>> - * @deprecated to be removed in version 2.0
>>> + * @throws IllegalStateException if the factory has already been set
>>> */
>>> @Override
>>> - @Deprecated
>>> public void setFactory(PoolableObjectFactory<T> factory)
>>> throws IllegalStateException {
>>> - assertOpen();
>>> - if (0 < getNumActive()) {
>>> - throw new IllegalStateException("Objects are already active");
>>> + if (this._factory == null) {
>>> + synchronized (factoryLock) {
>>> + if (this._factory == null) {
>>
>> This is double-checked locking, which won't work reliably unless
>> _factory is volatile.
>
> Yep. I'll fix that.
>
>> But why bother with the double check?
>
> We have to have the sync so may as well fail fast where we can.
>
>> Locks should be final.
>> If they are mutable, they can fail as locks.
>
> I'll fix that too although a client is going to have to go to a fair
> amount of trouble to actually change that.
Indeed, but the idea is to prevent accidental changes in the code
itself, as well as document that the lock needs to be immutable.
> 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: r1135111 - /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Posted by Mark Thomas <ma...@apache.org>.
On 13/06/2011 16:22, sebb wrote:
> On 13 June 2011 15:20, <ma...@apache.org> wrote:
>> Author: markt
>> Date: Mon Jun 13 14:20:56 2011
>> New Revision: 1135111
>>
>> URL: http://svn.apache.org/viewvc?rev=1135111&view=rev
>> Log:
>> Remove deprecation of setFactory method
>>
>> Modified:
>> commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
>>
>> Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
>> URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1135111&r1=1135110&r2=1135111&view=diff
>> ==============================================================================
>> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java (original)
>> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java Mon Jun 13 14:20:56 2011
>> @@ -1479,30 +1479,30 @@ public class GenericObjectPool<T> extend
>> }
>>
>> /**
>> - * Sets the {@link PoolableObjectFactory factory} this pool uses to create
>> - * new instances. Trying to change the <code>factory</code> while there are
>> - * borrowed objects will throw an {@link IllegalStateException}. If there
>> - * are instances idle in the pool when this method is invoked, these will be
>> - * destroyed using the original factory.
>> + * <p>Sets the poolable object factory associated with this pool.</p>
>> + *
>> + * <p>If this method is called when the factory has previously been set an
>> + * IllegalStateException is thrown.</p>
>> *
>> * @param factory
>> * the {@link PoolableObjectFactory} used to create new
>> * instances.
>> - * @throws IllegalStateException
>> - * when the factory cannot be set at this time
>> - * @deprecated to be removed in version 2.0
>> + * @throws IllegalStateException if the factory has already been set
>> */
>> @Override
>> - @Deprecated
>> public void setFactory(PoolableObjectFactory<T> factory)
>> throws IllegalStateException {
>> - assertOpen();
>> - if (0 < getNumActive()) {
>> - throw new IllegalStateException("Objects are already active");
>> + if (this._factory == null) {
>> + synchronized (factoryLock) {
>> + if (this._factory == null) {
>
> This is double-checked locking, which won't work reliably unless
> _factory is volatile.
Yep. I'll fix that.
> But why bother with the double check?
We have to have the sync so may as well fail fast where we can.
> Locks should be final.
> If they are mutable, they can fail as locks.
I'll fix that too although a client is going to have to go to a fair
amount of trouble to actually change that.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org