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

MemoryLeak in AntPathMatcher during caching AntPathStringMatcher instances [SPR-10803] #15429

Closed
spring-issuemaster opened this issue Aug 5, 2013 · 7 comments
Assignees
Labels
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Aug 5, 2013

Pawel Bobruk opened SPR-10803 and commented

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.


Issue Links:

  • #14383 Avoid per-request Pattern.compile() calls.

Referenced from: commits 3261542, d4f4225, 9cbac98, 7a5a689, a7af950, 4bcfbc3

0 votes, 8 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 5, 2013

Maciej Zasada commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 26, 2013

Pawel Bobruk commented

Created pull#343.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 29, 2013

Andrzej Wisłowski commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 29, 2013

Juergen Hoeller commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 30, 2013

Andrzej Wisłowski commented

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.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 30, 2013

Rossen Stoyanchev commented

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.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 30, 2013

Juergen Hoeller commented

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

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