Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnnotationTransactionAttributeSource should never apply to GroovyObject methods [SPR-14095] #18667

Closed
spring-projects-issues opened this issue Mar 29, 2016 · 4 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 29, 2016

László Csontos opened SPR-14095 and commented

How to reproduce

  1. Pull the branch in the reference URL
  2. Execute test org.springframework.test.context.junit4.spr14095.AnnotationTransactionAttributeSourceTest
% ./gradlew -D:spring-test:test.single=AnnotationTransactionAttributeSourceTest spring-test:clean spring-test:test

Issue

After deep investigation how Spring TX and Groovy work together, I found out that TransactionInterceptor gets applied to methods inherited from groovy.lang.GroovyObject.

Switching on debugging reveals that.

16:54:16.171 [localhost-startStop-1] DEBUG o.s.t.a.AnnotationTransactionAttributeSource - Adding transactional method 'LanguageServiceImpl.setMetaClass' with attribute: PROPAGATION_REQUIRED,ISOLATION_DEFAULT; ''
16:54:16.993 [localhost-startStop-1] DEBUG o.s.t.a.AnnotationTransactionAttributeSource - Adding transactional method 'UserServiceImpl.setMetaClass' with attribute: PROPAGATION_REQUIRED,ISOLATION_DEFAULT; ''
16:54:17.118 [localhost-startStop-1] DEBUG o.s.t.a.AnnotationTransactionAttributeSource - Adding transactional method 'CompanyServiceImpl.setMetaClas' with attribute: PROPAGATION_REQUIRED,ISOLATION_DEFAULT; ''
16:54:20.905 [localhost-startStop-1] DEBUG o.s.t.a.AnnotationTransactionAttributeSource - Adding transactional method 'CustomTermServiceImpl.setMetaClass' with attribute: PROPAGATION_REQUIRED,ISOLATION_DEFAULT; ''
16:54:20.913 [localhost-startStop-1] DEBUG o.s.t.a.AnnotationTransactionAttributeSource - Adding transactional method 'ContentItemServiceImpl.setMetaClass' with attribute: PROPAGATION_REQUIRED,ISOLATION_DEFAULT; ''

While stressing the system with requests, it can also be seen, that upon calling these internal Groovy methods, Spring TX perform commits, which eventually degrades performance. The stack trace below was captured from the execution of the following piece of code.

@Override
Authentication authenticate(Authentication authentication) throws AuthenticationException {
  String authenticationToken = authentication.getPrincipal()
  User user = userService.findByAuthenticationToken(authenticationToken as String)
  ...
}

Before userService.findByAuthenticationToken() would have been executed, the Groovy runtime called getMetaClass() internally as part of call() in order to fetch the meta data of the target object.

"http-nio-8080-exec-10" #89 daemon prio=5 os_prio=0 tid=0x00007f33e0009000 nid=0x3b5c runnable [0x00007f33f52b9000]
   java.lang.Thread.State: RUNNABLE
    at java.net.SocketInputStream.socketRead0(Native Method)
    ...
    at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:2447)
    ...
    at com.zaxxer.hikari.proxy.ConnectionProxy.commit(ConnectionProxy.java:300)
    ...
    at org.springframework.transaction.support.AbstractPlatformTransactionManager.processCommit(AbstractPlatformTransactionManager.java:757)
    at org.springframework.transaction.support.AbstractPlatformTransactionManager.commit(AbstractPlatformTransactionManager.java:726)
    at org.springframework.transaction.interceptor.TransactionAspectSupport.commitTransactionAfterReturning(TransactionAspectSupport.java:478)
    at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:272)
    at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:95)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
    at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:207)
    at com.sun.proxy.$Proxy144.getMetaClass(Unknown Source)
    at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.nonParamCheck(PogoMetaMethodSite.java:79)
    at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.checkCall(PogoMetaMethodSite.java:92)
    at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.call(PogoMetaMethodSite.java:66)
    at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:122)
    at app.web.security.AppAuthenticationProvider.authenticate(AppAuthenticationProvider.groovy:39)
    at org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:156)

Workaround

Currently the workaround is in the client's system is to override AnnotationTransactionAttributeSource and customize method getTransactionAttribute() in the following way.

public class GroovyAwareAnnotationTransactionAttributeSource extends AnnotationTransactionAttributeSource {

    private static final Logger log = LoggerFactory.getLogger(GroovyAwareAnnotationTransactionAttributeSource.class);

    @Override
    public TransactionAttribute getTransactionAttribute(Method method, Class<?> targetClass) {
        if (!ClassUtils.isUserLevelMethod(method)) {
            if (log.isTraceEnabled()) {
                log.trace("Transaction skipped for non-userlevel method {}", method.toString());
            }

            return null;
        }

        return super.getTransactionAttribute(method, targetClass);
    }

}

After that some manual tweaking is also necessary to replace tx:annotation-driven/.

<!-- Replace <tx:annotation-driven/> with manual config in order to work a Spring bug around -->
<aop:config/>

<bean id="transactionAttributeSource" class="app
.core.tx.GroovyAwareAnnotationTransactionAttributeSource"/>

<bean id="transactionInterceptor" class="org.springframework.transaction.interceptor.TransactionInterceptor">
  <property name="transactionManagerBeanName" value="transactionManager"/>
  <property name="transactionAttributeSource" ref="transactionAttributeSource"/>
</bean>

<bean class="org.springframework.transaction.interceptor.BeanFactoryTransactionAttributeSourceAdvisor">
  <property name="adviceBeanName" value="transactionInterceptor"/>
  <property name="transactionAttributeSource" ref="transactionAttributeSource"/>
</bean>

Affects: 4.1.7

Reference URL: https://github.com/laszlocsontos/spring-framework/tree/SPR-14095

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 29, 2016

Juergen Hoeller commented

We exclude GroovyObject methods for a couple of other TransactionAttributeSource implementations already, but not for AnnotationTransactionAttributeSource since we expected it to be clearly annotation-driven. However, I suppose you are using class-level @Transactional markers? In this case, Groovy methods would indeed be considered transactional, I suppose... We can certainly fine-tune this for 4.3.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2016

László Csontos commented

Hi Jürgen,

Thanks for your swift response!
I've opened a PR: #1020.

Cheers,
László

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2016

Juergen Hoeller commented

I've applied this at a slightly different level: specifically for class-level attributes in AbstractFallbackTransactionAttributeSource.computeTransactionAttribute. For explicitly annotated methods, let's rather consider those applicable in any case, even if not user-level.

As for the test setup in your pull request, it is rather unfortunate to have to turn the entire spring-test module over to cross-compilation here. I've used a unit test arrangement instead, using a custom Java implementation of the GroovyObject interface.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2016

Juergen Hoeller commented

I've applied analogous changes to our AbstractFallbackCacheOperationSource, making AnnotationCacheOperationSource behave the same way with respect to synthetic Groovy methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.