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

A BeanPostProcessor should not create bean definitions #3844

Closed
snicoll opened this issue Jul 13, 2022 · 8 comments · Fixed by #3877
Closed

A BeanPostProcessor should not create bean definitions #3844

snicoll opened this issue Jul 13, 2022 · 8 comments · Fixed by #3877

Comments

@snicoll
Copy link
Member

snicoll commented Jul 13, 2022

Right now, MessagingBeanPostProcessor is creating bean definitions. In order to do that, it has to inject the BeanFactory and downcast it to a BeanRegistry, see

As a side effect of this, we might be able to get rid of that @Bean lookup. @Bean is a configuration class parsing concern and shouldn't be relied upon outside of it.

@snicoll snicoll added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Jul 13, 2022
@jhoeller
Copy link

I would generally recommend an approach that does not require an individual runtime bean registration per endpoint, e.g. a central endpoint registry bean that accepts every such endpoint (as e.g. in JmsListenerAnnotationBeanPostProcessor).

Most importantly, bean definitions are not meant to be registered with an existing instance: A specified bean supplier is meant to create a new instance for every call, or at least perform a fresh lookup for an existing bean instance every time. For the purposes here, it seems you could be using registerSingleton with such an existing endpoint instance. That would avoid the registry downcast since registerSingleton is designed to be available on ConfigurableBeanFactory, not requiring internal metadata management in the core container but rather expressing the intent of making a pre-existing bean instance available.

@artembilan
Copy link
Member

It always worked that way and I guess that's just because there is no way in Spring to register several beans at once.
However I don't think that with our current simple "annotation on method" approach end-user would like to do anything else.
The solution here really pre-dates what you show in the JmsListenerAnnotationBeanPostProcessor, however I don't think a central registry is what we'd like to pursue here.
We have many more stuff around (JMX, for example, integration graph), where we indeed need an independent bean for the endpoint based on a messaging annotation.
I probably can rework the logic to the registerSingleton(), but as far as I remember there were some problems, e.g. with removal endpoints at runtime.

All of these looks too noise just for one @Bean I exposed for native...

Just a side note: we also have a IntegrationFlowBeanPostProcessor which parses an IntegrationFlow bean and registers many other beans (or reuse existing) according ti its tree-like structure.
We also support over there a runtime flows registration (and removal), which definitely might not work in native images. So, not a question here.

What I'm trying to say that registration (and removal) of BeanDefiniiton at runtime was really great feature for us for all of these years so it sounds a bit frustrating that some limitations in AOT force us for an overhaul which may lead not where because there is no way to register several bean from one definition in Spring...

As a middle ground I can look into a MergedBeanDefinitionPostProcessor for the @Bean with messaging annotations use-case.
This way I won't parse a @Bean and just register endpoints according messaging annotations.
The mentioned MessagingAnnotationPostProcessor will be left just for POJO methods, but still free from a @Bean parsing.

Thank you for your patience!

@jhoeller
Copy link

Runtime registration and removal of bean definitions isn't going away but unfortunately it is just not ideal for AOT processing, by the very nature of the approach. From my perspective, we should use the opportunity to streamline our registration arrangements for better predictability at build time.

The post-processor interfaces always hinted at this by not exposing registry references where they are not meant to be used... but of course downcasts always worked, bypassing that guidance that we had in the SPI design. A BeanPostProcessor kicks in pretty late in the game, on creation of specific bean instances, at a point when the factory is already optimized for complete bean definition metadata. A BeanDefinitionRegistryPostProcessor would kick in much earlier, operating on the complete initial set of bean definitions and being able to derive further bean definitions, just like configuration class processing itself does.

Runtime features such as registerSingleton are in a different ballpark since they do not affect the metadata management, just instance availability for state management and dependency injection purposes. If late registrations need to happen, it's highly recommended to use either a shared registry bean that accepts those individual registrations or registerSingleton but not late bean definition metadata registrations. That's generally sensible and also naturally compatible with the AOT approach.

@artembilan
Copy link
Member

Thank you, @jhoeller , for kind explanation!

I will look into your suggestions for the next milestone.

In addition, I'll investigate if that would be possible to generate some code for those endpoint beans via BeanRegistrationAotProcessor when we have messaging annotations on them: https://docs.spring.io/spring-integration/docs/current/reference/html/configuration.html#annotations_on_beans.
This way we would have an optimized configuration for native image, plus there won't be need for that @Bean reflection.
Of course, it might be possible if BeanRegistrationAotContribution allows to generate the code for additional beans...

@snicoll
Copy link
Member Author

snicoll commented Jul 14, 2022

All of these looks too noise just for one @bean I exposed for native...

For the record, this was just the trigger that made something totally unrelated to AOT apparent.

a bit frustrating that some limitations in AOT force us for an overhaul

I don't know which limitations you're talking about. None of this here has anything to do with AOT. The main purpose of this issue is the registration of bean definitions in a BeanPostProcessor. And if we manage to find another way to implement this feature, then the @Bean lookup will probably go away as a side-effect.

In addition, I'll investigate if that would be possible to generate some code

Please don't. For the first version, we really want the AOT engine to process contexts for various use cases in a generic fashion. Code should be written only for very specific use cases and we should do our best to improve the engine if that's not the case. Thanks!

@artembilan
Copy link
Member

No problem, @snicoll ! The code generation is still dark matter for me anyway ☺️

Then I will look into a BeanDefinitionRegistryPostProcessor for this @Bean with Messaging Annotations to register that mentioned extra endpoint bean definition.

@garyrussell
Copy link
Contributor

Most importantly, bean definitions are not meant to be registered with an existing instance: A specified bean supplier is meant to create a new instance for every call...

For the purposes here, it seems you could be using registerSingleton with such an existing endpoint instance.

@jhoeller One benefit of using registerBean with a supplier returning a single instance is that the instance is initialized when retrieved. With registerSingleton, we have to cast the BF to an AutowireCapableBeanFactory (e.g. CLBF) to initialize it afterwards.

To help the transition back to registering singletons, perhaps a new API could be provided that also initializes the bean; e.g.

<T> T initializeAndAddBean(String name, T instance);

Even better if it was available on a top level interface (e.g. ApplicationContext) to avoid having to cast (even if the default implementation throws an UnsupportedOperationException, or the initialize part is a no-op there).

@artembilan
Copy link
Member

Another side of this medal is a destroySingleton(String beanName) which is not available in the ConfigurableBeanFactory - just on the DefaultSingletonBeanRegistry.

I don't mind any discussion at any place, but it looks like we are hijacking this issue for other mater, than a concern around bean definition registration in the BeanPostProcessor.

I'm looking these days into making the mentioned MessagingAnnotationPostProcessor as a BeanDefinitionRegistryPostProcessor as well to process @Bean with messaging annotation on early stage.
The work reminds me our old good XML parsers, but instead of Element I parse Annotation 😄 .
Well, doesn't look like the solution is fast enough, so no any promises for the milestone next week...

@artembilan artembilan added this to the Backlog milestone Jul 18, 2022
@artembilan artembilan added in: core type: enhancement and removed status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Jul 18, 2022
@artembilan artembilan modified the milestones: Backlog, 6.0.0-M5 Aug 21, 2022
artembilan added a commit that referenced this issue Aug 21, 2022
Fixes #3844

* Make `MessagingAnnotationPostProcessor` as a `BeanDefinitionRegistryPostProcessor`
to process bean definitions as early as possible and register respective messaging
components at that early phase
* Make bean definitions parsing logic optional for AOT and native mode since beans
have bean parsed during AOT building phase
* Introduce a `BeanDefinitionPropertiesMapper` for easier mapping
of the annotation attributes to the target `BeanDefinition`
* Remove `@Bean`-related logic from method parsing process
* Change the logic for `@Bean`-based endpoint bean names:
since we don't deal with methods on the bean definition phase, then method name
does not make sense.
It even may mislead if we `@Bean` name is based on a method by default, so we end up
with duplicated word in the target endpoint bean name.
Now we don't
* Fix `configuration.adoc` respectively for a new endpoint bean name logic
* In the end the new logic in the `AbstractMethodAnnotationPostProcessor`
is similar to XML parsers: we feed annotation attributes to the
`AbstractStandardMessageHandlerFactoryBean` impls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants