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

Provide caching for HandlerMappingIntrospector lookups #31588

Closed
rstoyanchev opened this issue Nov 10, 2023 · 11 comments
Closed

Provide caching for HandlerMappingIntrospector lookups #31588

rstoyanchev opened this issue Nov 10, 2023 · 11 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@rstoyanchev
Copy link
Contributor

Spring Security relies on such lookups to align its request matching and CORS decisions with Spring MVC. However, it can perform the lookups multiple times per request, depending on the security configuration, and that can have a negative impact on performance, e.g. #31098.

We can expose a Filter to save and clear the result of a lookup in a request attribute before and after delegating to the rest of the filter chain. Spring Security can then integrate the Filter, allowing it to perform multiple calls without becoming a hotspot.

The solution needs to into account nested FORWARD and ERROR dispatches by checking for an existing value before replacing it, and also making sure it's restored at the end.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Nov 10, 2023
@rstoyanchev rstoyanchev added this to the 6.0.14 milestone Nov 10, 2023
@rstoyanchev rstoyanchev self-assigned this Nov 10, 2023
rstoyanchev added a commit that referenced this issue Nov 14, 2023
Expose methods to set and reset cache to use from a Filter instead
of a method to create such a Filter. Also use cached results only
if they match by dispatcher type and requestURI.

See gh-31588
@rPraml
Copy link

rPraml commented Nov 24, 2023

Hello @rstoyanchev

this change breaks some of our UI-tests, which scans for warnings and errors in the log.
The test finds unexpected warnings like this

Cache miss for REQUEST dispatch to '/' (previous null). Performing CorsConfiguration lookup. 
This is logged once only at WARN level, and every time at TRACE.

As workaround I can turn off the logger for now. My question is: How critical is this cache miss? Can I fix it by adjusting my CORS configuration?

Thanks
Roland

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Nov 24, 2023

@rPraml, Spring Security has not yet integrated this, see spring-projects/spring-security#14128, so at the moment this is expected. If you could check again with Spring Security snapshots once the issue is fixed, that would be great.

More generally, a cache miss does not mean that it won't work or will not make the right decision, but rather that the caching isn't used and there is an overhead in making repeated lookups to determine the target handler.

@pontusfalk
Copy link

pontusfalk commented Nov 26, 2023

With all due respect, logging a warning about something that's "expected" and nothing that can be resolved by the end user is problematic. The versions of Spring Framework and Spring security that was shipped with the latest Spring Boot 3.2 release will mean that this warning log will be present for all apps using request matchers(according to my quick tests to reproduce/silence it). I'm sure a lot of people will be googling for this warning in the coming weeks.
I suspect that the only workaround is to disable logging for HandlerMappingIntrospector altogether.

@cmdjulian
Copy link

I feel the same.

Just for reference setting:

logging:
  level:
    org.springframework.web.servlet.handler.HandlerMappingIntrospector: error

silences the warning (which does not feel good tbh)

@ctmay4
Copy link

ctmay4 commented Nov 27, 2023

Disabling the logging is easy enough, but this seems really strange. Why have a warning if there is no way to fix it?

@rwinch
Copy link
Member

rwinch commented Nov 28, 2023

I'm sorry for the problems this is causing everyone here.

Workaround for Current Requests

The following configuration can be used in a Spring Boot App to properly perform caching and avoid the log warning for the current request.

import jakarta.servlet.DispatcherType;
import jakarta.servlet.Filter;
import org.springframework.boot.web.servlet.FilterRegistrationBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.core.Ordered;
import org.springframework.web.servlet.handler.HandlerMappingIntrospector;

import java.util.EnumSet;

/**
 * @author Rob Winch
 */
@Configuration
class CacheHandlerMappingIntrospectorConfig {
	@Bean
	static FilterRegistrationBean<Filter> handlerMappingIntrospectorCacheFilter(HandlerMappingIntrospector hmi) {
		Filter cacheFilter = hmi.createCacheFilter();
		FilterRegistrationBean<Filter> registrationBean = new FilterRegistrationBean<>(cacheFilter);
		registrationBean.setOrder(Ordered.HIGHEST_PRECEDENCE);
		registrationBean.setDispatcherTypes(EnumSet.allOf(DispatcherType.class));
		return registrationBean;
	}
}

Cache Misses for Checking Another Request

The configuration above will ensure HandlerMappingIntrospector caching is done properly for the current request. However, if you are checking authorization on another request, then a cache miss will occur. An example of how this might occur is if you have a home page on / that only renders a link to /admin if the current user is authorized to access that URL.

This sort of logic is typically implemented using Spring Security's WebInvocationPrivilegeEvaluator API (e.g. Thymeleaf's authorize-url support or Spring Security's Authorize JSP Tag), so if you are using these APIs you will still see the warning.

Working on a Complete Solution

We will work with the Spring Framework team to provide a complete solution and get it out as soon as possible.

@jonnybecker
Copy link

jonnybecker commented Dec 21, 2023

I'm trying to figure out the (complete) status of this issue, since there are so many tickets regarding this issue (which are already closed/open).

Working with a Spring MVC application (Spring 6.1.2, Spring Security 6.2.1, Spring Boot 3.2.0) I'm still getting the HandlerMappingIntrospector.logCacheMiss warning.

I hope I don't sound pushy, but I can't figure out the status of how/in which combination this issue is already solved or what I have to do to on my own.

@bclozel
Copy link
Member

bclozel commented Dec 21, 2023

@jonnybecker Are you sure your application is using Spring Security 6.2.1 released earlier this week? I've tried a sample application with Spring Boot 3.2.1-SNAPSHOT (to be released today) and I'm not seeing the WARN logs.

@jonnybecker
Copy link

jonnybecker commented Dec 21, 2023

@bclozel sorry, my bad. I don't see any warning anymore with the versions mentioned above. After restarting my IDE and another deploy, the warning disappeared. So I guess there was some problem with the deployment in my development stage.

Sorry for the fuzz and thanks for your excellent work and support.

@tushar-meesho
Copy link

tushar-meesho commented Apr 22, 2024

@bclozel we have recently planning to migrate to spring-boot-3.0.1 version from spring-boot-2.7.10, Do we have to upgrade to Spring Boot 3.2.1-SNAPSHOT, so that we don't face any latency issues on production after migration. Is this fix live on release version spring-boot - 3.2.5

@bclozel
Copy link
Member

bclozel commented Apr 23, 2024

@tushar-meesho you should always upgrade to the latest maintenance version of the generation you are using. See https://spring.io/projects/spring-boot#learn

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

9 participants