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

Deterministic and JVM-independent @Bean registration order within Class-reflected configuration classes [SPR-14505] #19074

Closed
spring-projects-issues opened this issue Jul 21, 2016 · 4 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jul 21, 2016

Juergen Hoeller opened SPR-14505 and commented

As noted in #19073, the JVM's Class.getDeclaredMethods() may return the methods in any order... not just dependent on the bytecode but even dependent on the particular JVM run, which means it might differ on re-execution of the very same classes on the very same JVM.

As a consequence, we should consider some deterministic order that we can enforce within the framework, e.g. sorting the @Bean methods within each configuration class before registering bean definitions for them.

However, we have two modes of registration: ASM-based, automatically applied for bean definitions registered with class name (and therefore also for classpath scanning); and reflection-based, automatically applied for bean definitions registered with a pre-resolved Class. ASM-based parsing follows source-code declaration order, while reflection-based parsing follows the Class.getDeclaredMethods() order of the particular JVM. While the latter would benefit from a custom deterministic order that we're enforcing, that'd be a step backwards for our ASM mode since source-code declaration order is arguably ideal there.


Affects: 4.3.1

Issue Links:

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

The best compromise from my perspective: Let's preserve the original declaration order that we get from the ASM variant and simply accomplish it on a best-effort basis for the Class-originated code path as well.

For configuration classes with more than one @Bean method, we check for whether we're operating against Class reflection now, and if yes, we try to load the same class via ASM and read the @Bean metadata from there instead. If this fails, we simply proceed with the reflection metadata that we started with.

Given the low impact of only trying this for selected configuration classes and defensively falling back if not possible, and all of this only mattering for Class-provided setup (i.e. tests and small-scale programmatic bootstrap) to begin with, I'm inclined to backport this to 4.3.6 as well.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Artem Bilan, good catch there with expectations that BeanDefinition.getSource() is StandardMethodMetadata. While I would not recommend to make such hard assumptions, we can nevertheless restore compatibility with such casts and take the original MethodMetadata objects from our reflective introspection, literally just using the ASM-introspected variant for ordering.

As for scenarios like yours, for maximum efficiency, I'd recommend a conditional downcast: If it is StandardMethodMetadata, do a downcast and take the presolved Class; if not, take the return type name and resolve it yourself (like you do in your fix).

@spring-projects-issues
Copy link
Collaborator Author

Artem Bilan commented

Juergen Hoeller,

Thank you very much for all your wisdom and for this fix in particular!
Yes, indeed I have met before not one time that I need @DependsOn because of wrong methods introspection order from time to time.

As for your idea to restore compatibility. Well, I don't think so. This is a matter of Framework code, not sure that it would be so critical for target applications.
Even if that is broken like in our Spring Integration case, it would be better to fix it according the current state and get a gain of the original premise of the fix for this JIRA.
That's why I didn't talk to you about any regressions after this.
I'm fully sure that my fix is compatible with the previous versions because MethodMetadata is a super interface, so we are safe.

Yes, sure thing about your "maximum efficiency" suggestion. No reason to load class one more time if that is done by the StandardMethodMetadata :) !

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

That efficiency of reusing pre-loaded Class references is also why I'd like to restore StandardMethodMetadata exposure there: We've already loaded the Method handle and all of its reflective metadata, so we should keep exposing it. Easy enough to do, after all: We can take the order from the ASM method metadata but select the corresponding MethodMetadata handles from our original set of methods, reusing what we have already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants