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

Avoid re-creating HandlerMethod unless handler is resolved through the BeanFactory #34277

Closed
brucelwl opened this issue Jan 17, 2025 · 10 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@brucelwl
Copy link

  1. The following code caused my custom HandlerMethod subclass to be recreated as HandlerMethod,
  2. Every HTTP request calls createWithResolvedBean(), resulting in the creation of a new instance of HandlerMethod. The handler in HandlerMethod should be determined at startup(main thread), rather than being processed after each call

https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java#L302-L323

	/**
	 * Re-create the HandlerMethod and initialize
	 * {@link #shouldValidateArguments()} and {@link #shouldValidateReturnValue()}.
	 * @since 6.1.3
	 */
	public HandlerMethod createWithValidateFlags() {
		return new HandlerMethod(this, null, true);
	}


	/**
	 * If the provided instance contains a bean name rather than an object instance,
	 * the bean name is resolved before a {@link HandlerMethod} is created and returned.
	 */
	public HandlerMethod createWithResolvedBean() {
		Object handler = this.bean;
		if (this.bean instanceof String beanName) {
			Assert.state(this.beanFactory != null, "Cannot resolve bean name without BeanFactory");
			handler = this.beanFactory.getBean(beanName);
		}
		Assert.notNull(handler, "No handler instance");
		return new HandlerMethod(this, handler, false);
	}

my code

public class ControllerRequestMappingHandlerMapping extends RequestMappingHandlerMapping {

    private final List<ControllerInterceptor> controllerInterceptors;

    public ControllerRequestMappingHandlerMapping(List<ControllerInterceptor> controllerInterceptors) {
        this.controllerInterceptors = controllerInterceptors;
    }

    @Override
    protected HandlerMethod createHandlerMethod(Object handler, Method method) {
        ArrayList<ControllerInterceptor> interceptors = new ArrayList<>();
        Class<?> declaringClass = method.getDeclaringClass();
        for (ControllerInterceptor interceptor : controllerInterceptors) {
            Pointcut pointcut = interceptor.getPointcut();
            if (AopUtils.canApply(pointcut, declaringClass)) {
                interceptors.add(interceptor);
            }
        }
        if (interceptors.size() > 0) {
            if (handler instanceof String beanName) {
                return new InterceptorHandlerMethod(beanName,
                        obtainApplicationContext().getAutowireCapableBeanFactory(),
                        obtainApplicationContext(),
                        method, interceptors);
            }
            return new InterceptorHandlerMethod(handler, method, interceptors);
        }
        return super.createHandlerMethod(handler, method);
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 17, 2025
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 27, 2025

The main reason for createWithResolvedBean is to allow the handler, which could be a prototype bean, to be resolved per request through the ApplicationContext. If you're overriding AbstractHandlerMethodMapping#createHandlerMethod, however to fix the handler instance on startup, then there is not reason to re-create the HandlerMethod instance at runtime.

We can update HandlerMethod#createWithResolvedBean along those lines, and you will need to adjust your createHandlerMethod override to call the HandlerMethod constructor with a resolved handler instance.

@rstoyanchev rstoyanchev changed the title Custom HandlerMethod will be overwritten Avoid re-creating HandlerMethod unless handler is resolved through the BeanFactory Jan 27, 2025
@rstoyanchev rstoyanchev self-assigned this Jan 27, 2025
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 27, 2025
@rstoyanchev rstoyanchev added this to the 6.2.3 milestone Jan 27, 2025
@brucelwl
Copy link
Author

@rstoyanchev 👍

@brucelwl
Copy link
Author

brucelwl commented Feb 5, 2025

@rstoyanchev

Can change private to protected? I need to inherit it and implement my own HandlerMethod

private HandlerMethod(HandlerMethod handlerMethod, @Nullable Object handler, boolean initValidateFlags) 

https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java#L178-L198

@brucelwl
Copy link
Author

brucelwl commented Feb 5, 2025

@rstoyanchev
Is it possible to change the initialization method of RequestMappingHandlerMapping#afterPropertiesSet
and AbstractHandlerMethodMapping#afterPropertiesSet
from InitializingBean to SmartInitializingSingleton,
otherwise override the createHandlerMethod method to obtain beans may cause BeanCurrentlyInCreationException

Image

Image

Image

@rstoyanchev
Copy link
Contributor

I am re-opening as the changes caused test failures in Boot tests related to CORS configuration.

@rstoyanchev rstoyanchev reopened this Feb 5, 2025
@rstoyanchev
Copy link
Contributor

@brucelwl, I'll have a look at making the HandlerMethod constructor protected.

In terms of initialization and BeanCurrentlyInCreationException, I'm less inclined to do that, and not in a maintenance release. Something to consider is we have public registerMapping and unregisterMapping methods on AbstractHandlerMethodMapping. So you could get all the mappings via getHandlerMethods after those have been initialized and re-register them as a HandlerMethod subclass with bean instances.

@brucelwl
Copy link
Author

brucelwl commented Feb 5, 2025

@rstoyanchev Thanks, I resolved the BeanCurrentlyInCreationException as you suggested

@brucelwl
Copy link
Author

brucelwl commented Feb 6, 2025

@rstoyanchev I delayed the initialization of the HandlerMethod in the following way to avoid BeanCurrentlyInCreationException , is it feasible?

my custom RequestMappingHandlerMapping

Image

@rstoyanchev
Copy link
Contributor

As I mentioned before, this is not something we will do in a maintenance release. However, I've created #34375, which turns out is also related to a custom HandlerMethod, and will make some improvements for that scenario. I will comment again under that issue when I have more to share,

@brucelwl
Copy link
Author

brucelwl commented Feb 6, 2025

@rstoyanchev

Can change private to protected? I need to inherit it and implement my own HandlerMethod

private HandlerMethod(HandlerMethod handlerMethod, @nullable Object handler, boolean initValidateFlags)
https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java#L178-L198

@rstoyanchev Can we solve this problem first and change it to protected, so that I can also custom my own Handler Method,
I can override createWithValidateFlags() and createWithResolvedBean() to ensure that all returned HandlerMethodsare my CustomHandlerMethod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants