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

Clarify configuration lite support and offer an opt-in option #22461

Closed
snicoll opened this issue Feb 24, 2019 · 14 comments
Closed

Clarify configuration lite support and offer an opt-in option #22461

snicoll opened this issue Feb 24, 2019 · 14 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Feb 24, 2019

The Spring Boot team has been investigating startup time improvements by not relying on the proxy capabilities of configuration classes. Turns out that not using that stuff (basically switching all auto-configurations and related imports to "lite mode") makes quite a difference spring-projects/spring-boot#9068 (comment)

I am not a big fan that we were looking for @Bean methods in any component, treating them in lite mode if they don't have the @Configuration stereotype. I was wondering if we would be open to revisit this in the 5.2 line.

Being able to opt-out from the proxy mode using the @Configuration stereotype would be ideal. We are not keen to remove @Configuration and use the lite mode as it stands as:

  • That's not representative of what a good citizen of the framework should do (using the right stereotype)
  • We have an annotation processor that identifies configuration classes to extract conditions so that we can back off (without loading the ASM metadata or the class) when a static condition does not match (i.e. class not on the classpath)

Having a way to opt-out would be also a nice opportunity to clarify the behaviour and stop scanning components for bean factory methods.

@wilkinsona
Copy link
Member

I'd like to echo Stephane's dislike of using @Bean on any component. I find the behaviour surprising and it's caused confusion in the past. I too would prefer that you have to opt in to @Bean method support via a stereotype annotation that explicitly indicates that a class is a configuration class. Once you've opted in to being a configuration class and @Bean methods defining beans, we'd then need a way to opt out of proxying. Given that the use of proxies is more than an implementation detail and is already something that a user needs to think about (@Configuration classes cannot be final, for example), I wonder if a proxy attribute on @Configuration that defaults to true would suffice here.

In terms of the benefits of not proxying @Configuration classes, the table in the linked Spring Boot issue shows some of the performance benefits in terms of startup time. There are also other benefits with both metaspace and code cache usage being reduced due to fewer classes being loaded and subsequently processed by the JIT.

@philwebb raised another benefit of being able to identify a configuration class where @Bean methods should be supported but where proxying should not be used. It would allow an application to be started in a mode where the configuration classes were proxied anyway, but purely for the purpose of catching and warning about accidental interdependencies between @Bean methods. I modified Framework to use this technique to identify @Configuration classes in Spring Boot that currently rely on proxying and it proved very useful.

@jhoeller jhoeller self-assigned this Feb 25, 2019
@jhoeller jhoeller added this to the 5.2 M1 milestone Feb 25, 2019
@snicoll snicoll added type: enhancement A general enhancement and removed for: team-attention labels Mar 4, 2019
@sbrannen
Copy link
Member

sbrannen commented Mar 5, 2019

While discussing this with @wilkinsona, I proposed we name the boolean flag in @Configuration something like proxyMethodInvocations = false analogous to proxyTargetClass in other places.

He came back with "proxyBeanMethodInvocations or perhaps proxyBeanMethods to make it clear that it’s only @Bean methods that are affected."

I tend to like @Configuration(proxyBeanMethods = false) to turn off the default behavior.

@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Mar 8, 2019
@jhoeller
Copy link
Contributor

jhoeller commented Mar 8, 2019

I have an implementation ready to be committed now, using a proxyBeanMethods flag as suggested above. (Even if this is technically not "proxying" in the AOP sense but rather method interception through overriding in the same instance, I suppose it can still be called proxying in a general sense.)

Whatever we may feel about it, I can't see how we'd limit @Bean methods to @Configuration classes at this point... without breaking plenty of people who intentionally or unintentionally don't use the @Configuration stereotype for some of their @Bean-declaring classes. This has been out there for 10 years and I am not inclined to break this, even if it is now possible to effectively get the same behavior using a new flag on @Configuration... Arguably the very fact that this is now a well-defined mode of regular configuration classes makes it easier to understand that the same behavior is applicable to individual @Bean methods outside of configuration classes as well, with less surprise that there is no method interception through CGLIB subclasses there, and that this is a proper variant of operation.

As for the annotation detection overhead, let's tackle this along with our other issues for it. Once we have a method-level indication in the components index, we'll get efficient skipping not only for @Bean checks but also for @EventListener, @Scheduled, @Transactional, the caching annotations etc. And if we introduce corresponding programmatic hints at the bean definition level, those should be equally applicable to the detection of all those method-level annotations so that we never reflectively iterate the methods of the affected component classes - not for @Bean detection and not for AOP proxying either.

@wilkinsona
Copy link
Member

Really pleased to see this land. Thanks!

@sbrannen
Copy link
Member

@jhoeller, shall we reopen this issue to update the Reference Manual, or are you planning on doing that in a new issue?

@wilkinsona
Copy link
Member

wilkinsona commented Mar 11, 2019

I've noticed an interesting side-effect of this change that I hadn't anticipated.

When I switch from @Configuraton to @Configuration(proxyBeanMethods = false) the bean definitions for the class's @Bean methods change from having a Class beanClass to having a String beanClass. This became apparent due to FactoryBeans now being instantiated more aggressively as this type determination shortcut no longer happens. In the scenario where the change became apparent, this change in behaviour results in a BeanCurrentlyInCreationException. I can work around the problem by enabling bean method proxying on the @Configuration class that defines the factory bean but it feels like that shouldn't be necessary.

@jhoeller
Copy link
Contributor

@wilkinsona A strange effect indeed, I'll have a look at it this morning... I'm into that area anyway with the remainder of #22420.

@jhoeller
Copy link
Contributor

@wilkinsona Since you're seeing the beanClass populated in the first place, I suppose those are static @Bean methods? Otherwise it would use factoryBeanName, pointing to the configuration instance.

@wilkinsona
Copy link
Member

wilkinsona commented Mar 12, 2019

No, the methods aren't static. Let me see if I can boil things down into a handful of classes that reproduce the problem.

@jhoeller
Copy link
Contributor

jhoeller commented Mar 12, 2019

I can see why this would happen: The enhancement step for the generated subclass resolves the bean Class first, and we're skipping this completely for proxyBeanMethods=false. So you're probably talking about the bean definition for the configuration class itself, not for the derived @Bean definitions... This all makes sense then, no need to reproduce anything on your end :-)

@wilkinsona
Copy link
Member

Thanks. Yes, now I look more closely, this is indeed the bean definition for the configuration class itself. Sorry for the inaccuracy in the description above.

@jhoeller
Copy link
Contributor

The part with static @Bean methods is a nice side improvement in any case: If we got the bean class for the configuration class resolved already, we should pass it on to derived bean definitions for static methods. At this point we're always just setting the bean class name for those, losing the resolved Class.

@jhoeller
Copy link
Contributor

A configuration's bean Class gets resolved consistently as part of #22420 now.

bclozel added a commit to bclozel/spring-framework that referenced this issue Apr 3, 2019
This commit changes the main configuration classes for Spring MVC and
Spring WebFlux to not call other bean methods when setting up the web
infrastructure. This allows configuration classes extending
`DelegatingWebFluxConfiguration` and `DelegatingWebMvcConfiguration`
to opt-in the lite-mode, as introduced in spring-projectsgh-22461.
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

4 participants