Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-2258

Use of InheritableThreadLocal creates leak for newly created background threads

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.8, 2.0 RC2
    • Fix Version/s: 2.0.1, 1.2.9
    • Component/s: Core
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      [Quoting part of a reply to a post by Peter Veentjer on the Concurrency-Interest mailing list:]

      There seems to be almost a guarantee for Policy/Strategy support in most of the Spring framework: pure interface, partial base implementations providing useful defaults, and a default instance in the places that need it all this - in contrast with overriding methods. This is good, for many reasons.

      Now, a good idea would be to extend this flexibility to thread context creation in the framework in general: Global/ThreadLocal/InheritableThreadLocal, and the new ActuallyWorksWithGCThreadLocal mentioned by Doug some days ago. This is not because I want the new deterministic threadlocal garbage collection option, but because InheritableThreadLocal is hardcoded in 2 places in Spring: the locale holder, and the new scoping support.

      I find this to cause uncollectable reference leaks, in most applications of fair complexity, on all relevant platforms (ibm, bea, sun).

      The reason (as stated in the thread mentioned above) is simple: there are two groups of components: those that create InheritableThreadLocals, and those that create threads. The mix of these two usually results in references dangling off live threads long after the relevant ClassLoader would have been released. A minimal example would be:
      --------------------------------------
      public class ImageController implements Controller {

      public ModelAndView handleRequest(HttpServletRequest request, HttpServletResponse response) throws Exception

      { BufferedImage image = new BufferedImage(20, 20, BufferedImage.TYPE_INT_RGB); Graphics2D g = image.createGraphics(); response.setContentType("image/png"); OutputStream out = response.getOutputStream(); ImageIO.write(image, "png", out); out.close(); out.flush(); g.dispose(); return null; }

      }
      --------------------------------------
      There isn't much to see in the code, but this controller will activate the Java 2D subsystem, which happens to include starting the sun.java2d.Disposer thread, which will only exit at JVM shutdown (this can be verified with a JVMTI agent). See what I mean by saying that InheritableThreadLocal creators can't fathom all thread creation sites above their stack? I'll bet good money I can find 3 other leaks possibilities of this type caused by Spring in the Sun JVM alone (although I also found these two spring ITLs in a different place).

      Now, next for a related story - in the Acegi Security subframework (is there such a thing?):
      http://opensource.atlassian.com/projects/spring/browse/SEC-152

      In the beginning, Acegi used a regular ThreadLocal for auth context. Then developers using JFC or RMI requested transparent propagation to child threads, resulting in a patch to use InheritableThreadLocal. Some IBM 1.3 users reported bugs in their JVM implementation, causing the migration back to a regular ThreadLocal. This could, of course, not please everybody, so a more configurable solution was devised: let the framework user choose an implementation of a Strategy interface for 'thread bound context' creation:
      http://www.acegisecurity.org/multiproject/acegi-security/apidocs/org/acegisecurity/context/SecurityContextHolderStrategy.html

      Now this is a good solution, but not perfect: the modus of configuration is not very flexible, and it's Acegi-specific.
      I propose an extension to spring itself, exposing a strategy/polity extension point for child frameworks like Acegi, infrastructure like the Spring MVC locale support, the new Scoped AOP, and (most important of all) RMI/Swing/etc developers seeking a problem-specific solution.

      I think the place for such support would be somewhere on the ApplicationContext itself, making (for instance) configuration in web.xml possible.

        Activity

        Hide
        plethora Taras Tielkes added a comment -

        Another scenario is a thread pool invoked from the current deployment context (let's assume a war) that has a lifetime longer than the deployment unit. If the pool manages expansion by creating additional threads from the calling thread, hooking/leaking the classloader reference happens quite easily.

        I've categorized this entry as 'Improvement', but it's not far from 'Bug'.
        To paraphrase a controversial author of Java articles: "InheritableThreadLocals are evil"

        There's just no way to guarantee that some code below the current stack frame won't create a thread that outlives (and thus leaks/retains) the current context ClassLoader.

        Show
        plethora Taras Tielkes added a comment - Another scenario is a thread pool invoked from the current deployment context (let's assume a war) that has a lifetime longer than the deployment unit. If the pool manages expansion by creating additional threads from the calling thread, hooking/leaking the classloader reference happens quite easily. I've categorized this entry as 'Improvement', but it's not far from 'Bug'. To paraphrase a controversial author of Java articles: "InheritableThreadLocals are evil" There's just no way to guarantee that some code below the current stack frame won't create a thread that outlives (and thus leaks/retains) the current context ClassLoader.
        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        I'm actually inclined to simply change both of those Spring-internal ThreadLocals to a normal ThreadLocal instead of an InheritableThreadLocal for Spring 2.0.1. The reason is partly the potential leak that you pointed out, in particular with respect to a thread pool happening to create a new thread during a DispatcherServlet request, but also the request being closed after the main request processing ended. In the latter case, the InheritableThreadLocal both for the RequestAttributes and an HttpServletRequest-based LocaleContext can easily lead to the - then invalid - request being accessed afters the completion of its original processing. As this is all undesirable and there is no propagation of those to remote clients in the first place, I feel it's better to simply turn them back to normal ThreadLocals, with not even a need for configuration options there.

        As a side note: All of this just affects DispatcherServlet/DispatcherPortlet requests, as well as direct LocaleContext setting in application code (which is rare). There are no other InheritableThreadLocals within Spring, in particular not in the handling of transactional resources.

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - I'm actually inclined to simply change both of those Spring-internal ThreadLocals to a normal ThreadLocal instead of an InheritableThreadLocal for Spring 2.0.1. The reason is partly the potential leak that you pointed out, in particular with respect to a thread pool happening to create a new thread during a DispatcherServlet request, but also the request being closed after the main request processing ended. In the latter case, the InheritableThreadLocal both for the RequestAttributes and an HttpServletRequest-based LocaleContext can easily lead to the - then invalid - request being accessed afters the completion of its original processing. As this is all undesirable and there is no propagation of those to remote clients in the first place, I feel it's better to simply turn them back to normal ThreadLocals, with not even a need for configuration options there. As a side note: All of this just affects DispatcherServlet/DispatcherPortlet requests, as well as direct LocaleContext setting in application code (which is rare). There are no other InheritableThreadLocals within Spring, in particular not in the handling of transactional resources. Juergen
        Hide
        plethora Taras Tielkes added a comment -

        I would certainly welcome the (minimal) switch away from InheritableThreadLocal to regular ThreadLocal. Better a small improvement than none at all.

        Some of my other comments will perhaps come up again in the future, when Java 7 and the "deterministic cleanup" ThreadLocal variant are actually released.

        PS Perhaps you could fix the small typo in the issue title (provice -> provide). It seems regular users can't edit the title of their own issues?

        Show
        plethora Taras Tielkes added a comment - I would certainly welcome the (minimal) switch away from InheritableThreadLocal to regular ThreadLocal. Better a small improvement than none at all. Some of my other comments will perhaps come up again in the future, when Java 7 and the "deterministic cleanup" ThreadLocal variant are actually released. PS Perhaps you could fix the small typo in the issue title (provice -> provide). It seems regular users can't edit the title of their own issues?
        Hide
        plethora Taras Tielkes added a comment -

        Inclusion of the change for "localeContextHolder" in 1.2.9 would be very welcome.

        Show
        plethora Taras Tielkes added a comment - Inclusion of the change for "localeContextHolder" in 1.2.9 would be very welcome.
        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        I have turned this issue into a bug, pointing out the potential leak caused by the use of InheritableThreadLocal.

        The solution that I've implemented now is explicit support for both kinds of ThreadLocal in LocaleContextHolder and RequestContextHolder, plus a "threadContextInheritable" flag on DispatcherServlet and DispatcherPortlet – being off by default. This needs to be turned on explicitly to expose the LocaleContext and the RequestAttributes for child threads (that is, to enable the use of an InheritableThreadLocal instead of a standard ThreadLocal for this particular DispatcherServlet/Portlet's context exposure). I consider inheritance being turned off as the safer default. We generally do not recommend to rely on that state in child threads in the first place.

        The reason why I decided to not go for an extension hook at present is that this is all static infrastructure: static ThreadLocals hidden in static context accessors. An extension point could only be static as well, which does not fit well into Spring's general approach. Furthermore, switching the ThreadLocal implementation would affect all usages of LocaleContextHolder and RequestContextHolder in the entire ClassLoader, whereas with the present solution as outlined above, this can be chosen on a per-usage basis (e.g. per DispatcherServlet instance).

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - I have turned this issue into a bug, pointing out the potential leak caused by the use of InheritableThreadLocal. The solution that I've implemented now is explicit support for both kinds of ThreadLocal in LocaleContextHolder and RequestContextHolder, plus a "threadContextInheritable" flag on DispatcherServlet and DispatcherPortlet – being off by default. This needs to be turned on explicitly to expose the LocaleContext and the RequestAttributes for child threads (that is, to enable the use of an InheritableThreadLocal instead of a standard ThreadLocal for this particular DispatcherServlet/Portlet's context exposure). I consider inheritance being turned off as the safer default. We generally do not recommend to rely on that state in child threads in the first place. The reason why I decided to not go for an extension hook at present is that this is all static infrastructure: static ThreadLocals hidden in static context accessors. An extension point could only be static as well, which does not fit well into Spring's general approach. Furthermore, switching the ThreadLocal implementation would affect all usages of LocaleContextHolder and RequestContextHolder in the entire ClassLoader, whereas with the present solution as outlined above, this can be chosen on a per-usage basis (e.g. per DispatcherServlet instance). Juergen
        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        This is in CVS HEAD now; should be available in the next nightly snapshot. It would be great if you could give this a try!

        FYI, 2.0.1 is scheduled for release next week, with 1.2.9 following soon after.

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - This is in CVS HEAD now; should be available in the next nightly snapshot. It would be great if you could give this a try! FYI, 2.0.1 is scheduled for release next week, with 1.2.9 following soon after. Juergen
        Hide
        plethora Taras Tielkes added a comment -

        The nightly snapshots (for 2.0.1) no longer show the leak through inherited threadlocals, thumbs up!

        Affected / interested users might also want to take a look at a similar issue I filed for webflow:
        http://opensource.atlassian.com/projects/spring/browse/SWF-206

        Show
        plethora Taras Tielkes added a comment - The nightly snapshots (for 2.0.1) no longer show the leak through inherited threadlocals, thumbs up! Affected / interested users might also want to take a look at a similar issue I filed for webflow: http://opensource.atlassian.com/projects/spring/browse/SWF-206
        Hide
        juergen.hoeller Juergen Hoeller added a comment -

        FYI, the Spring 1.2.9 release is - finally - just around the corner, scheduled for December 30th. I've made a sort of release candidate available in the form of the 1.2.9-20061222 snapshot, available from:

        http://static.springframework.org/downloads/nightly

        Please give this a try, simply replacing the 1.2.8 jar with the 1.2.9 snapshot jar and testing whether everything still works for you. This will help us to ensure the quality of the actual 1.2.9 release next week!

        Juergen

        Show
        juergen.hoeller Juergen Hoeller added a comment - FYI, the Spring 1.2.9 release is - finally - just around the corner, scheduled for December 30th. I've made a sort of release candidate available in the form of the 1.2.9-20061222 snapshot, available from: http://static.springframework.org/downloads/nightly Please give this a try, simply replacing the 1.2.8 jar with the 1.2.9 snapshot jar and testing whether everything still works for you. This will help us to ensure the quality of the actual 1.2.9 release next week! Juergen

          People

          • Assignee:
            juergen.hoeller Juergen Hoeller
            Reporter:
            plethora Taras Tielkes
            Last updater:
            Trevor Marshall
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              10 years, 19 weeks, 4 days ago