RequestCondition optimizations

[rstoyanchev]

Introduce a common cache, e.g. RequestMappingInfoCache, that's stored as a request attribute (Spring MVC) or exchange attribute (WebFlux) to avoid repeated re-calculation of request related values required for condition checks.

There are already optimizations in WebFlux so request path, "Content-Type", and "Accept" are parsed only once, but even then "Accept" media types are sorted repeatedly + there are other recurring checks like isPreFlightRequest. This would allow Spring MVC be more on par with WebFlux in terms of optimal request condition checks.

A lot of the base functionality could be in AbstractRequestCondition with sub-classes accessing computeIfAbsent type methods from the base class.

[rstoyanchev]

Team Decision: This was brought up to the team. One comment was that MimeTypeUtils caches parsed media types at a lower level which helps Spring MVC to a degree. No other specific comments or objections made.

[rstoyanchev]

I've added a new HandlerMapping.LOOKUP_PATH request attribute that caches the result of UrlPathHelper#getRequestLookupPath and is looked up to avoid repeated resolution.

ProducesRequestCondition uses a similar attribute to stored the parsed and sorted list of acceptable media types.

Further optimizations have also been made to reduce object creation.

[hdeadman]

@rstoyanchev I am getting an NPE on line MediaType.java:550 of spring-web-5.2.0.M2.jar (currently line 563 in master) because the cache is apparently returning a null type from MimeTypeUtils.parseMimeType().

	public static MimeType parseMimeType(String mimeType) {
		return cachedMimeTypes.get(mimeType);
	}

and this method doesn't account for null return

	public static MediaType parseMediaType(String mediaType) {
		MimeType type;
		try {
			type = MimeTypeUtils.parseMimeType(mediaType);
		}
		catch (InvalidMimeTypeException ex) {
			throw new InvalidMediaTypeException(ex);
		}
		try {
			return new MediaType(type.getType(), type.getSubtype(), type.getParameters()); //NPE
		}
		catch (IllegalArgumentException ex) {
			throw new InvalidMediaTypeException(mediaType, ex.getMessage());
		}
	}

I don't see the current code handling this any better than 5.2.0.M2.jar. The MediaType.parseMediaType() method where the NPE is happening should also include the mediaType in the InvalidMimeTypeException that it throws (and it should probably throw that with the mediaType if parseMimeType(mediaType) returns null.

I am seeing this NPE today in production (in CAS 6.1.RC4-SNAPSHOT) on requests for a *.woff file (for which I had to turn on trace logging in DispatcherServlet). Yesterday I was seeing it on requests from Google Earth and I don't know what the mime type was or what the request was because I didn't have trace on, but the problem went away after I restarted the CAS (spring boot) application. The fact that restarting fixed the issue for the same Google Earth requests makes me suspect something is wonky with the cache. It looks like it is a LRU cache with max size 64 but what if you process more than 64 mime types?

EDIT: I just looked closer at the code and see now that "get" is actually doing more than just pulling from the cache, it is also populating it and not sure how it is returning null, but I am seeing NPE which implied "type" variable is null.

[hdeadman]

I think the ConcurrentLruCache in MimeTypeUtils has a bug b/c that is the only way null type could be getting returned. The ConcurrentLruCache.get(key) method does returns in two places and one of them is returning a null. The return in the write lock block looks safe but the return in the read lock block could return null if the internal queue had the same key in it twice but the internal cache map wouldn't necessarily have the value anymore. I think duplicate keys could get in the queue because the key is added to the queue in both the read lock and the write lock under heavy concurrency.

Pinging @bclozel since it looks like he wrote ConcurrentLruCache.

[bclozel]

@hdeadman Could you create an issue for that? It seems you've found the underlying problem, which is not related to this issue.

[hdeadman]

Done: #23211

[bclozel]

Awesome, thanks!