glassfish
  1. glassfish
  2. GLASSFISH-14128

purge ThreadLocals from Threads that are reinserted into thread pools

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.1
    • Fix Version/s: None
    • Component/s: performance
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      See this blog entry for what can happen when a ThreadLocal is left around in a
      thread that is reinserted into thread pool:

      http://weblogs.java.net/blog/jjviana/archive/2010/06/09/dealing-glassfish-301-
      memory-leak-or-threadlocal-thread-pool-bad-ide

      A defensive measure against this error would be to insert code into GlassFish that
      would remove ThreadLocals from a thread when it it returned to a thread pool. The
      code to do this is in the blog entry.

        Activity

        Hide
        oleksiys added a comment -

        IMO we should separate 2 usecases (may be more):

        1) ThreadLocals, which are used to drive the application logic
        for these ThreadLocals I'd agree, that they should be cleaned each time thread is getting back to a pool

        2) ThreadLocals, used as object cache
        these threadlocals is pretty cheap way (if using accurately) to maintain object cache. Access to such cache is not synchronized -> it's fast and cheap.
        For objects with complex internal structure (usually some contexts) it's very useful to have such a cache of limited size (1-2 objects per thread). It could be even weakref'ed to avoid leaks.
        These threadlocals shouldn't be cleaned, cause it can lead to perf. regressions.

        That's why I'm coming back to my original idea, that IMO cleanup logic is not something threadpool implementation should have by default.

        Show
        oleksiys added a comment - IMO we should separate 2 usecases (may be more): 1) ThreadLocals, which are used to drive the application logic for these ThreadLocals I'd agree, that they should be cleaned each time thread is getting back to a pool 2) ThreadLocals, used as object cache these threadlocals is pretty cheap way (if using accurately) to maintain object cache. Access to such cache is not synchronized -> it's fast and cheap. For objects with complex internal structure (usually some contexts) it's very useful to have such a cache of limited size (1-2 objects per thread). It could be even weakref'ed to avoid leaks. These threadlocals shouldn't be cleaned, cause it can lead to perf. regressions. That's why I'm coming back to my original idea, that IMO cleanup logic is not something threadpool implementation should have by default.
        Hide
        Tim Quinn added a comment -

        Keep in mind that Java provides no supported, published way to access all of a thread's ThreadLocals generically.

        The blog entry referenced in an earlier comment uses introspection to get to a package-visible field of the Thread class.

        I agree that, ideally, the thread pool would clean the threads. With no supported way for the pool to do that, we'll need to decide whether to place that responsibility on the module or use an unsupported technique from the thread pool.

        Show
        Tim Quinn added a comment - Keep in mind that Java provides no supported, published way to access all of a thread's ThreadLocals generically. The blog entry referenced in an earlier comment uses introspection to get to a package-visible field of the Thread class. I agree that, ideally, the thread pool would clean the threads. With no supported way for the pool to do that, we'll need to decide whether to place that responsibility on the module or use an unsupported technique from the thread pool.
        Hide
        Bill Shannon added a comment -

        How does a module that creates a thread local know that the thread is being reused
        so that it can clean up the thread local? Seems like only the thread pool knows
        this.

        I agree that it's gross that we would have to look inside the Thread object for a
        private field, but it's not clear there's a better and equally reliable approach.
        If the thread pool implementation knows that certain thread locals don't need to
        be cleaned up, it could skip them. So the Grizzly thread pool could know about the
        Grizzly thread locals and skip them if they're safe. We could even come up with a
        way to annotate thread local classes that are safe to preserve between different uses
        of a thread.

        Show
        Bill Shannon added a comment - How does a module that creates a thread local know that the thread is being reused so that it can clean up the thread local? Seems like only the thread pool knows this. I agree that it's gross that we would have to look inside the Thread object for a private field, but it's not clear there's a better and equally reliable approach. If the thread pool implementation knows that certain thread locals don't need to be cleaned up, it could skip them. So the Grizzly thread pool could know about the Grizzly thread locals and skip them if they're safe. We could even come up with a way to annotate thread local classes that are safe to preserve between different uses of a thread.
        Hide
        oleksiys added a comment -

        I agree, in some cases module, which added threadlocal doesn't know about its destiny and can not do a cleanup at the proper time.

        If we rely on Grizzly thread-pool to do a cleanup, may be it would be worth to create a general RecyclableThread interface [1], which will be implemented by Grizzly threads.
        Create utility class + method [2]

        So the custom code, which is interested to add threadlocal and remove it, when thread is going to be recycled, will look like [3].
        This way we'd not force Grizzly to iterate over all threadlocals and perform some comparing, we won't use reflections.

        [1]
        interface RecyclableThread

        { addOnRecycleTask(Runnable task); removeOnRecycleTask(Runnable task); }

        [2]
        public class RecyclableThreadUtils() {
        public static void addOnRecycleTask(final Runnable task) {
        final Thread t = Thread.currentThread();
        if (t instanceof RecyclableThread)

        { ((RecyclableThread) t).addOnRecycleTask(task); }

        }
        }

        [3]
        private final ThreadLocal<String> t = new ThreadLocal<String>();

        public void doSomething() {
        t.set("Hello");

        RecyclableThreadUtils.addOnRecycleTask(new Runnable() {

        public void run()

        { t.remove(); }

        });
        }

        Show
        oleksiys added a comment - I agree, in some cases module, which added threadlocal doesn't know about its destiny and can not do a cleanup at the proper time. If we rely on Grizzly thread-pool to do a cleanup, may be it would be worth to create a general RecyclableThread interface [1] , which will be implemented by Grizzly threads. Create utility class + method [2] So the custom code, which is interested to add threadlocal and remove it, when thread is going to be recycled, will look like [3] . This way we'd not force Grizzly to iterate over all threadlocals and perform some comparing, we won't use reflections. [1] interface RecyclableThread { addOnRecycleTask(Runnable task); removeOnRecycleTask(Runnable task); } [2] public class RecyclableThreadUtils() { public static void addOnRecycleTask(final Runnable task) { final Thread t = Thread.currentThread(); if (t instanceof RecyclableThread) { ((RecyclableThread) t).addOnRecycleTask(task); } } } [3] private final ThreadLocal<String> t = new ThreadLocal<String>(); public void doSomething() { t.set("Hello"); RecyclableThreadUtils.addOnRecycleTask(new Runnable() { public void run() { t.remove(); } }); }
        Hide
        Harald Wellmann added a comment -

        I've just finished removing memory leaks in my web app deployed on Tomcat, which has some useful diagnostic and clean up features. Looking for similar features in Glassfish, I came across this issue.

        In short, if an application fails to remove ThreadLocals, Tomcat does the cleanup when the application is undeployed:
        http://wiki.apache.org/tomcat/MemoryLeakProtection

        So if Tomcat can do it, I see no reason why Glassfish can't

        It's hard enough to keep one's own code clean, but it's bad luck when you depend on thirdparty libs with improper ThreadLocal usage.
        See JAXB-831 for an example.

        Show
        Harald Wellmann added a comment - I've just finished removing memory leaks in my web app deployed on Tomcat, which has some useful diagnostic and clean up features. Looking for similar features in Glassfish, I came across this issue. In short, if an application fails to remove ThreadLocals, Tomcat does the cleanup when the application is undeployed: http://wiki.apache.org/tomcat/MemoryLeakProtection So if Tomcat can do it, I see no reason why Glassfish can't It's hard enough to keep one's own code clean, but it's bad luck when you depend on thirdparty libs with improper ThreadLocal usage. See JAXB-831 for an example.

          People

          • Assignee:
            oleksiys
            Reporter:
            Tom Mueller
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated: