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

Thread contention in HandlerMethod due to unnecessary BeanFactory.getType call [SPR-12832] #17429

Closed
spring-issuemaster opened this issue Mar 19, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Mar 19, 2015

Nick Verbeck opened SPR-12832 and commented

I am seeing a very large thread blocking contention on AbstractRefreshableApplicationContext.getBeanFactory() method for a very high transnational MVC app. I have done a bit of digging into the spring code base and know what the issue is. I have also got a few ideas on how to fix it.
First off here is the thread dump showing the block:

java.lang.Thread.State: BLOCKED
at org.springframework.context.support.AbstractRefreshableApplicationContext.getBeanFactory(AbstractRefreshableApplicationContext.java:168)
waiting to lock <78d90cb> (a java.lang.Object) owned by "EmbeddedJettyThread-75" t@75
at org.springframework.context.support.AbstractApplicationContext.getType(AbstractApplicationContext.java:1009)
at org.springframework.web.method.HandlerMethod.getBeanType(HandlerMethod.java:161)
at org.springframework.web.method.HandlerMethod$HandlerMethodParameter.getContainingClass(HandlerMethod.java:258)
at org.springframework.core.ResolvableType.forMethodParameter(ResolvableType.java:1094)
at org.springframework.core.ResolvableType.forMethodParameter(ResolvableType.java:1063)
at org.springframework.core.convert.TypeDescriptor.<init>(TypeDescriptor.java:77)
at org.springframework.beans.TypeConverterDelegate.convertIfNecessary(TypeConverterDelegate.java:107)
at org.springframework.beans.TypeConverterSupport.doConvert(TypeConverterSupport.java:64)
at org.springframework.beans.TypeConverterSupport.convertIfNecessary(TypeConverterSupport.java:47)
at org.springframework.validation.DataBinder.convertIfNecessary(DataBinder.java:603)
at org.springframework.web.method.annotation.AbstractNamedValueMethodArgumentResolver.resolveArgument(AbstractNamedValueMethodArgumentResolver.java:104)
at org.springframework.web.method.support.HandlerMethodArgumentResolverComposite.resolveArgument(HandlerMethodArgumentResolverComposite.java:77)
at org.springframework.web.method.support.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:162)
at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:129)
at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:110)
at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandleMethod(RequestMappingHandlerAdapter.java:777)
at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:706)
at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:85)
at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:943)
at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:877)
at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:966)
at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:857)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:687)
at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:842)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)

In my digging around and debugging. I traced the contention back to the RequestMappingHandlerMapping. When it goes to handle a request it will find the respective HandlerMethod for the controller method being requested. Once it has the instance it then calls HandlerMethod.createWithResolvedBean(). This is where one of the first blockings happens, as the HandlerMethod instance is created with just the beanName value vs the actual bean reference. So it now must call out to the BeanFactory and get the bean, resulting in a sync lock. Now as part of the createWithResolvedBean() flow the method params (HandlerMethod.HandlerMethodParameter) from the previous HandlerMethod are passed to the new one. This part becomes important as you will see.
Continuing on with the handling of the request we later go about performing method parameter resolving. At this point we iterate over all the HandlerMethod.parameters (InvocableHandlerMethod.getMethodArgumentValues()) to see if we need to resolve the type to a different one. As part of that check we call HandlerMethod.HandlerMethodParameter.getContainingClass() which is a call back up to the parent class's HandlerMethod.getBeanType(). Now as I mentioned earlier this parent was created with the beanName not the bean reference. Even thought the params where copied to a new HandlerMethod instance with a resolved bean. The parent reference in HandlerMethodParameter is to the previous HandlerMethod it was created under. This results in a block for every parameter your controller method is looking for. So in a highly transactional app, especially with lots of params being expected, it ends up blocking most of the threads all the time.
Hopefully that all makes sense.
Now I have 2 proposals for a change to remedy this issue.

  1. Change the AbstractHandlerMethodMapping.initHandlerMethods() to pass the bean reference when it calls detectHandlerMethods() instead of the beanName. This would cause the cached instances of HandlerMethod to hold a reference as well as their HandlerMethodParameter instances. This would removed the locks during params type resolution as well as during the initial HandlerMapping.getHandler() (HandlerMethod.createWithResolvedBean). This should also greatly reduce CPU load.
  2. The second optional I have is to modify HandlerMethod.<init>(HandlerMethod handlerMethod) and HandlerMethod.<init>(HandlerMethod handlerMethod, Object handler). To do a shallow copy of the params. Which I believe should move the parent reference over to the new HandlerMapping instance. This would remove the contention on the param type resolution, but would leave the one on the HandlerMapping.getHandler().

Affects: 4.1.5

Issue Links:

  • #14382 Share MethodParameter cache between HandlerMethods

Referenced from: commits 5f95ff6, 898c24f

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 19, 2015

Rossen Stoyanchev commented

The MethodParameter[] is re-used to avoid another performance cost of reflection (see #14382). In HandlerMethod however we could cache the resolved bean and the resolved bean type so that repeated calls to createWithResolvedBean and getBeanType won't have to do look-ups every time. Does that makes sense?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2015

Nick Verbeck commented

That makes sense. The only worry I'd have is with thread collision. Since multiple threads could be calling getBeanType and createWithResolvedBean at the same time. Resulting in two or more threads setting this.bean.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2015

Juergen Hoeller commented

It looks like there's no need to cache the bean instance that way to begin with. Late resolution of bean names is a feature aimed for non-singleton beans, and we should preserve it as much as possible. It looks like immediate caching of the bean type in HandlerMethod's constructor is feasible which would reduce the getType calls to one per method.

In addition, AbstractHandlerMethodMapping may construct HandlerMethod instances with the internal BeanFactory instead of the ApplicationContext facade which skips the outer lifecycle lock there. This would avoid your contention spot to begin with, making getBean calls for singleton beans very efficient (i.e. effectively a straight ConcurrentHashMap lookup).

I'll have a commit ready in master later today, and will backport this to 4.1.6 by Monday.

Juergen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.