Spring Framework
  1. Spring Framework
  2. SPR-10065

AbstractCachingViewResolver - caching redirect views leads to memory leak

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Complete
    • Affects Version/s: 3.1.3
    • Fix Version/s: 3.1.4, 3.2 GA
    • Component/s: Web
    • Labels:
      None

      Description

      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

        Issue Links

          Activity

          Hide
          Rossen Stoyanchev added a comment - - edited

          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.

          Show
          Rossen Stoyanchev added a comment - - edited 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.
          Hide
          Juergen Hoeller added a comment - - edited

          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

          Show
          Juergen Hoeller added a comment - - edited 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
          Hide
          Michał Jaśtak added a comment -

          @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!

          Show
          Michał Jaśtak added a comment - @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!
          Hide
          Adedayo E added a comment -

          @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!!

          Show
          Adedayo E added a comment - @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!!
          Hide
          Rossen Stoyanchev added a comment -

          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.

          Show
          Rossen Stoyanchev added a comment - 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.
          Hide
          Adedayo E added a comment - - edited

          @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!

          Show
          Adedayo E added a comment - - edited @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!
          Hide
          Rossen Stoyanchev added a comment -

          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.

          Show
          Rossen Stoyanchev added a comment - 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.

            People

            • Assignee:
              Juergen Hoeller
              Reporter:
              Michał Jaśtak
              Last updater:
              Juergen Hoeller
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                1 year, 16 weeks, 4 days ago