Reduce PlatformTransactionManager lookups

Prior to this commit, cache operations mentioning a qualifier led to
a lookup for the same PlatformTransactionManager over and over again.
The same applied when a transactionManager bean name was specified on
the interceptor.

This commit adds a cache to store the reference of such transaction
managers. As a convenience, the default PlatformTransactionManager is
also initialized if it has not been through configuration.

Issue: SPR-11954

spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java

@@ -33,6 +33,7 @@ import org.springframework.util.StringUtils;
 
 import java.lang.reflect.Method;
 import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Base class for transactional aspects, such as the {@link TransactionInterceptor}
@@ -76,6 +77,9 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init
 			new NamedThreadLocal<TransactionInfo>("Current aspect-driven transaction");
 
 
+	private final ConcurrentHashMap<String, PlatformTransactionManager> transactionManagerCache =
+			new ConcurrentHashMap<String, PlatformTransactionManager>();
+
 	/**
 	 * Subclasses can use this to return the current TransactionInfo.
 	 * Only subclasses that cannot handle all operations in one method,
@@ -328,13 +332,27 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init
 		}
 		String qualifier = txAttr.getQualifier();
 		if (StringUtils.hasLength(qualifier)) {
-			return BeanFactoryAnnotationUtils.qualifiedBeanOfType(this.beanFactory, PlatformTransactionManager.class, qualifier);
+			PlatformTransactionManager txManager = this.transactionManagerCache.get(qualifier);
+			if (txManager == null) {
+				txManager = BeanFactoryAnnotationUtils.qualifiedBeanOfType(
+						this.beanFactory, PlatformTransactionManager.class, qualifier);
+				this.transactionManagerCache.putIfAbsent(qualifier, txManager);
+			}
+			return txManager;
 		}
 		else if (this.transactionManagerBeanName != null) {
-			return this.beanFactory.getBean(this.transactionManagerBeanName, PlatformTransactionManager.class);
+			PlatformTransactionManager txManager  = this.transactionManagerCache.get(this.transactionManagerBeanName);
+			if (txManager == null) {
+				txManager = this.beanFactory.getBean(
+						this.transactionManagerBeanName, PlatformTransactionManager.class);
+				this.transactionManagerCache.putIfAbsent(this.transactionManagerBeanName, txManager);
+			}
+			return txManager;
 		}
 		else {
-			return this.beanFactory.getBean(PlatformTransactionManager.class);
+			// Lookup the default transaction manager and store it for next call
+			this.transactionManager = this.beanFactory.getBean(PlatformTransactionManager.class);
+			return this.transactionManager;
 		}
 	}
 

spring-tx/src/test/java/org/springframework/transaction/interceptor/AbstractTransactionAspectTests.java

@@ -18,7 +18,7 @@ package org.springframework.transaction.interceptor;
 
 import java.lang.reflect.Method;
 
-import junit.framework.TestCase;
+import org.junit.Test;
 
 import org.springframework.dao.OptimisticLockingFailureException;
 import org.springframework.tests.sample.beans.ITestBean;
@@ -33,6 +33,7 @@ import org.springframework.transaction.TransactionSystemException;
 import org.springframework.transaction.UnexpectedRollbackException;
 import org.springframework.transaction.interceptor.TransactionAspectSupport.TransactionInfo;
 
+import static org.junit.Assert.*;
 import static org.mockito.BDDMockito.*;
 
 /**
@@ -47,7 +48,7 @@ import static org.mockito.BDDMockito.*;
  * @author Rod Johnson
  * @since 16.03.2003
  */
-public abstract class AbstractTransactionAspectTests extends TestCase {
+public abstract class AbstractTransactionAspectTests {
 
 	protected Method exceptionalMethod;
 
@@ -69,7 +70,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	}
 
 
-	public void testNoTransaction() throws Exception {
+	@Test
+	public void noTransaction() throws Exception {
 		PlatformTransactionManager ptm = mock(PlatformTransactionManager.class);
 
 		TestBean tb = new TestBean();
@@ -91,7 +93,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	/**
 	 * Check that a transaction is created and committed.
 	 */
-	public void testTransactionShouldSucceed() throws Exception {
+	@Test
+	public void transactionShouldSucceed() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		MapTransactionAttributeSource tas = new MapTransactionAttributeSource();
@@ -116,7 +119,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	 * Check that a transaction is created and committed using
 	 * CallbackPreferringPlatformTransactionManager.
 	 */
-	public void testTransactionShouldSucceedWithCallbackPreference() throws Exception {
+	@Test
+	public void transactionShouldSucceedWithCallbackPreference() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		MapTransactionAttributeSource tas = new MapTransactionAttributeSource();
@@ -135,7 +139,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 		assertFalse(ptm.getStatus().isRollbackOnly());
 	}
 
-	public void testTransactionExceptionPropagatedWithCallbackPreference() throws Throwable {
+	@Test
+	public void transactionExceptionPropagatedWithCallbackPreference() throws Throwable {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		MapTransactionAttributeSource tas = new MapTransactionAttributeSource();
@@ -163,7 +168,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	/**
 	 * Check that two transactions are created and committed.
 	 */
-	public void testTwoTransactionsShouldSucceed() throws Exception {
+	@Test
+	public void twoTransactionsShouldSucceed() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		MapTransactionAttributeSource tas1 = new MapTransactionAttributeSource();
@@ -191,7 +197,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	/**
 	 * Check that a transaction is created and committed.
 	 */
-	public void testTransactionShouldSucceedWithNotNew() throws Exception {
+	@Test
+	public void transactionShouldSucceedWithNotNew() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		MapTransactionAttributeSource tas = new MapTransactionAttributeSource();
@@ -213,7 +220,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 		verify(ptm).commit(status);
 	}
 
-	public void testEnclosingTransactionWithNonTransactionMethodOnAdvisedInside() throws Throwable {
+	@Test
+	public void enclosingTransactionWithNonTransactionMethodOnAdvisedInside() throws Throwable {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		MapTransactionAttributeSource tas = new MapTransactionAttributeSource();
@@ -258,7 +266,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 		verify(ptm).commit(status);
 	}
 
-	public void testEnclosingTransactionWithNestedTransactionOnAdvisedInside() throws Throwable {
+	@Test
+	public void enclosingTransactionWithNestedTransactionOnAdvisedInside() throws Throwable {
 		final TransactionAttribute outerTxatt = new DefaultTransactionAttribute();
 		final TransactionAttribute innerTxatt = new DefaultTransactionAttribute(TransactionDefinition.PROPAGATION_NESTED);
 
@@ -314,35 +323,43 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 		verify(ptm).commit(outerStatus);
 	}
 
-	public void testRollbackOnCheckedException() throws Throwable {
+	@Test
+	public void rollbackOnCheckedException() throws Throwable {
 		doTestRollbackOnException(new Exception(), true, false);
 	}
 
-	public void testNoRollbackOnCheckedException() throws Throwable {
+	@Test
+	public void noRollbackOnCheckedException() throws Throwable {
 		doTestRollbackOnException(new Exception(), false, false);
 	}
 
-	public void testRollbackOnUncheckedException() throws Throwable {
+	@Test
+	public void rollbackOnUncheckedException() throws Throwable {
 		doTestRollbackOnException(new RuntimeException(), true, false);
 	}
 
-	public void testNoRollbackOnUncheckedException() throws Throwable {
+	@Test
+	public void noRollbackOnUncheckedException() throws Throwable {
 		doTestRollbackOnException(new RuntimeException(), false, false);
 	}
 
-	public void testRollbackOnCheckedExceptionWithRollbackException() throws Throwable {
+	@Test
+	public void rollbackOnCheckedExceptionWithRollbackException() throws Throwable {
 		doTestRollbackOnException(new Exception(), true, true);
 	}
 
-	public void testNoRollbackOnCheckedExceptionWithRollbackException() throws Throwable {
+	@Test
+	public void noRollbackOnCheckedExceptionWithRollbackException() throws Throwable {
 		doTestRollbackOnException(new Exception(), false, true);
 	}
 
-	public void testRollbackOnUncheckedExceptionWithRollbackException() throws Throwable {
+	@Test
+	public void rollbackOnUncheckedExceptionWithRollbackException() throws Throwable {
 		doTestRollbackOnException(new RuntimeException(), true, true);
 	}
 
-	public void testNoRollbackOnUncheckedExceptionWithRollbackException() throws Throwable {
+	@Test
+	public void noRollbackOnUncheckedExceptionWithRollbackException() throws Throwable {
 		doTestRollbackOnException(new RuntimeException(), false, true);
 	}
 
@@ -413,7 +430,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	/**
 	 * Test that TransactionStatus.setRollbackOnly works.
 	 */
-	public void testProgrammaticRollback() throws Exception {
+	@Test
+	public void programmaticRollback() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		Method m = getNameMethod;
@@ -447,7 +465,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	 * Simulate a transaction infrastructure failure.
 	 * Shouldn't invoke target method.
 	 */
-	public void testCannotCreateTransaction() throws Exception {
+	@Test
+	public void cannotCreateTransaction() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		Method m = getNameMethod;
@@ -482,7 +501,8 @@ public abstract class AbstractTransactionAspectTests extends TestCase {
 	 * Check that the target method was invoked, but that the transaction
 	 * infrastructure exception was thrown to the client
 	 */
-	public void testCannotCommitTransaction() throws Exception {
+	@Test
+	public void cannotCommitTransaction() throws Exception {
 		TransactionAttribute txatt = new DefaultTransactionAttribute();
 
 		Method m = setNameMethod;

spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java

@@ -16,10 +16,20 @@
 
 package org.springframework.transaction.interceptor;
 
+import static org.junit.Assert.*;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.*;
+
 import java.io.Serializable;
 import java.util.Properties;
 
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
 import org.springframework.aop.framework.ProxyFactory;
+import org.springframework.beans.factory.BeanFactory;
+import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.transaction.PlatformTransactionManager;
 import org.springframework.transaction.TransactionDefinition;
 import org.springframework.transaction.TransactionException;
@@ -34,6 +44,9 @@ import org.springframework.util.SerializationTestUtils;
  */
 public class TransactionInterceptorTests extends AbstractTransactionAspectTests {
 
+	@Rule
+	public final ExpectedException thrown = ExpectedException.none();
+
 	@Override
 	protected Object advised(Object target, PlatformTransactionManager ptm, TransactionAttributeSource[] tas) throws Exception {
 		TransactionInterceptor ti = new TransactionInterceptor();
@@ -63,11 +76,12 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests
 		return pf.getProxy();
 	}
 
-/**
+	/**
 	 * A TransactionInterceptor should be serializable if its
 	 * PlatformTransactionManager is.
 	 */
-	public void testSerializableWithAttributeProperties() throws Exception {
+	@Test
+	public void serializableWithAttributeProperties() throws Exception {
 		TransactionInterceptor ti = new TransactionInterceptor();
 		Properties props = new Properties();
 		props.setProperty("methodName", "PROPAGATION_REQUIRED");
@@ -82,7 +96,8 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests
 		assertNotNull(ti.getTransactionAttributeSource());
 	}
 
-	public void testSerializableWithCompositeSource() throws Exception {
+	@Test
+	public void serializableWithCompositeSource() throws Exception {
 		NameMatchTransactionAttributeSource tas1 = new NameMatchTransactionAttributeSource();
 		Properties props = new Properties();
 		props.setProperty("methodName", "PROPAGATION_REQUIRED");
@@ -106,6 +121,84 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests
 		assertTrue(ctas.getTransactionAttributeSources()[1] instanceof NameMatchTransactionAttributeSource);
 	}
 
+	@Test
+	public void determineTransactionManagerWithQualifierUnknown() {
+		BeanFactory beanFactory = mock(BeanFactory.class);
+		TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory);
+		DefaultTransactionAttribute attribute = new DefaultTransactionAttribute();
+		attribute.setQualifier("fooTransactionManager");
+
+		thrown.expect(NoSuchBeanDefinitionException.class);
+		thrown.expectMessage("'fooTransactionManager'");
+		ti.determineTransactionManager(attribute);
+	}
+
+	@Test
+	public void determineTransactionManagerWithQualifierSeveralTimes() {
+		BeanFactory beanFactory = mock(BeanFactory.class);
+		TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory);
+
+		PlatformTransactionManager txManager = mock(PlatformTransactionManager.class);
+		given(beanFactory.containsBean("fooTransactionManager")).willReturn(true);
+		given(beanFactory.getBean("fooTransactionManager", PlatformTransactionManager.class)).willReturn(txManager);
+
+		DefaultTransactionAttribute attribute = new DefaultTransactionAttribute();
+		attribute.setQualifier("fooTransactionManager");
+		PlatformTransactionManager actual = ti.determineTransactionManager(attribute);
+		assertSame(txManager, actual);
+
+		// Call again, should be cached
+		PlatformTransactionManager actual2 = ti.determineTransactionManager(attribute);
+		assertSame(txManager, actual2);
+		verify(beanFactory, times(1)).containsBean("fooTransactionManager");
+		verify(beanFactory, times(1)).getBean("fooTransactionManager", PlatformTransactionManager.class);
+	}
+
+	@Test
+	public void determineTransactionManagerWithBeanNameSeveralTimes() {
+		BeanFactory beanFactory = mock(BeanFactory.class);
+		TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory);
+		ti.setTransactionManagerBeanName("fooTransactionManager");
+
+		PlatformTransactionManager txManager = mock(PlatformTransactionManager.class);
+		given(beanFactory.getBean("fooTransactionManager", PlatformTransactionManager.class)).willReturn(txManager);
+
+		DefaultTransactionAttribute attribute = new DefaultTransactionAttribute();
+		PlatformTransactionManager actual = ti.determineTransactionManager(attribute);
+		assertSame(txManager, actual);
+
+		// Call again, should be cached
+		PlatformTransactionManager actual2 = ti.determineTransactionManager(attribute);
+		assertSame(txManager, actual2);
+		verify(beanFactory, times(1)).getBean("fooTransactionManager", PlatformTransactionManager.class);
+	}
+
+	@Test
+	public void determineTransactionManagerDefaultSeveralTimes() {
+		BeanFactory beanFactory = mock(BeanFactory.class);
+		TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory);
+
+		PlatformTransactionManager txManager = mock(PlatformTransactionManager.class);
+		given(beanFactory.getBean(PlatformTransactionManager.class)).willReturn(txManager);
+
+		DefaultTransactionAttribute attribute = new DefaultTransactionAttribute();
+		PlatformTransactionManager actual = ti.determineTransactionManager(attribute);
+		assertSame(txManager, actual);
+
+		// Call again, should be cached
+		PlatformTransactionManager actual2 = ti.determineTransactionManager(attribute);
+		assertSame(txManager, actual2);
+		verify(beanFactory, times(1)).getBean(PlatformTransactionManager.class);
+	}
+
+	private TransactionInterceptor createTestTransactionInterceptor(BeanFactory beanFactory) {
+		TransactionInterceptor ti = new TransactionInterceptor();
+		ti.setBeanFactory(beanFactory);
+		ti.setTransactionAttributeSource(new NameMatchTransactionAttributeSource());
+		ti.afterPropertiesSet();
+		return ti;
+	}
+
 
 	/**
 	 * We won't use this: we just want to know it's serializable.