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

add InitDestroyAnnotationBeanPostProcessor to BeanFactoryInitializationAotProcessor #31686

Closed
wants to merge 1 commit into from

Conversation

bdshadow
Copy link
Contributor

closes gh-30755

…onAotProcessor

closes spring-projectsgh-30755

Signed-off-by: Dmitrii Bocharov <bdshadow@gmail.com>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 26, 2023
@bdshadow
Copy link
Contributor Author

I understand that InitDestroyAnnotationBeanPostProcessorTests must be improved further (and i'm ready to do it), but anyway made a mr to discuss if the whole idea of the fix is correct

As Sam wrote here #30724 (comment), the problem was indeed in merging init/destroy method names during AOT processing and then there're treated equally in runtime, however it's not true in non-aot mode. Because @PostConstruct annotations are considered externallyManaged and are processed differently. So i think the correct solution is to register CommonAnnotationBeanPostProcessor (or other InitDestroyAnnotationBeanPostProcessor-s) when they're used.

@snicoll
Copy link
Member

snicoll commented Dec 15, 2023

Thanks for the PR. Unfortunately, I don't think we should register the post-processor. It's a bit backwards as the AOT processing is meant to replace the post-processor by some optimized code.

Removing the method names from the generated code is also not something we'd like to pursue. Ideally the full list of methods to invoke would be in generated code, and the distinction with externally managed would not exist. I haven't looked at the problem enough to know if that's possible without changing something in the core container.

With that said, do you want to continue working on this or should we close this?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 15, 2023
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Dec 15, 2023
@snicoll
Copy link
Member

snicoll commented Dec 30, 2023

Alright I am going to close this now as this approach is not what we'd consider to fix the related issue. Thanks for the PR, in any case!

@snicoll snicoll closed this Dec 30, 2023
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 30, 2023
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) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invoke lifecycle methods in AOT mode in the same order as standard JVM mode
4 participants