@Transactional qualifier is ignored by TransactionAspectSupport if default transaction manager is set [SPR-12541]

[spring-issuemaster]

Donnchadh O Donnabhain opened SPR-12541 and commented

Status Quo

#16570 introduced caching of the default PlatformTransactionManager in TransactionAspectSupport.

Specifically, the transactionManager instance field is now set within determineTransactionManager(). In subsequent invocations of determineTransactionManager(), the following if-logic at the beginning of the method results in the cached default transaction manager being returned instead of the transaction manager with the specified qualifier.

if (this.transactionManager != null || this.beanFactory == null || txAttr == null) {
    return this.transactionManager;
}

Note, however, that the default transaction manager may have been set in Java Config due to the use of TransactionManagementConfigurer, or it may have been set in XML config via TransactionProxyFactoryBean.setTransactionManager(). Therefore, the behavior reported in this issue is not limited to caching of the default transaction manager in determineTransactionManager(). Rather, the behavior can be experienced any time the transactionManager field in TransactionAspectSupport has been set.

---

Steps to Reproduce

Given

@Transactional
public void doFoo() {
    // ...
}

@Transactional("transactionManager2")
public void doBar() {
    // ...
}

and

service.doFoo();
service.doBar();

... the default transaction manager will be used for invocations of service.doBar() rather than transactionManager2.

---

Further Resources

See discussion on GitHub.

---

Affects: 4.1 GA

Issue Links:

• #11812 Potential null-pointer in TransactionAspectSupport.determineTransactionManager() ("depends on")
• #8635 support for multiple transaction managers with @Transactional / tx:annotation-driven ("depends on")
• #16570 Reduce PlatformTransactionManager lookups in TransactionAspectSupport ("depends on")
• #17123 AnnotationTransactionAspect retains reference to JpaTransactionManager from closed context
• #17207 Wrong TransactionManager selected when mixing @Transactional with and w/o qualifier
• #17178 Regression in TransactionAspectSupport.determineTransactionManager(…)
• #19080 Transaction manager bean in TransactionInterceptor retained after JUnit test class completes

Referenced from: commits 961574b, 4a0ac97, cec26e9

0 votes, 5 watchers

[spring-issuemaster]

Sam Brannen commented

This issue is related to #17123.

[spring-issuemaster]

Stéphane Nicoll commented

I don't see the link Sam. It's liked to #16570 but I don't see the relationship with #17123: the problem here is about the qualifier not being applied anymore, not a side-effect of caching.

[spring-issuemaster]

Sam Brannen commented

The code block that Donnchadh questions on GitHub hasn't changed recently.

if (this.transactionManager != null || this.beanFactory == null || txAttr == null) {
    return this.transactionManager;
}

What has changed is that the default transaction manager is now cached in an instance variable within the aspect:

else {
    // Lookup the default transaction manager and store it for next call
    this.transactionManager = this.beanFactory.getBean(PlatformTransactionManager.class);
    return this.transactionManager;
}

The fact that this.transactionManager is now cached is also the cause of the regression mentioned in #17123. That's why I linked that issue as well.

I haven't investigated it fully, but it appears that the issue Donnchadh has reported would actually have been present with JavaConfig prior to Spring 4.1 if the developer implemented TransactionManagementConfigurer to declare the default transaction manager, since both AspectJTransactionManagementConfiguration and ProxyTransactionManagementConfiguration invoke TransactionAspectSupport.setTransactionManager(PlatformTransactionManager).

In summary, I don't believe that this is entirely a new issue. It's just that setting the transactionManager instance variable in TransactionAspectSupport.determineTransactionManager() now makes the issue occur in more use cases. So the solution is likely to rework the logic in the (this.transactionManager != null || this.beanFactory == null || txAttr == null) if-condition so that a qualifier is always honored if present.

In addition, I think we should change all of the documentation involving this particular transactionManager to refer to it as the default transaction manager (as you appropriately did in your comments "Lookup the default transaction manager and store it for next call"). Otherwise, it is not clear what the intent of that instance field is.

Does that make sense?

Cheers,

Sam

[spring-issuemaster]

Sam Brannen commented

With regard to the term caching, I consider both the transactionManagerCache and transactionManager fields within TransactionAspectSupport to be a form of reference caching since the same aspect is used for every advised class within the class loader.

I hope that clears up any confusion I may have caused with the word "caching". ;)

[spring-issuemaster]

Sam Brannen commented

Stéphane Nicoll, when you fix this bug, please keep in mind that the regression naturally applies not only to a qualified transaction manager but also to a named transaction manager.

For example, the AnnotationDrivenBeanDefinitionParser sets the transactionManagerBeanName property of TransactionAspectSupport via reflection.

Update to my previous comment: the solution is likely to rework the logic in the (this.transactionManager != null || this.beanFactory == null || txAttr == null) if-condition so that both qualified and named transaction managers are returned if a qualifier or transactionManagerBeanName has been specified.

[spring-issuemaster]

Stéphane Nicoll commented

I confirm the problem with the qualifier but I don't see the problem with "named" transaction manager actually. If the default transaction manager is set, then I don't see what the "default transaction manager bean name" has to do with this: if you don't have a named transaction manager, you'll never go in that statement that caches the transaction manager.

[spring-issuemaster]

Stéphane Nicoll commented

#16570 introduced a regression that when the "default" transaction
manager is cached, qualified transaction managers are not taken into
account anymore.

This commit rework the "determineTransactionManager" condition to
favour qualifier and "named" transaction managers. If none of these apply,
the default transaction manager is used as it should.

Also reworked the caching infrastructure so that a single cache holds
all transaction manager instances.

[spring-issuemaster]

Stéphane Nicoll commented

This fix actually broke certain use cases where it is actually legit to return a null PlatformTransactionManager

[spring-issuemaster]

Stéphane Nicoll commented

Juergen Hoeller I think this one deserves a closer look on your end. Thanks!

[spring-issuemaster]

Stéphane Nicoll commented

This fix broke yet another use case. If you are upgrading to Spring Framework 4.1.4 and your application throws the following exception, you may be affected:

org.springframework.beans.factory.NoUniqueBeanDefinitionException: No qualifying bean of type [org.springframework.transaction.PlatformTransactionManager] is defined: expected single matching bean but found 2: transactionManager,anotherTransactionManager
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBean(DefaultListableBeanFactory.java:365)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBean(DefaultListableBeanFactory.java:331)
	at org.springframework.transaction.interceptor.TransactionAspectSupport.determineTransactionManager(TransactionAspectSupport.java:370)
	at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:271)
	at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:96)

An easy workaround to this issue is to mark your "main" PlatformTransactionManager as Primary. Your main transaction manager is the one you are using without a qualifier. To do so, add @Primary to your @Bean declaration if you're using java config or set the primary attribute to true if you're using XML configuration.

A fix for this issue will be available in 4.1.5. If you're still affected with this workaround please create a separate issue with more details. Thanks!

[spring-issuemaster]

Chad Wilson commented

Thanks for noting the workaround here Stéphane - confirmed that this resolve the issue for us on 4.1.4.

[spring-issuemaster]

Stéphane Nicoll commented

Thanks for the feedback Chad, much appreciated!