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

ChainedTransactionManagerPostProcessor applied too early #10824

Closed
zyro23 opened this issue Oct 4, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@zyro23
Copy link
Contributor

commented Oct 4, 2017

Task List

  • Steps to reproduce provided
  • Stacktrace (if present) provided
  • Example that reproduces the problem uploaded to Github
  • Full description of the issue provided (see below)

Steps to Reproduce

  1. run sample app
  2. see the bootstrap assertion fail

Expected Behaviour

the ChainedTransactionManagerPostProcessor should have been applied and the transactionManager should be instanceof ChainedTransactionManager

Actual Behaviour

org.codehaus.groovy.runtime.powerassert.PowerAssertionError:
assert transactionManager in ChainedTransactionManager
       |                  |
       |                  false
       org.grails.orm.hibernate.GrailsHibernateTransactionManager@1ff542a3

the transactionManager is still a GrailsHibernateTransactionManager.

if you debug into the ChainedTransactionManagerPostProcessor, you can see that it when it runs, there is only one transactionManager bean definition, so it is not applied.

reason is likely, that both the ChainedTransactionManagerPostProcessor and the HibernateDatastoreConnectionSourcesRegistrar (which is responsible for creating the secondary transaction managers) are BeanDefinitionRegistryPostProcessors, but only the ChainedTransactionManagerPostProcessor implements Ordered (with Order.HIGHEST_PRECEDENCE), so it runs first.

i guess the HibernateDatastoreConnectionSourcesRegistrar should also be Ordered and the order of ChainedTransactionManagerPostProcessor should be lower than that?

Environment Information

  • Operating System: win x64
  • Grails Version: 3.3.1, gorm-6.1.7
  • JDK Version: oracle 8u131
  • Container Version (If Applicable): n/a

Example Application

will be referenced in a minute

@zyro23

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

i just tried to come up with a pull-request like this:

// grails-core
public class ChainedTransactionManagerPostProcessor implements BeanDefinitionRegistryPostProcessor, Ordered {
...
    @Override
    public int getOrder() {
        return Ordered.HIGHEST_PRECEDENCE + 200;
    }
...
}

// grails-data-mapping
class HibernateDatastoreConnectionSourcesRegistrar implements BeanDefinitionRegistryPostProcessor, Ordered {
...
    @Override
    int getOrder() {
        return Ordered.HIGHEST_PRECEDENCE + 100
    }
...
}

but testing with the sample-app and local snapshot publications, the ChainedTransactionManagerPostProcessor was still applied before HibernateDatastoreConnectionSourcesRegistrar!

debugging into the spring context refresh logic, i hit the following wall (https://github.com/spring-projects/spring-framework/blob/v4.3.9.RELEASE/spring-context/src/main/java/org/springframework/context/support/PostProcessorRegistrationDelegate.java#L113):

the initially found BeanDefinitionRegistryPostProcessors get ordered.
but subsequent BeanDefinitionRegistryPostProcessor that are added by other BeanDefinitionRegistryPostProcessors are just applied in the order they occur.

as all our BeanDefinitionRegistryPostProcessors are only added after GrailsApplicationPostProcessor.postProcessBeanDefinitionRegistry ran, it seems we cannot rely on correct order...

debugger looks like this:

invokeBeanFactoryPostProcessors:113, PostProcessorRegistrationDelegate (org.springframework.context.support)

// initially added (and ordered) BeanDefinitionRegistryPostProcessors:

postProcessorNames = {String[2]@3859} 
 0 = "org.springframework.context.annotation.internalConfigurationAnnotationProcessor"
 1 = "grailsApplicationPostProcessor"

// new BeanDefinitionRegistryPostProcessors added/applied in arbitrary/wrong order after grailsApplicationPostProcessor.postProcessBeanDefinitionRegistry ran:

postProcessorNames = {String[6]@4582} 
 0 = "org.springframework.context.annotation.internalConfigurationAnnotationProcessor"
 1 = "grailsApplicationPostProcessor"
 2 = "grailsConfigurationClassPostProcessor"
 3 = "chainedTransactionManagerPostProcessor"
 4 = "hibernateDatastoreConnectionSourcesRegistrar"
 5 = "grailsLayoutViewResolverPostProcessor"

and at this point, @graemerocher your feedback would be much appreciated. not sure if/how we should work around this problem or maybe raise a spring issue as this does not look like it is intentional?

@zyro23

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

upstream spring issue raised: https://jira.spring.io/browse/SPR-16043

@zyro23

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

upstream issue fixed/resolved:
spring-projects/spring-framework@7610210

i verified with the sample app above:

  • spring-context:4.3.12.BUILD-SNAPSHOT
  • grails-data-mapping:6.18.BUILD-SNAPSHOT (locally published/tested)
  • grails-core:3.3.2.BUILD-SNAPSHOT (locally published/tested)

assert transactionManager in ChainedTransactionManager passes, it contains two delegate transaction managers (one for each data source).

i will pull-request the changes now. and ill leave it up to the reviewer if they get merged right away (imho should not have a negative impact) or if we wait until spring(-context) is actually released and used.

@zyro23

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

graemerocher pushed a commit that referenced this issue Oct 19, 2017

graemerocher added a commit that referenced this issue Oct 19, 2017

Merge pull request #10828 from zyro23/grails-core-10824
#10824 - lower the precedence of ChainedTransactionManagerPostProcessor

graemerocher added a commit to grails/grails-data-mapping that referenced this issue Oct 19, 2017

Merge pull request #1012 from zyro23/grails-core-10824
grails/grails-core#10824 - HibernateDatastoreConnectionSourcesRegistrar implements Ordered
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.