Avoid full synchronization in AbstractRefreshableApplicationContext.getBeanFactory() since it can lead to massive thread blocking

[axel-grossmann-sap]

spring-framework/spring-context/src/main/java/org/springframework/context/support/AbstractRefreshableApplicationContext.java

Line 175 in b122483

synchronized (this.beanFactoryMonitor) {

On large servers running Tomcat with lots of cores we see massive thread blocking in getBeanFactory() due to the synchronized block. This is hit as we're using XmlWebApplicationContext and any time a bean is looked up during a web request the bean factory is required.

Outside of web applications we're also having another context using GenericXmlApplicationContext which doesn't have that problem.

Would be great to get some insight to either find a workaround on our side or fix the implementation.

Thank you
Axel

[jhoeller]

Which code path typically hits the blocking there? Some form of getBean lookups on the WebApplicationContext reference, internally delegating to the BeanFactory every time? Our internal lookups often hold on to the inner BeanFactory and operate directly on it, this could also be a way out for custom retrieval code.

That said, we might be able to rework this (rather old) beanFactoryMonitor lock there with some finer-grained locking for read access, potentially even just as a volatile field for access and a dedicated lock for (re-)initialization and shutdown.

[axel-grossmann-sap]

I'm just trying to collect some cases.

Here's an first interesting one which is a pure Spring one (TenantIgnoreXmlWebApplicationContext is our class but doesn't override containsBean() or anything in the way ). Seems that each request going through org.springframework.web.multipart.support.MultipartFilter is always doing a bean lookup and that's always hitting getBeanFactory().

...
TenantIgnoreXmlWebApplicationContext(AbstractRefreshableApplicationContext).getBeanFactory() line: 175	
	TenantIgnoreXmlWebApplicationContext(AbstractApplicationContext).containsBean(String) line: 1146	
	MultipartFilter.lookupMultipartResolver() line: 157	
	MultipartFilter.lookupMultipartResolver(HttpServletRequest) line: 143	
	MultipartFilter.doFilterInternal(HttpServletRequest, HttpServletResponse, FilterChain) line: 108	
	MultipartFilter(OncePerRequestFilter).doFilter(ServletRequest, ServletResponse, FilterChain) line: 119	
	ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 193	
	ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 166	
	FilterChainProxy$VirtualFilterChain.doFilter(ServletRequest, ServletResponse) line: 320	
	FilterSecurityInterceptor.invoke(FilterInvocation) line: 127	
	FilterSecurityInterceptor.doFilter(ServletRequest, ServletResponse, FilterChain) line: 91	

[jhoeller]

Indeed, some of our filter implementations and also some web integration delegates (e.g. for JSF) call getBean methods at the WebApplicationContext level and therefore go through the lock.

While we could revise those spots to hold on to the nested BeanFactory instead, it seems more attractive to switch AbstractRefreshableApplicationContext to a volatile beanFactory field (which I tried locally already, seems to work fine so far). Since AbstractApplicationContext applies a startupShutdownMonitor already, we should be able to get rid of the beanFactoryMonitor completely, always accessing the volatile field instead.

[axel-grossmann-sap]

I'm not sure where there is a nested BeanFactory, because in our stack traces there are always just those two two methods 'above' our own call to getBean(String):

• org.springframework.context.support.AbstractApplicationContext.getBean(String, Class) calling
• org.springframework.context.support.AbstractRefreshableApplicationContext.getBeanFactory()

May be we're missing out on something before that call, when we look up the current web application context the way below. (It's a static utility method we need for our legacy code base, just in case you wonder.)

	private static ApplicationContext getWebApplicationContextIfExists()
	{
		ServletContext servletContext = null;

		final RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
		if (requestAttributes instanceof ServletRequestAttributes)
		{
			servletContext = ((ServletRequestAttributes) requestAttributes).getRequest().getServletContext();
		}

...
		if (servletContext != null)
		{
			final ApplicationContext context = WebApplicationContextUtils.getWebApplicationContext(servletContext);
			if (context == null)
			{
				if (log.isDebugEnabled())
				{
					log.debug("No web spring configuration for webapp " + servletContext.getServletContextName()
							+ " found, using only core configuration.");
				}
			}
			else
			{
…
				return context;

			}
		}
		return null;
	}

[axel-grossmann-sap]

Aside of that volatile sounds like a good way. 👍