Remove commons pool 2.x workaround

While working on SPR-12532, an extra IdentityWrapper was added to work
around a backward compatible issue between commons pool 1.x and 2.x. This
issue (POOL-283) has actually been fixed in 2.4 and their IdentityWrapper
is using object equality so our wrapper is in the way.

Looking retrospectively, the code looks all fine without the workaround
and commons pool 2.4 or later so it has been removed.

spring-aop/src/main/java/org/springframework/aop/target/CommonsPool2TargetSource.java

@@ -43,14 +43,7 @@ import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
  * meaningful validation. All exposed Commons Pool properties use the
  * corresponding Commons Pool defaults.
  *
- * <p>Commons Pool 2.x uses object equality while Commons Pool 1.x used identity
- * equality. This clearly means that Commons Pool 2 behaves differently if several
- * instances having the same identity according to their {@link Object#equals(Object)}
- * method are managed in the same pool. To provide a smooth upgrade, a
- * backward-compatible pool is created by default; use {@link #setUseObjectEquality(boolean)}
- * if you need the standard Commons Pool 2.x behavior.
- *
- * <p>Compatible with Apache Commons Pool 2.2
+ * <p>Compatible with Apache Commons Pool 2.4
  *
  * @author Rod Johnson
  * @author Rob Harrop
@@ -81,8 +74,6 @@ public class CommonsPool2TargetSource extends AbstractPoolingTargetSource implem
 
 	private boolean blockWhenExhausted = GenericObjectPoolConfig.DEFAULT_BLOCK_WHEN_EXHAUSTED;
 
-	private boolean useObjectEquality;
-
 	/**
 	 * The Apache Commons {@code ObjectPool} used to pool target objects
 	 */
@@ -197,24 +188,6 @@ public class CommonsPool2TargetSource extends AbstractPoolingTargetSource implem
 		return blockWhenExhausted;
 	}
 
-	/**
-	 * Set if the pool should use object equality. Commons Pool 1.x has no specific requirement in
-	 * that regard and allows two distinct instances being equal to be put in the same pool. However,
-	 * this behavior has changed with commons pool 2. To preserve backward compatibility, the pool
-	 * is configured to use reference equality ({@code false}.
-	 */
-	public void setUseObjectEquality(boolean useObjectEquality) {
-		this.useObjectEquality = useObjectEquality;
-	}
-
-	/**
-	 * Specify if the pool should use object equality. Return {@code false} if it should use
-	 * reference equality (as it was the case for Commons Pool 1.x)
-	 */
-	public boolean isUseObjectEquality() {
-		return useObjectEquality;
-	}
-
 	/**
 	 * Creates and holds an ObjectPool instance.
 	 * @see #createObjectPool()
@@ -251,8 +224,7 @@ public class CommonsPool2TargetSource extends AbstractPoolingTargetSource implem
 	 */
 	@Override
 	public Object getTarget() throws Exception {
-		Object o = this.pool.borrowObject();
-		return (isUseObjectEquality() ? o : ((IdentityWrapper)o).target);
+		return this.pool.borrowObject();
 	}
 
 	/**
@@ -260,8 +232,7 @@ public class CommonsPool2TargetSource extends AbstractPoolingTargetSource implem
 	 */
 	@Override
 	public void releaseTarget(Object target) throws Exception {
-		Object value = (isUseObjectEquality() ? target : new IdentityWrapper(target));
-		this.pool.returnObject(value);
+		this.pool.returnObject(target);
 	}
 
 	@Override
@@ -291,9 +262,7 @@ public class CommonsPool2TargetSource extends AbstractPoolingTargetSource implem
 
 	@Override
 	public PooledObject<Object> makeObject() throws Exception {
-		Object target = newPrototypeInstance();
-		Object poolValue = (isUseObjectEquality() ? target : new IdentityWrapper(target));
-		return new DefaultPooledObject<Object>(poolValue);
+		return new DefaultPooledObject<Object>(newPrototypeInstance());
 	}
 
 	@Override
@@ -314,32 +283,4 @@ public class CommonsPool2TargetSource extends AbstractPoolingTargetSource implem
 	public void passivateObject(PooledObject<Object> p) throws Exception {
 	}
 
-
-	/**
-	 * Wraps the target type in the pool to restore the behavior of commons-pool 1.x.
-	 */
-	private static class IdentityWrapper {
-		private final Object target;
-
-		public IdentityWrapper(Object target) {
-			this.target = target;
-		}
-
-		@Override
-		public int hashCode() {
-			return System.identityHashCode(this.target);
-		}
-
-		@Override
-		public boolean equals(Object o) {
-			if (this == o) return true;
-			if (o == null || getClass() != o.getClass()) return false;
-
-			IdentityWrapper that = (IdentityWrapper) o;
-
-			return !(target != null ? !(target == that.target) : that.target != null);
-		}
-
-	}
-
 }

spring-context/src/test/java/org/springframework/aop/target/CommonsPool2TargetSourceTests.java

@@ -228,25 +228,6 @@ public class CommonsPool2TargetSourceTests {
 		targetSource.releaseTarget(second);
 	}
 
-	@Test
-	public void objectIdentityReleaseWithSeveralEqualInstances() throws Exception{
-		CommonsPool2TargetSource targetSource = new CommonsPool2TargetSource();
-		targetSource.setUseObjectEquality(true);
-		targetSource.setMaxWait(1);
-		prepareTargetSource(targetSource);
-
-		Object first = targetSource.getTarget();
-		Object second = targetSource.getTarget();
-		assertTrue(first instanceof SerializablePerson);
-		assertTrue(second instanceof SerializablePerson);
-		assertEquals(first, second);
-
-		targetSource.releaseTarget(first);
-		thrown.expect(IllegalStateException.class); // Only one object in pool
-		targetSource.releaseTarget(second);
-
-	}
-
 	private void prepareTargetSource(CommonsPool2TargetSource targetSource) {
 		String beanName = "target";
 

spring-context/src/test/java/org/springframework/aop/target/CommonsPoolTargetSourceTests.java

@@ -0,0 +1,219 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.aop.target;
+
+import java.util.NoSuchElementException;
+
+import org.apache.commons.pool.impl.GenericObjectPool;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.springframework.aop.framework.Advised;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+import org.springframework.beans.factory.xml.XmlBeanDefinitionReader;
+import org.springframework.context.support.StaticApplicationContext;
+import org.springframework.core.io.ClassPathResource;
+import org.springframework.tests.sample.beans.Person;
+import org.springframework.tests.sample.beans.SerializablePerson;
+import org.springframework.tests.sample.beans.SideEffectBean;
+import org.springframework.util.SerializationTestUtils;
+
+import static org.junit.Assert.*;
+
+/**
+ * Tests for pooling invoker interceptor.
+ * TODO: need to make these tests stronger: it's hard to
+ * make too many assumptions about a pool.
+ *
+ * @author Rod Johnson
+ * @author Rob Harrop
+ * @author Chris Beams
+ */
+public class CommonsPoolTargetSourceTests {
+
+	/**
+	 * Initial count value set in bean factory XML
+	 */
+	private static final int INITIAL_COUNT = 10;
+
+	private DefaultListableBeanFactory beanFactory;
+
+	@Before
+	public void setUp() throws Exception {
+		this.beanFactory = new DefaultListableBeanFactory();
+		new XmlBeanDefinitionReader(this.beanFactory).loadBeanDefinitions(
+				new ClassPathResource(getClass().getSimpleName() + "-context.xml", getClass()));
+	}
+
+	/**
+	 * We must simulate container shutdown, which should clear threads.
+	 */
+	@After
+	public void tearDown() {
+		// Will call pool.close()
+		this.beanFactory.destroySingletons();
+	}
+
+	private void testFunctionality(String name) {
+		SideEffectBean pooled = (SideEffectBean) beanFactory.getBean(name);
+		assertEquals(INITIAL_COUNT, pooled.getCount());
+		pooled.doWork();
+		assertEquals(INITIAL_COUNT + 1, pooled.getCount());
+
+		pooled = (SideEffectBean) beanFactory.getBean(name);
+		// Just check that it works--we can't make assumptions
+		// about the count
+		pooled.doWork();
+		//assertEquals(INITIAL_COUNT + 1, apartment.getCount() );
+	}
+
+	@Test
+	public void testFunctionality() {
+		testFunctionality("pooled");
+	}
+
+	@Test
+	public void testFunctionalityWithNoInterceptors() {
+		testFunctionality("pooledNoInterceptors");
+	}
+
+	@Test
+	public void testConfigMixin() {
+		SideEffectBean pooled = (SideEffectBean) beanFactory.getBean("pooledWithMixin");
+		assertEquals(INITIAL_COUNT, pooled.getCount());
+		PoolingConfig conf = (PoolingConfig) beanFactory.getBean("pooledWithMixin");
+		// TODO one invocation from setup
+		//assertEquals(1, conf.getInvocations());
+		pooled.doWork();
+		//	assertEquals("No objects active", 0, conf.getActive());
+		assertEquals("Correct target source", 25, conf.getMaxSize());
+		//	assertTrue("Some free", conf.getFree() > 0);
+		//assertEquals(2, conf.getInvocations());
+		assertEquals(25, conf.getMaxSize());
+	}
+
+	@Test
+	public void testTargetSourceSerializableWithoutConfigMixin() throws Exception {
+		CommonsPoolTargetSource cpts = (CommonsPoolTargetSource) beanFactory.getBean("personPoolTargetSource");
+
+		SingletonTargetSource serialized = (SingletonTargetSource) SerializationTestUtils.serializeAndDeserialize(cpts);
+		assertTrue(serialized.getTarget() instanceof Person);
+	}
+
+
+	@Test
+	public void testProxySerializableWithoutConfigMixin() throws Exception {
+		Person pooled = (Person) beanFactory.getBean("pooledPerson");
+
+		//System.out.println(((Advised) pooled).toProxyConfigString());
+		assertTrue(((Advised) pooled).getTargetSource() instanceof CommonsPoolTargetSource);
+
+		//((Advised) pooled).setTargetSource(new SingletonTargetSource(new SerializablePerson()));
+		Person serialized = (Person) SerializationTestUtils.serializeAndDeserialize(pooled);
+		assertTrue(((Advised) serialized).getTargetSource() instanceof SingletonTargetSource);
+		serialized.setAge(25);
+		assertEquals(25, serialized.getAge());
+	}
+
+	@Test
+	public void testHitMaxSize() throws Exception {
+		int maxSize = 10;
+
+		CommonsPoolTargetSource targetSource = new CommonsPoolTargetSource();
+		targetSource.setMaxSize(maxSize);
+		targetSource.setMaxWait(1);
+		prepareTargetSource(targetSource);
+
+		Object[] pooledInstances = new Object[maxSize];
+
+		for (int x = 0; x < maxSize; x++) {
+			Object instance = targetSource.getTarget();
+			assertNotNull(instance);
+			pooledInstances[x] = instance;
+		}
+
+		// should be at maximum now
+		try {
+			targetSource.getTarget();
+			fail("Should throw NoSuchElementException");
+		}
+		catch (NoSuchElementException ex) {
+			// desired
+		}
+
+		// lets now release an object and try to accquire a new one
+		targetSource.releaseTarget(pooledInstances[9]);
+		pooledInstances[9] = targetSource.getTarget();
+
+		// release all objects
+		for (int i = 0; i < pooledInstances.length; i++) {
+			targetSource.releaseTarget(pooledInstances[i]);
+		}
+	}
+
+	@Test
+	public void testHitMaxSizeLoadedFromContext() throws Exception {
+		Advised person = (Advised) beanFactory.getBean("maxSizePooledPerson");
+		CommonsPoolTargetSource targetSource = (CommonsPoolTargetSource) person.getTargetSource();
+
+		int maxSize = targetSource.getMaxSize();
+		Object[] pooledInstances = new Object[maxSize];
+
+		for (int x = 0; x < maxSize; x++) {
+			Object instance = targetSource.getTarget();
+			assertNotNull(instance);
+			pooledInstances[x] = instance;
+		}
+
+		// should be at maximum now
+		try {
+			targetSource.getTarget();
+			fail("Should throw NoSuchElementException");
+		}
+		catch (NoSuchElementException ex) {
+			// desired
+		}
+
+		// lets now release an object and try to accquire a new one
+		targetSource.releaseTarget(pooledInstances[9]);
+		pooledInstances[9] = targetSource.getTarget();
+
+		// release all objects
+		for (int i = 0; i < pooledInstances.length; i++) {
+			targetSource.releaseTarget(pooledInstances[i]);
+		}
+	}
+
+	@Test
+	public void testSetWhenExhaustedAction() {
+		CommonsPoolTargetSource targetSource = new CommonsPoolTargetSource();
+		targetSource.setWhenExhaustedActionName("WHEN_EXHAUSTED_BLOCK");
+		assertEquals(GenericObjectPool.WHEN_EXHAUSTED_BLOCK, targetSource.getWhenExhaustedAction());
+	}
+
+	private void prepareTargetSource(CommonsPoolTargetSource targetSource) {
+		String beanName = "target";
+
+		StaticApplicationContext applicationContext = new StaticApplicationContext();
+		applicationContext.registerPrototype(beanName, SerializablePerson.class);
+
+		targetSource.setTargetBeanName(beanName);
+		targetSource.setBeanFactory(applicationContext);
+	}
+
+}

spring-context/src/test/resources/org/springframework/aop/target/CommonsPoolTargetSourceTests-context.xml

@@ -0,0 +1,69 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE beans PUBLIC "-//SPRING//DTD BEAN 2.0//EN" "http://www.springframework.org/dtd/spring-beans-2.0.dtd">
+
+<beans>
+
+	<bean id="prototypeTest" class="org.springframework.tests.sample.beans.SideEffectBean" scope="prototype">
+		<property name="count"><value>10</value></property>
+	</bean>
+
+	<bean id="poolTargetSource" class="org.springframework.aop.target.CommonsPoolTargetSource">
+		<property name="targetBeanName"><value>prototypeTest</value></property>
+		<property name="maxSize"><value>25</value></property>
+	</bean>
+
+	<bean id="poolConfigAdvisor" class="org.springframework.beans.factory.config.MethodInvokingFactoryBean">
+		<property name="targetObject"><ref local="poolTargetSource" /></property>
+		<property name="targetMethod"><value>getPoolingConfigMixin</value></property>
+	</bean>
+
+	<bean id="nop" class="org.springframework.tests.aop.interceptor.NopInterceptor"/>
+
+	<!--
+		This will create a bean for each thread ("apartment")
+	-->
+	<bean id="pooled"  class="org.springframework.aop.framework.ProxyFactoryBean">
+		<property name="targetSource"><ref local="poolTargetSource"/></property>
+		<property name="interceptorNames"><value>nop</value></property>
+	</bean>
+
+	<bean id="pooledNoInterceptors"  class="org.springframework.aop.framework.ProxyFactoryBean">
+		<property name="targetSource"><ref local="poolTargetSource"/></property>
+	</bean>
+
+	<bean id="pooledWithMixin" class="org.springframework.aop.framework.ProxyFactoryBean">
+		<property name="targetSource"><ref local="poolTargetSource"/></property>
+		<property name="interceptorNames"><value>poolConfigAdvisor</value></property>
+		<!-- Necessary as have a mixin and want to avoid losing the class,
+			 because there's no target interface -->
+		<property name="proxyTargetClass"><value>true</value></property>
+	</bean>
+
+	<!-- Serialization tests using serializable target and advice -->
+	<bean id="serializableNop" class="org.springframework.tests.aop.interceptor.SerializableNopInterceptor" />
+
+	<bean id="prototypePerson" class="org.springframework.tests.sample.beans.SerializablePerson" scope="prototype"/>
+
+	<bean id="personPoolTargetSource" class="org.springframework.aop.target.CommonsPoolTargetSource">
+		<property name="targetBeanName"><value>prototypePerson</value></property>
+		<property name="maxSize"><value>10</value></property>
+	</bean>
+
+	<bean id="pooledPerson" class="org.springframework.aop.framework.ProxyFactoryBean">
+		<property name="targetSource"><ref local="personPoolTargetSource"/></property>
+		<property name="interceptorNames"><value>serializableNop</value></property>
+
+	</bean>
+
+	<bean id="maxSizePooledPerson" class="org.springframework.aop.framework.ProxyFactoryBean">
+		<property name="targetSource">
+			<bean class="org.springframework.aop.target.CommonsPoolTargetSource">
+				<property name="targetBeanName" value="prototypePerson"/>
+				<property name="maxSize" value="10"/>
+				<property name="maxWait" value="1"/>
+			</bean>
+		</property>
+		<property name="interceptorNames"><value>serializableNop</value></property>
+	</bean>
+
+</beans>