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

Regression: InjectionMetadata cache needs to handle different bean classes per bean name [SPR-11246] #15871

Closed
spring-projects-issues opened this issue Dec 18, 2013 · 9 comments

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 18, 2013

Ludovic Ronsin opened SPR-11246 and commented

In my project I use a request-scoped factory bean in order to inject the expected implementation between several beans (singletons) depending on request context.
So given a bean name, several candidates can be injected.

The InjectionMetadata cache in AutowiredAnnotationBeanPostProcessor use the bean name as a key. (See AutowiredAnnotationBeanPostProcessor.findAutowiringMetadata(String beanName, Class<?> clazz))
In my case it doesn't work because it doesn't always return the metadata of the corresponding class, instead it will always return the metadata of the first used class.

I attached a test project to reproduce this bug, just run the included junit test.
Seems to be reproducible since 3.2.5.RELEASE and still present on 4.0.0.RELEASE


Affects: 3.2.5, 4.0 GA

Issue Links:

  • #15855 AutowireCapableBeanFactory no longer autowiring since version 3.2.5
  • #16170 Providing unique names to prototype inner beans causes excessive memory and CPU use

Referenced from: commits 5c3f6a1, 5308b3e, 08aa22f, b146074

Backported to: 3.2.7

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 18, 2013

Juergen Hoeller commented

How does this request-scoped FactoryBean work concretely? Wouldn't it be that FactoryBean itself getting autowired, and not the result that it produces?

We weren't aware of (valid) scenarios where the bean class has to be part of the cache key. If you can make a case there, we will have to reconsider...

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 19, 2013

Juergen Hoeller commented

As a defensive measure, we're checking the target class of the pre-cached InjectionMetadata now and re-cache if it differs from the current bean class. So we're still caching by bean name but will always cache injection metadata for the latest bean class.

Note that we still recommend to keep the bean class stable per bean name. There are usually better ways to express bean arrangements with differing classes.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 20, 2013

Ludovic Ronsin commented

Sorry for being a bit late on this, here is how to reproduce the bug https://github.com/zeludo/spring-bug-SPR-11246-reproduction

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 20, 2013

Juergen Hoeller commented

I see, so you're using a custom factory method that dynamically creates instances of different classes based on some runtime condition. This is a little unusual but indeed arguably a valid scenario.

So as of the current 4.0.1/3.2.7 snapshots, we're caching metadata for the latest bean class in use for each bean name. This won't be caching all your bean classes side by side but should at least work correctly for such a changing-classes-per-bean-name scenario.

Feel free to give one of those snapshots a try (see http://projects.spring.io/spring-framework/ for repo details)...

Juergen

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2014

Lari Hotari commented

I guess the regression was caused by changes made for #15655 .

This bug is causing also problems in Grails when service beans are wrapped with TransactionProxyFactoryBean .

BeanDefinitionValueResolver's resolveValueIfNecessary method uses "(inner bean)" as bean name prefix which I guess causes bean name conflicts in the cache:

return resolveInnerBean(argName, "(inner bean)", bd);

Grails bug is http://jira.grails.org/browse/GRAILS-10991

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2014

Juergen Hoeller commented

Lars, we're consistently adapting the inner bean names with a suffix now, so this part should work fine.

Please test against a recent 3.2.7 and/or 4.0.1 snapshot and let us know whether it works for Grails there...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 16, 2014

Lari Hotari commented

@Juergen, I cannot reproduce the Grails bug even on Grails 2.3.5.BUILD-SNAPSHOT that uses Spring 3.2.6 .

I found #15742 in Comparing v3.2.5.RELEASE...v3.2.6.RELEASE.
I wonder if #15742 related changes could fix the problems in Grails.

I'm just wondering what the change could be. I haven't yet tested with Grails 2.3.4 and just upgrading Spring to 3.2.6 so that I could be sure that the change in Spring fixes the problem that I can reproduce with the test app that's attached to GRAILS-10991 .

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 16, 2014

Lari Hotari commented

I can confirm that the Grails related @Autowired problem (http://jira.grails.org/browse/GRAILS-10991) is already fixed in Spring 3.2.6.RELEASE. There is a more information about this in GRAILS-10991 issue comments.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 16, 2014

Lari Hotari commented

These changes are already in Spring 3.2.6.RELEASE: v3.2.5.RELEASE...v3.2.6.RELEASEdiff-8f48c8608e60a4e13d2abb5d99d2f461R239 . Commit is 6bed180 which references #15742

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