From 4e257243f2dd2fdd5625286d01976b346a74f5d7 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Fri, 4 Jul 2014 16:40:43 +0200 Subject: [PATCH] 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 --- .../interceptor/TransactionAspectSupport.java | 24 ++++- .../AbstractTransactionAspectTests.java | 62 ++++++++---- .../TransactionInterceptorTests.java | 99 ++++++++++++++++++- 3 files changed, 158 insertions(+), 27 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index 67281101c6..c08408ab0c 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/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("Current aspect-driven transaction"); + private final ConcurrentHashMap transactionManagerCache = + new ConcurrentHashMap(); + /** * 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; } } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/AbstractTransactionAspectTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/AbstractTransactionAspectTests.java index b097bf382c..ef5a832d87 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/AbstractTransactionAspectTests.java +++ b/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; diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java index 9b41bc3997..cdb8643659 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java +++ b/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.