You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ps...@apache.org on 2015/05/12 15:23:03 UTC
svn commit: r1678941 - in /commons/proper/pool/trunk: ./ src/changes/
src/main/java/org/apache/commons/pool2/impl/ src/site/xdoc/
src/test/java/org/apache/commons/pool2/impl/
Author: psteitz
Date: Tue May 12 13:23:02 2015
New Revision: 1678941
URL: http://svn.apache.org/r1678941
Log:
Replaced raw objects used as keys in allObjects maps with IdentityWrappers so
equal but distinct instances can cohabitate in GOP, GKOP and mutation does not
cause problems.
JIRA: POOL-283, POOL-284
Modified:
commons/proper/pool/trunk/findbugs-exclude-filter.xml
commons/proper/pool/trunk/src/changes/changes.xml
commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
commons/proper/pool/trunk/src/site/xdoc/index.xml
commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
Modified: commons/proper/pool/trunk/findbugs-exclude-filter.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/findbugs-exclude-filter.xml?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/findbugs-exclude-filter.xml (original)
+++ commons/proper/pool/trunk/findbugs-exclude-filter.xml Tue May 12 13:23:02 2015
@@ -161,4 +161,16 @@
<Method name="allocate" />
<Bug pattern="VO_VOLATILE_INCREMENT" />
</Match>
+ <Match>
+ <!-- Only used internally in hashmap to compare keys known to be of the same type -->
+ <Class name="org.apache.commons.pool2.impl.BaseGenericObjectPool$IdentityWrapper" />
+ <Method name="equals" />
+ <Bug pattern="BC_EQUALS_METHOD_SHOULD_WORK_FOR_ALL_OBJECTS" />
+ </Match>
+ <Match>
+ <!-- Only used internally in hashmap to compare keys that can't be null -->
+ <Class name="org.apache.commons.pool2.impl.BaseGenericObjectPool$IdentityWrapper" />
+ <Method name="equals" />
+ <Bug pattern="NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT" />
+ </Match>
</FindBugsFilter>
Modified: commons/proper/pool/trunk/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/changes/changes.xml?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/changes/changes.xml (original)
+++ commons/proper/pool/trunk/src/changes/changes.xml Tue May 12 13:23:02 2015
@@ -51,6 +51,15 @@ The <action> type attribute can be add,u
</release>
<release version="2.3" date="2014-12-30" description=
"This is a maintenance release that includes bug fixes and minor enhancements.">
+ <action dev="psteitz" type="fix" issue="POOL-283">
+ Eliminated the requirement that objects managed by GenericObjectPool or
+ GenericKeyedObjectPool be discernible by equals. Prior to this fix,
+ equal but distinct object instances could not be stored in the same pool.
+ </action>
+ <action dev="psteitz" type="fix" issue="POOL-284">
+ Eliminated the requirement that object equality and hashcodes do not change
+ while objects are under management by GenericObjectPool or GenericKeyedObjectPool.
+ </action>
<action dev="psteitz" type="fix" issue="POOL-279" due-to="Jacopo Cappellato">
Eliminated possibility that DefaultPoolObject#getIdleTimeMillis() could
return a negative value. Use by pool implementations would not hit this
Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java (original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/BaseGenericObjectPool.java Tue May 12 13:23:02 2015
@@ -1151,5 +1151,46 @@ public abstract class BaseGenericObjectP
}
}
+
+ /**
+ * Wrapper for objects under management by the pool.
+ *
+ * GenericObjectPool and GenericKeyedObjectPool maintain references to all
+ * objects under management using maps keyed on the objects. This wrapper
+ * class ensures that objects can work as hash keys.
+ *
+ * @param <T> type of objects in the pool
+ */
+ static class IdentityWrapper<T> {
+ /** Wrapped object */
+ private final T instance;
+
+ /**
+ * Create a wrapper for an instance.
+ *
+ * @param instance object to wrap
+ */
+ public IdentityWrapper(T instance) {
+ this.instance = instance;
+ }
+
+ @Override
+ public int hashCode() {
+ return System.identityHashCode(instance);
+ }
+
+ @Override
+ @SuppressWarnings("rawtypes")
+ public boolean equals(Object other) {
+ return ((IdentityWrapper) other).instance == instance;
+ }
+
+ /**
+ * @return the wrapped object
+ */
+ public T getObject() {
+ return instance;
+ }
+ }
}
Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java (original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java Tue May 12 13:23:02 2015
@@ -471,7 +471,7 @@ public class GenericKeyedObjectPool<K,T>
ObjectDeque<T> objectDeque = poolMap.get(key);
- PooledObject<T> p = objectDeque.getAllObjects().get(obj);
+ PooledObject<T> p = objectDeque.getAllObjects().get(new IdentityWrapper<T>(obj));
if (p == null) {
throw new IllegalStateException(
@@ -575,7 +575,7 @@ public class GenericKeyedObjectPool<K,T>
ObjectDeque<T> objectDeque = poolMap.get(key);
- PooledObject<T> p = objectDeque.getAllObjects().get(obj);
+ PooledObject<T> p = objectDeque.getAllObjects().get(new IdentityWrapper<T>(obj));
if (p == null) {
throw new IllegalStateException(
"Object not currently part of this pool");
@@ -1036,7 +1036,7 @@ public class GenericKeyedObjectPool<K,T>
}
createdCount.incrementAndGet();
- objectDeque.getAllObjects().put(p.getObject(), p);
+ objectDeque.getAllObjects().put(new IdentityWrapper<T>(p.getObject()), p);
return p;
}
@@ -1059,7 +1059,7 @@ public class GenericKeyedObjectPool<K,T>
boolean isIdle = objectDeque.getIdleObjects().remove(toDestroy);
if (isIdle || always) {
- objectDeque.getAllObjects().remove(toDestroy.getObject());
+ objectDeque.getAllObjects().remove(new IdentityWrapper<T>(toDestroy.getObject()));
toDestroy.invalidate();
try {
@@ -1428,12 +1428,11 @@ public class GenericKeyedObjectPool<K,T>
private final AtomicInteger createCount = new AtomicInteger(0);
/*
- * The map is keyed on pooled instances. Note: pooled instances
- * <em>must</em> be distinguishable by equals for this structure to
- * work properly.
+ * The map is keyed on pooled instances, wrapped to ensure that
+ * they work properly as keys.
*/
- private final Map<S, PooledObject<S>> allObjects =
- new ConcurrentHashMap<S, PooledObject<S>>();
+ private final Map<IdentityWrapper<S>, PooledObject<S>> allObjects =
+ new ConcurrentHashMap<IdentityWrapper<S>, PooledObject<S>>();
/*
* Number of threads with registered interest in this key.
@@ -1485,7 +1484,7 @@ public class GenericKeyedObjectPool<K,T>
*
* @return All the objects
*/
- public Map<S, PooledObject<S>> getAllObjects() {
+ public Map<IdentityWrapper<S>, PooledObject<S>> getAllObjects() {
return allObjects;
}
Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java (original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java Tue May 12 13:23:02 2015
@@ -534,7 +534,7 @@ public class GenericObjectPool<T> extend
*/
@Override
public void returnObject(T obj) {
- PooledObject<T> p = allObjects.get(obj);
+ PooledObject<T> p = allObjects.get(new IdentityWrapper<T>(obj));
if (!isAbandonedConfig()) {
if (p == null) {
@@ -635,7 +635,7 @@ public class GenericObjectPool<T> extend
*/
@Override
public void invalidateObject(T obj) throws Exception {
- PooledObject<T> p = allObjects.get(obj);
+ PooledObject<T> p = allObjects.get(new IdentityWrapper<T>(obj));
if (p == null) {
if (isAbandonedConfig()) {
return;
@@ -866,7 +866,7 @@ public class GenericObjectPool<T> extend
}
createdCount.incrementAndGet();
- allObjects.put(p.getObject(), p);
+ allObjects.put(new IdentityWrapper<T>(p.getObject()), p);
return p;
}
@@ -881,7 +881,7 @@ public class GenericObjectPool<T> extend
private void destroy(PooledObject<T> toDestory) throws Exception {
toDestory.invalidate();
idleObjects.remove(toDestory);
- allObjects.remove(toDestory.getObject());
+ allObjects.remove(new IdentityWrapper<T>(toDestory.getObject()));
try {
factory.destroyObject(toDestory);
} finally {
@@ -1033,7 +1033,7 @@ public class GenericObjectPool<T> extend
public void use(T pooledObject) {
AbandonedConfig ac = this.abandonedConfig;
if (ac != null && ac.getUseUsageTracking()) {
- PooledObject<T> wrapper = allObjects.get(pooledObject);
+ PooledObject<T> wrapper = allObjects.get(new IdentityWrapper<T>(pooledObject));
wrapper.use();
}
}
@@ -1119,8 +1119,8 @@ public class GenericObjectPool<T> extend
* #_maxActive}. Map keys are pooled objects, values are the PooledObject
* wrappers used internally by the pool.
*/
- private final Map<T, PooledObject<T>> allObjects =
- new ConcurrentHashMap<T, PooledObject<T>>();
+ private final Map<IdentityWrapper<T>, PooledObject<T>> allObjects =
+ new ConcurrentHashMap<IdentityWrapper<T>, PooledObject<T>>();
/*
* The combined count of the currently created objects and those in the
* process of being created. Under load, it may exceed {@link #_maxActive}
@@ -1137,4 +1137,5 @@ public class GenericObjectPool<T> extend
// Additional configuration properties for abandoned object tracking
private volatile AbandonedConfig abandonedConfig = null;
+
}
Modified: commons/proper/pool/trunk/src/site/xdoc/index.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/site/xdoc/index.xml?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/site/xdoc/index.xml (original)
+++ commons/proper/pool/trunk/src/site/xdoc/index.xml Tue May 12 13:23:02 2015
@@ -87,11 +87,6 @@ public interface PooledObjectFactory<
where <code>Foo</code> is the type of the objects being pooled (the return type of <code>create()</code>).
</p>
<p>
- Another important difference between 1.x and version 2 pools is that the implementations provided maintain references
- to all objects under management by the pool. Correct behavior depends on underlying instances being discernable by
- equals - i.e., if A and B are two different instances being managed by the pool, A.equals(B) should return false.
- </p>
- <p>
<a href="./apidocs/org/apache/commons/pool2/KeyedPooledObjectFactory.html"><code>KeyedPooledObjectFactory</code></a>
defines a similar interface for <code>KeyedObjectPool</code>s:
</p>
Modified: commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java (original)
+++ commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java Tue May 12 13:23:02 2015
@@ -29,6 +29,7 @@ import java.io.PrintWriter;
import java.io.StringWriter;
import java.lang.management.ManagementFactory;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.NoSuchElementException;
import java.util.Random;
import java.util.Set;
@@ -2179,6 +2180,46 @@ public class TestGenericKeyedObjectPool
pool.returnObject("one", o2);
pool.close();
}
+
+ /**
+ * Verifies that when a factory's makeObject produces instances that are not
+ * discernible by equals, the pool can handle them.
+ *
+ * JIRA: POOL-283
+ */
+ @Test
+ public void testEqualsIndiscernible() throws Exception {
+ final HashSetFactory factory = new HashSetFactory();
+ final GenericKeyedObjectPool<String,HashSet<String>> pool =
+ new GenericKeyedObjectPool<String, HashSet<String>>(
+ factory, new GenericKeyedObjectPoolConfig());
+ final HashSet<String> s1 = pool.borrowObject("a");
+ final HashSet<String> s2 = pool.borrowObject("a");
+ pool.returnObject("a", s1);
+ pool.returnObject("a", s2);
+ pool.close();
+ }
+
+ /**
+ * Verifies that when a borrowed object is mutated in a way that does not
+ * preserve equality and hashcode, the pool can recognized it on return.
+ *
+ * JIRA: POOL-284
+ */
+ @Test
+ public void testMutable() throws Exception {
+ final HashSetFactory factory = new HashSetFactory();
+ final GenericKeyedObjectPool<String,HashSet<String>> pool =
+ new GenericKeyedObjectPool<String, HashSet<String>>(
+ factory, new GenericKeyedObjectPoolConfig());
+ final HashSet<String> s1 = pool.borrowObject("a");
+ final HashSet<String> s2 = pool.borrowObject("a");
+ s1.add("One");
+ s2.add("One");
+ pool.returnObject("a", s1);
+ pool.returnObject("a", s2);
+ pool.close();
+ }
private static class DummyFactory
extends BaseKeyedPooledObjectFactory<Object,Object> {
@@ -2191,6 +2232,23 @@ public class TestGenericKeyedObjectPool
return new DefaultPooledObject<Object>(value);
}
}
+
+ /**
+ * Factory that creates HashSets. Note that this means
+ * 0) All instances are initially equal (not discernible by equals)
+ * 1) Instances are mutable and mutation can cause change in identity / hashcode.
+ */
+ private static final class HashSetFactory
+ extends BaseKeyedPooledObjectFactory<String, HashSet<String>> {
+ @Override
+ public HashSet<String> create(String key) throws Exception {
+ return new HashSet<String>();
+ }
+ @Override
+ public PooledObject<HashSet<String>> wrap(HashSet<String> value) {
+ return new DefaultPooledObject<HashSet<String>>(value);
+ }
+ }
private static class SimplePerKeyFactory
extends BaseKeyedPooledObjectFactory<Object,Object> {
Modified: commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java?rev=1678941&r1=1678940&r2=1678941&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java (original)
+++ commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java Tue May 12 13:23:02 2015
@@ -2406,7 +2406,7 @@ public class TestGenericObjectPool exten
*
* JIRA: POOL-283
*/
- //@Test
+ @Test
public void testEqualsIndiscernible() throws Exception {
final HashSetFactory factory = new HashSetFactory();
final GenericObjectPool<HashSet<String>> pool = new GenericObjectPool<HashSet<String>>(
@@ -2424,7 +2424,7 @@ public class TestGenericObjectPool exten
*
* JIRA: POOL-284
*/
- //@Test
+ @Test
public void testMutable() throws Exception {
final HashSetFactory factory = new HashSetFactory();
final GenericObjectPool<HashSet<String>> pool = new GenericObjectPool<HashSet<String>>(