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

AbstractCachingViewResolver - caching redirect views leads to memory leak [SPR-10065] #14698

Closed
spring-issuemaster opened this issue Dec 3, 2012 · 7 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 3, 2012

Michał Jaśtak opened SPR-10065 and commented

When user uses URL prefixed with "redirect:" as the method invocation result in Controller, it is cached as the whole (with provided parameters) in AbstractCachingViewResolver. Because the parameters for redirect may vary for the same URL used in redirect, and HashMap based cache is used, that leads to memory leak.

PS: this problem exists also in 2.5.x, I didn't checked how far in the Spring history it reaches


Affects: 3.1.3

Reference URL: http://vard-lokkur.blogspot.com/2012/12/springs-web-mvc-redirect-to-memory-leak.html

Issue Links:

  • #7831 Performance improvement on AbstractCachingViewResolver

Referenced from: commits f19bc57, 9deaefe

Backported to: 3.1.4

1 votes, 7 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2012

Rossen Stoyanchev commented

In the example from the referenced blog post, the same redirect URL can be expressed as a URI template, which would ensure the same view name is used as the cache key every time.

In other words instead of:

return "redirect:form.html?entityId=" + entityId;

Do this:

return "redirect:form.html?entityId={entityId}";

In the above example, entityId can be a model attribute or if it is present as a URI variable on the current request, then it'll work fine.

In Spring 3.1+ it is actually preferable to use RedirectAttributes:

@RequestMapping(method = RequestMethod.POST)
public String onPost(RedirectAttributes attrs) {
    ...
    attrs.addAttribute(entityId, 123);
    return "redirect:form.html;   // resulting URL has entityId=123
}

The above would also work before Spring 3.1 as long as entityId is in the model. However, using RedirectAttributes is preferable. See the reference documentation for more detail on that.

In summary I think AbstractCachingViewResolver does what it's expected to do. It has a flag to disable caching but that should not be necessary. The main concern is that the memory leak can occur if a redirect URL is constructed in the controller and ends being different every time. However, I don't see an easy way to detect that.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 11, 2012

Juergen Hoeller commented

It doesn't hurt to use a LinkedHashMap with a configurable cache limit (default 1024) here, similar to how we do it in CachingMetadataReaderFactory and NamedParameterJdbcTemplate. I have added this for 3.2 GA and 3.1.4 now.

However, Rossen's advice still applies: Ideally, don't build custom redirect URLs with such changing ids through String concatenation. Just like you don't concatenate values into SQL Strings either but rather use PreparedStatements.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 12, 2012

Michał Jaśtak commented

@Rossen - I agree that redirect methods presented by you are a way better than one used by me, in fact should be used instead - thanks for recalling them here :) - It doesn't change the fact, that method used by me is allowed too, and shouldn't lead to memory leak anyway.

Thank you all for correcting this very fast!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 23, 2012

Adedayo E commented

@Rossen Please can you verify this is true "The above would also work before Spring 3.1 as long as entityId is in the model". A quick test i did on at least version 3.0.7 does not seem to validate the statement. Thanks!!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 24, 2012

Rossen Stoyanchev commented

Adedayo E, it has always worked that way for primitive model attributes (int, long, etc). From the Javadoc of RedirectView:

By default all primitive model attributes (or collections thereof) are exposed as
HTTP query parameters (assuming they've not been used as URI template variables),
but this behavior can be changed by overriding the
isEligibleProperty(String, Object) method.

Also see RedirectTests.singleParam() for example.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 24, 2012

Adedayo E commented

@Rossen My comment relates to being able to achieve redirect:form.html?entityId={entityId} with previous Spring MVC versions (I use 3.0.7). My assumption was that your initial statement suggestions this was possible with previous versions. When I looked into the source of RedirectView (3.1.3) I see expandUriTemplateVariables = true. This is not present in version 3.0.7. Only commented because I spent some time trying to get it to work with 3.0.7, but it wasn’t working. My URIs look smtng like "redirect:/org/{orgId}/cars" vs "redirect:/org/"+orgId+"/cars". Thanks for the great work!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 26, 2012

Rossen Stoyanchev commented

URI variables are not supported in redirect URLs before 3.1. Apologies if my comment was not clear but "the above would also work before Spring 3.1" was in reference to the preceding example, not all examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.