[SPR-10803] MemoryLeak in AntPathMatcher during caching AntPathStringMatcher instances Created: 05/Aug/13  Updated: 15/Jan/19  Resolved: 29/Oct/13

Status: Closed
Project: Spring Framework
Component/s: None
Affects Version/s: None
Fix Version/s: 3.2.5, 4.0 RC1

Type: Bug Priority: Minor
Reporter: Pawel Bobruk Assignee: Juergen Hoeller
Resolution: Complete Votes: 0
Labels: pull-request-submitted
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relate
relates to SPR-9749 Avoid per-request Pattern.compile() c... Closed
Days since last comment: 1 year, 30 weeks, 3 days ago
Last commented by a User: true
Last updater: Spring Issuemaster

 Description   

Implementation matchStrings:

	private boolean matchStrings(String pattern, String str, Map<String, String> uriTemplateVariables) {
		AntPathStringMatcher matcher = this.stringMatcherCache.get(pattern);
		if (matcher == null) {
			matcher = new AntPathStringMatcher(pattern);
			this.stringMatcherCache.put(pattern, matcher);
		}
		return matcher.matchStrings(str, uriTemplateVariables);
	}

is adding unlimited number of AntPathStringMatcher to "cache".

In applications with SEO friendly addresses with lots of combinations of parameters causes a problem with the heap memory, every url pattern are stored in cache.



 Comments   
Comment by Maciej Zasada [ 05/Aug/13 ]

To be precise - in our scenario, we have specific SEO requirements for our search page - all of the attributes must be included in as a part of the URL, not as a parameter, e.g.

example.com/man/brand+nike,adidas

We have one annotated catch-all-requests controller. We also have a mvc interceptor, which does URL parsing and transforms path tokens into set of parameters, e.g.

example.com/man/brand+nike,adidas => [category: [man], brand: [nike,adidas]]

Doing that, we have dynamic URLs based on product attributes and we can handle all of the defined attributes in one fashion. The downside is heavy memory use of AntPathMatcher cache, because we can produce as many unique URLs as cartesian product of all attributes. In our production env, this Map grown into >50% of the total heap size.

What would be nice to have is a plugable definition of cache strategy (e.g. fixed-sized LRU) for org.springframework.util.AntPathMatcher

Comment by Pawel Bobruk [ 26/Aug/13 ]

Created pull#343.

Comment by Andrzej Wisłowski [ 29/Oct/13 ]

As a temporary workaround we just added one fixed java class to our application in the /WEB-INF/classes of our web application. Due to tomcat class loading priority our application loads fixed AntPathMatcher java class before class from jar in the /WEB-INF/lib directory

Tomcat class-loader documentation : http://tomcat.apache.org/tomcat-7.0-doc/class-loader-howto.html

Comment by Juergen Hoeller [ 29/Oct/13 ]

I've addressed this with a general overhaul of AntPathMatcher's caching. In particular, I've introduced:

  • a "setCachePatterns(boolean)" method for explicit configuration (turning on an unlimited cache or turning it off completely)
  • a default turnoff threshold at 65536 entries (at which point we're deciding that caching isn't worthwhile because patterns are unlikely to be reoccurring often enough, so we're clearing the cache and deactivating it)
  • and an "AntPathStringMatcher getStringMatcher(String pattern)" template method which can be overridden in subclasses for a custom caching strategy.

That said, I am wondering where all those pattern entries come from in your scenario. After all, in a regular dispatcher arrangement, there is a finite number of mapped handler methods with an equally finite number of path patterns that they are mapped to. It's just the incoming requests that may have all sorts of path permutations. However, what we're caching is the mapped patterns, not the incoming paths...

I would argue that AntPathMatcher's caching is only worthwhile with a limited number of path patterns. Once you go beyond a certain limit (and the default 65536 is rather generous there anyway), it's not worth synchronizing access to the entire cache just in order to be able to drop the oldest entries from a LinkedHashMap. Instead, it's appropriate to turn off the cache completely.

Juergen

Comment by Andrzej Wisłowski [ 30/Oct/13 ]

In the class RequestMappingInfoHandlerMapping in the method handleMatch if no matching handler method for the current request was found then lookupPath is used as a pattern.

https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java#L101

We just stressed application with url (/unmapped_path/{i}) incrementing {i} to generate OutOfMemeryException.

Comment by Rossen Stoyanchev [ 30/Oct/13 ]

The lookupPath is used as a pattern if the matched request mapping does not have any patterns, i.e. an @RequestMapping with no patterns but possibly other mapping attributes (params, headers, etc).

I'm fixing this regardless but just confirming my understanding that it seems to be a very specific case that triggers this.

Comment by Juergen Hoeller [ 30/Oct/13 ]

Given that we fixed the hotspot that previously filled AntPathMatcher's cache beyond any limits, I'm inclined to only backport that fix to 3.2.5 and leave the general AntPathMatcher overhaul to the 4.0 line. After all, any such overhaul may also introduce subtle side effects; so if there's no strong need, I'd rather not backport it.

Juergen

Comment by Spring Issuemaster [ 14/Jan/19 ]

The Spring Framework has migrated to GitHub Issues. This issue corresponds to spring-projects/spring-framework#15429.

Generated at Fri Aug 14 15:25:04 UTC 2020 using Jira 8.5.4#805004-sha1:0444eab799707f9ad7b248d69f858774aadfd250.