Guice leaking memory on webapp redeploy?

[Gili]

Hi guys,

I would appreciate your help commenting on this issue:
https://glassfish.dev.java.net/issues/show_bug.cgi?id=5104

The Glassfish guys have identified two memory leaks, one of which
results from Guice's use of thread-local storage. They're asking
questions about the Guice codebase. You know a lot more about this
code than I do so I would appreciate your feedback :)

I will post a similar messages in the warp-persist mailing list.

Thanks,
Gili

[Dhanji R. Prasanna]

Threadlocals in request-scope are diligently cleared at the end of each request in a finally block, so I doubt the problem is there.

The problem is running out of permgen space, which is due to the generated proxies. If any instance or class hangs around after undeploying from a parent classloader (the famous commons-logging problem), the entire set of classes in the child classloader will stick around. So you run out of permgen on the 5th or 6th deploy.

Dhanji.

[Stuart McCulloch]

2008/10/28 Gili <gili.t...@gmail.com>

Hi guys,

I would appreciate your help commenting on this issue:
https://glassfish.dev.java.net/issues/show_bug.cgi?id=5104

The Glassfish guys have identified two memory leaks, one of which
results from Guice's use of thread-local storage. They're asking
questions about the Guice codebase. You know a lot more about this
code than I do so I would appreciate your feedback :)

firstly, which version of guice is being used?

secondly, I would fully expect the current trunk to leak classes on a restart
because the code to allow unloading of proxies is currently disabled due
to issue http://code.google.com/p/google-guice/issues/detail?id=235

there is a working patch for 235, but it has not yet been committed

to confirm this you could try running with the "guice.custom.loader" system
property set to "true", which will re-enable the unloading of proxy classes.

as for threadlocals, there is only one in the core code and it should get
cleaned up when the context stack completes - but again, this could be
related to the proxy classes (depending which version they're using)

I will post a similar messages in the warp-persist mailing list.

Thanks,
Gili

--
Cheers, Stuart

[Dhanji R. Prasanna]

On Tue, Oct 28, 2008 at 2:35 PM, Stuart McCulloch <mcc...@gmail.com> wrote:

2008/10/28 Gili <gili.t...@gmail.com>

Hi guys,

I would appreciate your help commenting on this issue:
https://glassfish.dev.java.net/issues/show_bug.cgi?id=5104

The Glassfish guys have identified two memory leaks, one of which
results from Guice's use of thread-local storage. They're asking
questions about the Guice codebase. You know a lot more about this
code than I do so I would appreciate your feedback :)

firstly, which version of guice is being used?

secondly, I would fully expect the current trunk to leak classes on a restart
because the code to allow unloading of proxies is currently disabled due
to issue http://code.google.com/p/google-guice/issues/detail?id=235

 

It shouldn't matter, the webapp classloader itself should be gc'ed away. This is some other leak, maybe thru Hibernate or commons-logging.

Dhanji.

[Stuart McCulloch]

2008/10/28 Dhanji R. Prasanna <dha...@gmail.com>

maybe, but enabling that option would reduce the "noise" from the proxy classes and help them find the true cause, especially if they're doing leak analysis of the Java heap...

Dhanji.

--
Cheers, Stuart

[Gili]

On Oct 27, 11:38 pm, "Dhanji R. Prasanna" <dha...@gmail.com> wrote:

> On Tue, Oct 28, 2008 at 2:35 PM, Stuart McCulloch <mccu...@gmail.com> wrote:
> > firstly, which version of guice is being used?

Now that you mention it I don't recall. I believe it was v1. I need to
retest now anyway with warp-persist v2 and guice v2 so we'll know for
sure shortly.

> > secondly, I would fully expect the current trunk to leak classes on a
> > restart
> > because the code to allow unloading of proxies is currently disabled due

> > to issuehttp://code.google.com/p/google-guice/issues/detail?id=235

>
> It shouldn't matter, the webapp classloader itself should be gc'ed away.
> This is some other leak, maybe thru Hibernate or commons-logging.

The Glassfish guy was saying that they reuse threads even after the
webapp dies. So Guice's TLS could potentially keep a strong reference
to classes in the ClassLoader, preventing it from garbage-collecting.
Right?

Gili

[Stuart McCulloch]

2008/10/28 Gili <gili.t...@gmail.com>

Guice v1 definitely has a threadlocal leak in the injector, where it wasn't being removed properly - this was fixed in one of my OSGi patches that Jesse committed to trunk, as it was stopping bundle code from being fully unloaded.

so I'd be interested to hear how things are with v2 ( with and without the "guice.custom.loader" system property :)
 

Gili

--
Cheers, Stuart

[limpb...@gmail.com]

I believe the FinalizableReferenceQueue leak might also pose problems.
This will be fixed soon...

http://code.google.com/p/google-guice/issues/detail?id=227

[Gili Tzabari]

I've tested warp-persist v2 with Guice v2 with guice.custom.loader=true
and the leak is still there. I've spent a couple of hours trying to
track it down but it seems I don't know what I'm doing. It's not obvious
to me how to use the Netbeans profiler or JVisualVM to track this down.

I tried deploying and undeploying my webapp twice and comparing the two
memory snapshots. That got me nowhere. Now I'm thinking I'll deploy and
undeploy my webapp, take a snapshot and investigate why Guice is still
in memory (since it shouldn't be).

Gili

[Gili]

Got it. I just confirmed that the GC root leaking all the memory (at
least for now) is the one mentioned in http://code.google.com/p/google-guice/issues/detail?id=227

Jesse, please put out a new snapshot or SVN tag a new release once you
fix this. I will retest and let you know if the leak is gone.

Gili

[Bob Lee]

Like Jesse said, I think the FRQ thread is the real problem. I don't recall a ThreadLocal memory leak in Guice core and would be surprised if one ever existed.

Bob

[Gili Tzabari]

That's basically what I'm seeing on my end too. Let me know once
this is fixed and I will retest for other leaks.

Gili

Bob Lee wrote:
> Like Jesse said, I think the FRQ thread is the real problem. I don't
> recall a ThreadLocal memory leak in Guice core and would be surprised
> if one ever existed.
>
> Bob
>
> On Mon, Oct 27, 2008 at 11:31 PM, je...@swank.ca

> <mailto:je...@swank.ca> <limpb...@gmail.com

[Bob Lee]

OK, I came up with an implementation of FinalizableReferenceQueue (pasted below) that won't create a strong reference from a thread to your application class loader, but I really don't think we should commit this.

If you want to support web application re-deploying, it will be much simpler and safer to just put Guice in your system classpath.

Bob

/*
 * Copyright (C) 2007 Google Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.google.common.base;

import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.lang.ref.PhantomReference;
import java.lang.reflect.Method;
import java.lang.reflect.Field;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.io.InputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.net.URL;
import java.net.URLClassLoader;

/**
 * Starts a background thread that cleans up after reclaimed referents.
 *
 * <p>Most of this code goes to preventing creation of a strong reference
 * from the thread to this class's loader so we can support reloading in
 * application servers.
 *
 * @author Bob Lee
 */
class FinalizableReferenceQueue extends ReferenceQueue<Object> {

  private static final Logger logger
      = Logger.getLogger(FinalizableReferenceQueue.class.getName());

  private static final ReferenceQueue<Object> queue = startFinalizer();

  /**
   * Returns the singleton instance.
   */
  public static ReferenceQueue<Object> getInstance() {
    return queue;
  }

  private static final String FINALIZER_CLASS_NAME
      = "com.google.common.base.Finalizer";

  /**
   * Starts the finalizer thread.
   */
  private static ReferenceQueue<Object> startFinalizer() {
    try {
      ClassLoader systemLoader = ClassLoader.getSystemClassLoader();
      if (systemLoader != null) {
        /*
         * If the class is already in the system class path, don't bother
         * creating a new class loader.
         */
        try {
          if (systemLoader.loadClass(FINALIZER_CLASS_NAME) != null) {
            //return startFinalizerDirectly();
          }
        }
        catch (ClassNotFoundException e) { /* ignore */ }
      }

      // Read Finalizer class bytes from class loader.
      ClassLoader thisLoader = FinalizableReferenceQueue.class.getClassLoader();

      String fileName = "/" + FINALIZER_CLASS_NAME.replace('.', '/') + ".class";
      InputStream in = thisLoader.getResourceAsStream(fileName);
      if (in == null) {
        throw new IOException("Could not find " + fileName + " in class path.");
      }

      // Copy bytes into a byte[].
      ByteArrayOutputStream out = new ByteArrayOutputStream();
      byte[] buffer = new byte[1024];
      int read;
      while ((read = in.read(buffer)) > -1) {
        out.write(buffer, 0, read);
      }
      in.close();
      byte[] finalizerBytes = out.toByteArray();

      /*
       * We use URLClassLoader because it's the only concrete class loader
       * implementation in the JDK. If we used our own ClassLoader subclass,
       * Finalizer would have a reference to our custom class loader instance,
       * which would reference its class, which would reference the application
       * class loader, which we're trying to prevent.
       */
      ClassLoader loader = new URLClassLoader(new URL[0], systemLoader);
      Method defineClass = ClassLoader.class.getDeclaredMethod("defineClass",
          String.class, byte[].class, int.class, int.class);
      defineClass.setAccessible(true);
      Class<?> finalizer = (Class<?>) defineClass.invoke(loader,
          FINALIZER_CLASS_NAME, finalizerBytes, 0, finalizerBytes.length);

      // Give Finalizer a reference to this class so it knows when to stop.
      Method backReference = finalizer.getDeclaredMethod(
          "backReference", Class.class);
      backReference.setAccessible(true);
      backReference.invoke(null, FinalizableReferenceQueue.class);

      return extractQueue(finalizer);
    } catch (Exception e) {
      logger.log(Level.WARNING, "Could not load " + FINALIZER_CLASS_NAME
          + " in its own class loader.", e);

      // Fall back to loading Finalizer directly.
      return startFinalizerDirectly();
    }
  }

  /**
   * Loads the finalizer in this class loader and starts it.
   */
  private static ReferenceQueue<Object> startFinalizerDirectly() {
    try {
      return extractQueue(Class.forName(FINALIZER_CLASS_NAME));
    }
    catch (ClassNotFoundException e) {
      throw new AssertionError(e);
    }
  }

  /**
   * Copies a queue reference from Finalizer into this class.
   */
  @SuppressWarnings("unchecked")
  private static ReferenceQueue<Object> extractQueue(Class<?> finalizer) {
    try {
      Field queueField = finalizer.getDeclaredField("queue");
      queueField.setAccessible(true);
      return (ReferenceQueue<Object>) queueField.get(null);
    }
    catch (NoSuchFieldException e) {
      throw new RuntimeException(e);
    }
    catch (IllegalAccessException e) {
      throw new RuntimeException(e);
    }
  }
}

/**
 * Thread that finalizes referents. All references should implement Runnable.
 *
 * THE APPLICATION CLASS LOADER SHOULD *NOT* LOAD THIS CLASS DIRECTLY. THIS
 * CLASS AND ITS CLASS LOADER SHOULD NOT STRONGLY REFERENCE THE APPLICATION
 * CLASS LOADER.
 *
 * Doing so would create a strong reference from a thread to an application
 * class which would keep the garbage collector from reclaiming the
 * application class loader, thereby breaking reloading of applications
 * in application servers by leaking memory.
 */
class Finalizer extends Thread {

  private static final Logger logger
      = Logger.getLogger(Finalizer.class.getName());

  /**
   * Reference queue. The application will grab a reference to this
   * reflectively.
   */
  static final ReferenceQueue<Object> queue = new ReferenceQueue<Object>();

  private static PhantomReference<Class<?>> backReference;

  /**
   * Creates a reference back to the application class loader so we can find
   * out if it has been garbage collected. Once it has, we can safely stop
   * the finalizer thread.
   */
  static void backReference(Class<?> clazz) {
    backReference = new PhantomReference<Class<?>>(clazz, queue);
  }

  static {
    // Start background thread.
    new Finalizer().start();
  }

  /** Constructs a new finalizer thread. */
  private Finalizer() {
    super(Finalizer.class.getSimpleName());
    setDaemon(true);
  }

  /**
   * Loops continuously, pulling references off the queue and cleaning them up.
   */ 
  @Override
  public void run() {
    while (true) {
      try {
        Reference<?> reference = queue.remove();

        if (reference == backReference) {
          // The application class loader has been garbage collected. We can
          // stop and be collected ourselves.
          // Theoretically, this is the last reference in the queue.
          return;
        }

        cleanUp(reference);
      } catch (InterruptedException e) { /* ignore */ }
    }
  }

  /**
   * Cleans up a single reference. Catches and logs all throwables.
   */
  private void cleanUp(Reference<?> reference) {
    try {
      // TODO: We really don't want to make the FinalizableXxxReference classes
      // implement Runnable (it would expose the finalization in the public
      // API!). We need to pass FinalizableReference.class into this class
      // (perhaps in backReference()) and then reflectively invoke
      // finalizeReferent().
      ((Runnable) reference).run();
    } catch (Throwable t) {
      logger.log(Level.SEVERE, "Error cleaning up after reference.", t);
    }
  }
}

[Bob Lee]

Also, to my knowledge, Guice does not have a ThreadLocal leak, despite accusations to the contrary in this thread and the Glassfish thread.

Bob

[Gili Tzabari]

Bob Lee wrote:
> OK, I came up with an implementation of FinalizableReferenceQueue
> (pasted below) that won't create a strong reference from a thread to
> your application class loader, but I really don't think we should commit
> this.

Are you saying the code needs more cleanup before it's ready for commit
or are you saying something else?

> If you want to support web application re-deploying, it will be much
> simpler and safer to just put Guice in your system classpath.

Wouldn't that make things worse? We want to ensure that all
webapp-specific proxies get unloaded when the webapp gets unloaded.

Gili

[Bob Lee]

On Tue, Oct 28, 2008 at 5:16 PM, Gili Tzabari <gili.t...@gmail.com> wrote:

       Are you saying the code needs more cleanup before it's ready for commit
or are you saying something else?

I'm saying that I don't think the solution is viable. This needs to be fixed in the VM instead. The workaround is just too nasty for my taste, and I'd just assume ask users to put Guice in their system class path instead.
 

       Wouldn't that make things worse? We want to ensure that all
webapp-specific proxies get unloaded when the webapp gets unloaded.

cglib loads the proxies in the same class loader as the proxied class, so the proxies would indeed get unloaded along with the web app.

Bob

[Gili Tzabari]

What about the fact that we can hook servlet shutdown? Would it be
possible for FinalizableReferenceQueue's thread to shut down cleanly if
asked?

BTW: What's the point of FinalizableReferenceQueue anyway? It's not
like you're doing some sort of disposal once the Reference gets queued.
Why use a ReferenceQueue at all?

Gili

[Bob Lee]

On Tue, Oct 28, 2008 at 6:37 PM, Gili Tzabari <gili.t...@gmail.com> wrote:

       What about the fact that we can hook servlet shutdown? Would it be
possible for FinalizableReferenceQueue's thread to shut down cleanly if
asked?

That's a possibility, but right now, FinalizableReferenceQueue isn't exposed in the published API at all. I'd rather not do so, especially considering you can just put Guice in your system classpath during development and work around the problem.
 

       BTW: What's the point of FinalizableReferenceQueue anyway? It's not
like you're doing some sort of disposal once the Reference gets queued.
Why use a ReferenceQueue at all?

It does do disposal (finalizeReferent() in the old code, Runnable.run() in the new code).

Bob

[Gili Tzabari]

Bob Lee wrote:
> That's a possibility, but right now, FinalizableReferenceQueue isn't
> exposed in the published API at all. I'd rather not do so, especially
> considering you can just put Guice in your system classpath during
> development and work around the problem.

This problem occurs more frequently during development time, but is
actually more critical during production time. Ironically the original
bug report against Glassfish was by people complaining that it was
unacceptable to have to restart the web server every time you wanted to
redeploy your webapp. Apparently it happens often enough to warrant
attention. Also, please consider the implications of running Guice at
the system ClassLoader level: all webapps would have to share the same
Guice instance/version. It would make it difficult to upgrade one webapp
independently of the others. This goes pretty strongly against webapp
"best practices".

That being said, I think we can have our cake and eat it too. Please
take a look at http://forums.sun.com/thread.jspa?messageID=10484065 and
http://java.sun.com/javase/6/docs/api/java/lang/ref/package-summary.html#reachability

The more I read about this, the more I become convinced that ejp is
right. In theory all we'd need to do is have the servlet listener ask
Guice to stop the thread and the remaining References would get
garbage-collected automatically. Yes, this would require you to expose
an extra API function but I think it is quite reasonable when
considering the benefit. I wouldn't expose FinalizableReferenceQueue
either (that's too low-level). Going by the Guice 1.0 API I would
suggest adding Guice.close() that mirrors the way
EntityManagerFactory.close() works. Alternatively you could use
Injector.close() -- I believe others have asked for this anyway to
satisfy other use-cases. Non-servlet users wouldn't have to change anything.

In short: you'd expose a single API call consisting of 2-3 lines of
code (shutting down the thread). In exchange, you'd retain the original
clean implementation of FinalizableReferenceQueue. I know you're not a
fan of exposing a new method but *please* consider the cost/benefit for
us poor web users :)

Thanks,
Gili

[Bob Lee]

On Tue, Oct 28, 2008 at 9:58 PM, Gili Tzabari <gili.t...@gmail.com> wrote: 

       That being said, I think we can have our cake and eat it too. Please
take a look at http://forums.sun.com/thread.jspa?messageID=10484065 and
http://java.sun.com/javase/6/docs/api/java/lang/ref/package-summary.html#reachability

I don't understand what you're getting at.

The problem is that we have a strong reference chain from Thread -> MyRunnable -> MyRunnable's Class -> Application ClassLoader.

My code above breaks the dependency between MyRunnable (Finalizer) and the application classes and then loads MyRunnable in its own class loader so that the Application ClassLoader is eligible for collection.
 

       In short: you'd expose a single API call consisting of 2-3 lines of
code (shutting down the thread). In exchange, you'd retain the original
clean implementation of FinalizableReferenceQueue. I know you're not a
fan of exposing a new method but *please* consider the cost/benefit for
us poor web users :)

FRQ is actually in Google Collections, not Guice.

I think I'd use my code above before I'd add a shutDown() method to the API...

Bob

[limpb...@gmail.com]

On Oct 28, 9:58 pm, Gili Tzabari <gili.tzab...@gmail.com> wrote:
> I know you're not a fan of exposing a new method but *please* consider the cost/benefit for us poor web users :)

We're sympathetic. I don't like the idea of Guice having consequences
on deployment. Or redeployment.

Perhaps we could clean up the FRQ code by moving classloading to its
own helper class. This would make that code testable, and perhaps
worthy of inclusion...

[Gili Tzabari]

Bob Lee wrote:
> I don't understand what you're getting at.
>
> The problem is that we have a strong reference chain from Thread ->
> MyRunnable -> MyRunnable's Class -> Application ClassLoader.

The GC root in this case is the Thread, right? I am saying that if you
shut down the Thread then the application ClassLoader would no longer be
strongly reachable and all associated Soft/WeakReferences and
ReferenceQueue would get garbage-collected properly.

> FRQ is actually in Google Collections, not Guice.
>
> I think I'd use my code above before I'd add a shutDown() method to the
> API...

This issue affects more than just Guice. Any code depending on Google
Collections would potentially suffer from this memory leak while running
under a web container. Look, I don't understand the reluctance. Did I
fail to provide a good use-case? Did I say something wrong?

je...@swank.ca wrote:
> We're sympathetic. I don't like the idea of Guice having consequences
> on deployment. Or redeployment.
>
> Perhaps we could clean up the FRQ code by moving classloading to its
> own helper class. This would make that code testable, and perhaps
> worthy of inclusion...

I would really appreciate any effort you make to this end. Please let
me know if there is anything more I can do to help.

Thank you,
Gili

[Stuart McCulloch]

2008/10/29 Bob Lee <craz...@crazybob.org>

Also, to my knowledge, Guice does not have a ThreadLocal leak, despite accusations to the contrary in this thread and the Glassfish thread.

I'd like to clarify this if I may... there was a small ThreadLocal leak in the Guice1 injector:

   http://fisheye2.atlassian.com/browse/google-guice/trunk/src/com/google/inject/InjectorImpl.java?r=253#l738

see where it nulls out the reference... but this still leaves the ThreadLocal in place which
has a strong reference to Guice internals, this meant that Guice1 couldn't be completely
unloaded from OSGi containers

to show this, I've hacked up a basic testcase consisting of two classes that loads Guice
and a testclass in their own custom classloader, so they can be unloaded when they are
no longer being used (similar to OSGi and many web-app containers)

results:

     javac -classpath guice-1.0.jar *.java

     # you can see Guice classes being loaded, but never unloaded
     java -classpath "guice-1.0.jar;." -verbose:class Main

     # you can see Guice classes being loaded *and* unloaded
     java -classpath "guice-snapshot.jar;." -verbose:class Main

I believe the GlassFish team were originally testing with Guice1, hence their statement
about the ThreadLocal (which would show up during heap analysis) however this issue
is definitely fixed in the current Guice codebase / snapshots

HTH

//==================================================

import com.google.inject.Guice;
import com.google.inject.Injector;

public class Test {
  static class Foo {}

  public static void doInject() {
    Injector injector = Guice.createInjector();
    injector.getInstance(Foo.class);
  }
}

//==================================================

import java.io.File;
import java.lang.reflect.Method;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;

public class Main {

  public static void main(String[] args) throws Exception {

    ClassLoader customLoader = new UnloadingClassLoader();
    Class clazz = customLoader.loadClass("Test");
    Method test = clazz.getDeclaredMethod("doInject", new Class[0]);

    test.invoke(null, new Object[0]);

    test = null;
    clazz = null;
    customLoader = null;

    while (true) {
      try {
        System.runFinalization();
        System.gc();
        Thread.sleep(3000);
      } catch (InterruptedException ie) {}
    }
  }

  static class UnloadingClassLoader extends URLClassLoader {

    public UnloadingClassLoader() {
      super(new URL[0]);

      String systemPath = System.getProperty("java.class.path");
      String[] classpath = systemPath.split(File.pathSeparator);
      for (int i = 0; i < classpath.length; i++) {
        try {
          addURL(new URL(classpath[i]));
        } catch (MalformedURLException e1) {
          try {
            addURL(new File(classpath[i]).toURI().toURL());
          } catch (MalformedURLException e2) {
            throw new RuntimeException(e1);
          }
        }
      }
    }

    protected Class loadClass(String name, boolean resolve)
        throws ClassNotFoundException {

      synchronized (this) {
        Class clazz = findLoadedClass(name);
        if (clazz != null) {
          return clazz;
        }
      }

      if (name.startsWith("java.")) {
        return super.loadClass(name, resolve);
      } else {
        Class clazz = findClass(name);
        if (resolve) {
          resolveClass(clazz);
        }
        return clazz;
      }
    }
  }
}

//==================================================

--
Cheers, Stuart

[Stuart McCulloch]

2008/10/29 Gili Tzabari <gili.t...@gmail.com>

Bob Lee wrote:
> If you want to support web application re-deploying, it will be much
> simpler and safer to just put Guice in your system classpath.

       Wouldn't that make things worse? We want to ensure that all
webapp-specific proxies get unloaded when the webapp gets unloaded.

FYI with the "guice.custom.loader" property enabled, proxies are loaded into
a custom classloader separate to Guice's classloader - this means they can
be safely unloaded even if Guice is on your system classpath

Gili

--
Cheers, Stuart

[Stuart McCulloch]

2008/10/29 Stuart McCulloch <mcc...@gmail.com>

and as Bob points out, even without this CGLIB will load proxies in the same
classloader as the proxied type - so they should still get unloaded along with
the webapp container (the custom classloader solution just means the proxy
classes can be unloaded sooner)

--
Cheers, Stuart

[Bob Lee]

Ohhh... this isn't actually a leak. The ThreadLocal itself is no longer strongly referenced (the only reference was from the Injector instance).

The problem is that the ThreadLocal class doesn't clean up the underlying map entry until you call set() or remove() on another ThreadLocal instance in the same thread. Once you call set() or remove() a couple times, the ClassLoader should get garbage collected with no problem.

The right thing to do here would be for me to use Object[] instead of InternalContext[] (http://crazybob.org/2006/07/hard-core-java-threadlocal.html).

I've actually been working a new implementation of ThreadLocal that cleans up immediately from a background thread (w/o locks): http://code.google.com/p/google-threadlocal/source/browse/trunk/main/java/lang/ThreadLocal.java

This is a great use case for justifying my new implementation. Thanks for pointing it out.

Bob

[Bob Lee]

On Tue, Oct 28, 2008 at 10:49 PM, Gili Tzabari <gili.t...@gmail.com> wrote:

       This issue affects more than just Guice. Any code depending on Google
Collections would potentially suffer from this memory leak while running
under a web container. Look, I don't understand the reluctance. Did I
fail to provide a good use-case? Did I say something wrong?

My point is just that your proposal would require adding a shutdown() method to both the Google Collections and the Guice APIs (since Google Collections is an internal implementation detail). It would also require users to explicitly invoke this shutdown hook (which very well may become a no-op in some future version).

Rather than add a shutdown hook, we'll just use my solution above which requires no API changes or servlet context hooks.

Bob

[Stuart McCulloch]

2008/10/29 Bob Lee <craz...@crazybob.org>

Hi Bob,

your current solution relies on reading the classfile from the current classloader
by using getResourceAsStream(), but I don't think this is guaranteed to work in
all situations - a safer solution would be to use ASM to generate the bytecode
at runtime (downside: even more code and less readable)

but then again, the ASM generated class could be very simple, perhaps just
a loop that calls a Callable<Boolean> object passed in via the constructor?

Bob

--
Cheers, Stuart

[Bob Lee]

On Wed, Oct 29, 2008 at 8:22 AM, Stuart McCulloch <mcc...@gmail.com> wrote:

your current solution relies on reading the classfile from the current classloader
by using getResourceAsStream(), but I don't think this is guaranteed to work in
all situations - a safer solution would be to use ASM to generate the bytecode
at runtime (downside: even more code and less readable)

I think it'll work 99% of the time, and it can fail back to loading the class directly in the remaining cases. If it becomes an issue, we can just put the class in a constant byte[].
 

but then again, the ASM generated class could be very simple, perhaps just
a loop that calls a Callable<Boolean> object passed in via the constructor?

Presumably, the Callable<Boolean> would be implemented by a Guice class, so you'd have a leak.

Bob

[Gili Tzabari]

My main objection to the new implementation is the complexity
factor, it's much more prone to subtle bugs. Let's give it a try anyway.
If it works then everyone is happy, if not we can try something else.

PS: Should I try rebuilding this code on my end or were you planning to
clean up it further and cut a snapshot release?

Thank you,
Gili

[Bob Lee]

On Wed, Oct 29, 2008 at 9:55 AM, Gili Tzabari <gili.t...@gmail.com> wrote:

   My main objection to the new implementation is the complexity
factor, it's much more prone to subtle bugs. Let's give it a try anyway.
If it works then everyone is happy, if not we can try something else.

PS: Should I try rebuilding this code on my end or were you planning to
clean up it further and cut a snapshot release?

I'll finish it up. I need to make it not use Runnable, write some tests, etc.

Thank you for the feedback and the offer to help. I appreciate it.

Bob