2009/2/8 Gili <[email protected]> > > Stuart, > > It was my understanding [1] that there was no need for a worker > thread. I thought that we had established that there were concurrent > reference map implementations out there that will do a great job > without it and one of them might even end up in Java7 (all the better > since we'd end up using a core Java API). > > [1] http://code.google.com/p/google-guice/issues/detail?id=288 >
I'll have to defer to Bob, Doug and the other JSR166 experts on this ;) [ http://code.google.com/p/google-guice/issues/detail?id=288#c11 ] a concurrent reference map is imho an important (and useful) feature, so I don't mind them taking time in making sure everything works ok I personally favor any of the solutions you mentioned except for > running Guice in the system or boot classpath. > > Gili > > On Feb 8, 7:10 am, Stuart McCulloch <[email protected]> wrote: > > 2009/2/7 Gili <[email protected]> > > > > > Hi, > > > > > I noticed you guys committed some new code to tackle the memory leaks > > > recently so I gave it a spin. I still see the memory leak under > > > Glassfish v2ur2 and the GC root that is keeping the instances alive > > > seems to be com.google.inject.internal.Finalizer. > > > > > Stuart, can you please look into whether Glassfish or Guice is at > > > fault here? > > > > Hi Gili, > > > > The current reference cache in Guice relies on a background worker thread > > to dequeue and finalize references. Because an active Java thread is a GC > > root this thread must be stopped when it's no longer required, otherwise > any > > classes that it refers to (and their classloaders) cannot be collected. > > > > There are various ways to do this - some libraries expose a public API > that > > containers can use to tell the thread when to stop. Others use > "declarative" > > metadata (such as the Bundle-Activator manifest entry in OSGi) to refer > to > > names of helper classes that can take part in certain managed lifecycles. > > > > The approach used in Guice (and google-collections) is for the thread > itself > > to try and detect when it's no longer required. It does this by waiting > to > > see > > when the reference queue's classloader is eligible for GC, which means > the > > client application no longer (strongly) refers to the queue. > > > > For this to work properly the worker thread must have no strong > references > > to the client application's classloader, otherwise it would indirectly > keep > > the > > queue alive. This is why Bob's code goes to great lengths to try and load > > the worker class in a separate classloader unconnected to the > application. > > > > While this works in most environments, there are two potential references > > that can occur when you spawn a thread from a container managed thread: > > > > 1) the new thread automatically picks up any InheritedThreadLocals > > > > 2) it gets the current AccessControlContext (only when security is on) > > > > These references are internal to the thread and cannot be removed using > > the public API or other runtime independent methods. So if either of > these > > situations occur the worker thread will not stop and you will see a > "leak". > > > > 1) could occur for many reasons, such as an application or container not > > removing its thread locals before returning a thread to the common pool. > > > > 2) is expected in almost all Java EE environments, hence what you see :( > > > > So... where do we go from here? > > > > Well, you could consider putting Guice on the system or boot classpath, > ie. > > tied to some classloader that's above the client classloader. So while > the > > worker thread will still have a reference to the system-level > classloader, > > it > > shouldn't stop the client classloader from being unloaded. The downside > is > > that you can't replace Guice without restarting the container - but this > > might > > be acceptable, given you're more likely to replace client apps than > > libraries > > > > [ at the very least you'd need the com.google.inject.internal.Finalizer > > class > > available on the system / container classpath, you don't need all of > Guice > > ] > > > > Another option is to remove the need for a worker thread, but I'm not > sure > > if > > this is feasible (there are alternative reference maps out there but YMMV > :) > > Note that you'll also have a separate worker thread for each application > > that > > jarjars the reference queue under a different package ( ie. if you use > Guice > > and google-collections you might end up with two worker threads! ) > > > > Alternatively, the google team could think again about exposing an API to > > shutdown this thread - or possibly provide a BundleActivator class that > > could manage the thread from an OSGi perspective, then you wouldn't be > > exposing a public API but would help anyone using an OSGi container* ;) > > > > [ * it would introduce a compile dep on OSGi for the library, but not > > clients ] > > > > The last resort would be to patch Guice to shutdown this thread and use > > this in your application rather than the official binary - but this may > not > > be > > an option for everyone ;) > > > > Of course once the JDK provides an inbuilt reference map these issues > > should disappear (and you'd only need one worker thread) - most of the > > problems stem from doing all of this outside of the JDK core classes! > > > > HTH > > > > Thanks, > > > > > Gili > > > > -- > > Cheers, Stuart > > > -- Cheers, Stuart --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "google-guice" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/google-guice?hl=en -~----------~----~----~----~------~----~------~--~---
