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

refactor: BeanPostProcessor should avoid dependencies on other beans #1304

Closed

Conversation

DanielLiu1123
Copy link

This PR primarily addresses two issues:

  • LoadBalancerRestClientBuilderBeanPostProcessor and LoadBalancerWebClientBuilderBeanPostProcessor are BeanPostProcessor and should not depend on other beans. This could lead to the premature initialization of Configuration classes, potentially resulting in warnings like the following:
2023-12-02T14:20:26.574+08:00  WARN 85116 --- [load-balancer] [    Test worker] trationDelegate$BeanPostProcessorChecker : Bean 'org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerBeanPostProcessorAutoConfiguration' of type [org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerBeanPostProcessorAutoConfiguration] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). The currently created BeanPostProcessor [loadBalancerWebClientBuilderBeanPostProcessor] is declared through a non-static factory method on that class; consider declaring it as static instead.
2023-12-02T14:20:26.576+08:00  WARN 85116 --- [load-balancer] [    Test worker] trationDelegate$BeanPostProcessorChecker : Bean 'org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerBeanPostProcessorAutoConfiguration$ReactorDeferringLoadBalancerFilterConfig' of type [org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerBeanPostProcessorAutoConfiguration$ReactorDeferringLoadBalancerFilterConfig] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). Is this bean getting eagerly injected into a currently created BeanPostProcessor [loadBalancerWebClientBuilderBeanPostProcessor]? Check the corresponding BeanPostProcessor declaration and its dependencies.
2023-12-02T14:20:26.579+08:00  WARN 85116 --- [load-balancer] [    Test worker] trationDelegate$BeanPostProcessorChecker : Bean 'reactorDeferringLoadBalancerExchangeFilterFunction' of type [org.springframework.cloud.client.loadbalancer.reactive.DeferringLoadBalancerExchangeFilterFunction] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). Is this bean getting eagerly injected into a currently created BeanPostProcessor [loadBalancerWebClientBuilderBeanPostProcessor]? Check the corresponding BeanPostProcessor declaration and its dependencies.
  • The default value for spring.cloud.loadbalancer.retry.enabled is true, but this has not been consistently applied in some code.

@DanielLiu1123 DanielLiu1123 changed the title BeanPostProcessor should avoid dependencies on other beans. refactor: BeanPostProcessor should avoid dependencies on other beans Dec 2, 2023
@spencergibb
Copy link
Member

Not sure we can make these breaking changes in minor releases.

@DanielLiu1123
Copy link
Author

Including these changes in version 4.1.0 (before the GA) seems reasonable.

@spencergibb
Copy link
Member

It's a minor release. We won't make breaking changes

@DanielLiu1123
Copy link
Author

These changes will be accepted?

@spencergibb
Copy link
Member

please be patient

@app2smile
Copy link

After upgrade to Spring Cloud 2023.0.0 , when I start up my application

[ WARN ]  PostProcessorRegistrationDelegate$BeanPostProcessorChecker.postProcessAfterInitialization(429):  Bean 'org.springframework.cloud.client.loadbalancer.LoadBalancerAutoConfiguration$LoadBalancerInterceptorConfig' of type [org.springframework.cloud.client.loadbalancer.LoadBalancerAutoConfiguration$LoadBalancerInterceptorConfig] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). The currently created BeanPostProcessor [lbRestClientPostProcessor] is declared through a non-static factory method on that class; consider declaring it as static instead.

[ WARN ]  PostProcessorRegistrationDelegate$BeanPostProcessorChecker.postProcessAfterInitialization(437):  Bean 'org.springframework.cloud.loadbalancer.config.BlockingLoadBalancerClientAutoConfiguration' of type [org.springframework.cloud.loadbalancer.config.BlockingLoadBalancerClientAutoConfiguration] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). Is this bean getting eagerly injected into a currently created BeanPostProcessor [lbRestClientPostProcessor]? Check the corresponding BeanPostProcessor declaration and its dependencies.

@crmky
Copy link

crmky commented Dec 13, 2023

Unfortunately the issue was not addressed in Spring Cloud 2023 release. Similar to @app2smile, our applications generates many warnings during startup after upgrade to Spring Boot 3.2 with Spring Cloud 2023.

Those warnings are caused by changes introduced by #1294. The new introduced LoadBalancerRestClientBuilderBeanPostProcessor depends on either RetryLoadBalancerInterceptor or LoadBalancerInterceptor, which causes the dependencies in the whole dependency chain get early initialized.

It will be good if we can address those warnings in the next release.

@OlgaMaciaszek
Copy link
Collaborator

OlgaMaciaszek commented Jan 11, 2024

Thanks for creating the PR @DanielLiu1123. As indicated in this comment, while the warnings provide information, we have not noticed this is causing any bugs. In any case, we can consider the change proposal for a future major (date or release train number not set yet), however, please create a separate PR with changes for retry behaviour and a comment with your justification for it, and remove those changes from this one, as we might be considering it independently.

@DanielLiu1123
Copy link
Author

I'd be happy to do that.

@OlgaMaciaszek
Copy link
Collaborator

Hello @DanielLiu1123 please keep in mind, that I've already created a PR related to changes in the post-processor after determining there was a bug: #1319, so please just create a separate PR with your retry logic changes, and we will review it.

@DanielLiu1123
Copy link
Author

Already done, #1321

@OlgaMaciaszek
Copy link
Collaborator

Thanks @DanielLiu1123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants