281067 - ThreadLocal in Context prevents class unloading

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Attila Szegedi]

User-Agent:       Opera/7.54 (Windows NT 5.0; U)  [en]
Build Identifier: 

I have an environment where most of the system (including Rhino) is hot-
reloadable. The classes are loaded through a particular java.net.URLClassLoader, 
and upon detecting a change, the class loader is thrown away, and a new one is 
created, allowing for the classes to be reloaded. However, a small issue in 
Rhino's Context class prevents the throwaway class loader (and all the classes 
it loads) from being garbage collected, resulting in memory leak over time.

The issue is the ThreadLocal "Context[] storage". Since the type of the value in 
ThreadLocal is org.mozilla.javascript.Context[], the class org.mozilla.
javascript.Context will remain permanently loaded, and will keep the class 
loader from being garbage collected -- we're utilizing a permanent set of 
threads, actually they're created by JMS sessions, so we can't just terminate 
and restart the threads to get rid of the stuck thread locals. However, there is 
a brilliantly simple solution - store an Object[] instead of Context[] in the 
thread local. Granted, we sacrifice a bit of static type safety, but Rhino will 
no longer prevent the class loader that loaded it from being garbage collected.
Patch on the way.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Attila Szegedi]

Attachment: The patch that solves the problem

Patch attached

Comment 2

[Igor Bukanov]

I thought that Java used week references to store ThreadLocal in Thread
instances. Apparently it does not which also means that even with the patch
after Rhino reloading all those ThreaLocal instances from reloaded Rhino classes
would stuck in memory forever.

Of cause it is much better then forever stuck Rhino classes themselves so I will
commit the patch after regression testing. 

Comment 3

[Igor Bukanov]

I committed the patch. 

BTW, since you repeatedly reload Rhino classes, did the patch address memory
leak completely on your JVM? Or do you still see some leaks like the one
described in 
https://bugzilla.mozilla.org/show_bug.cgi?id=243305#c9 ?

Comment 4

[Attila Szegedi]

(In reply to comment #3)
> 
> BTW, since you repeatedly reload Rhino classes, did the patch address memory
> leak completely on your JVM? Or do you still see some leaks like the one
> described in 
> https://bugzilla.mozilla.org/show_bug.cgi?id=243305#c9 ?

I'll run it through a profiler now (been profiling all day :-) ). However, what 
you say is certainly true - the ThreadLocal object and the Object[] will get 
stuck in the thread's local table... Hm... maybe in the end I'll really have to 
introduce a feature for perodically shutting down and recreating the JMS 
sessions...

Comment 5

[Attila Szegedi]

(In reply to comment #3)
> I committed the patch. 
> 
> BTW, since you repeatedly reload Rhino classes, did the patch address memory
> leak completely on your JVM? Or do you still see some leaks like the one
> described in 
> https://bugzilla.mozilla.org/show_bug.cgi?id=243305#c9 ?

No, I see no leaks now. As a matter of fact, Thread indeed holds the ThreadLocal 
objects using weak references. However, weak references are defeated if the 
value - directly or indirectly - refers back strongly to the key. In the case 
with storing a Context[], we had a strong reference from value to key via the 
path value = Context[] instance -> Context[].class -> Context.class -> Context.
threadLocalCx = key. Storing an Object[] instead of Context[] solved it for good 
:-). Provided the array element is null (which is easy by using a Context.exit() 
in a finally block), the ThreadLocal object can be fully garbage collected after 
the Context class is unloaded.

Comment 6

[Attila Szegedi]

Thinking about it, I believe it'd be necessary to provide a comment around 
"storage = new Object[1];" explaining why do we use it instead of Context[] - 
it's not immediately apparent, and some otherwise well-meaning soul (who comes 
to maintain it sometime in the future after we all moved along) unsuspectingly 
rewrites it back to Context[]...

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> As a matter of fact, Thread indeed holds the ThreadLocal 
> objects using weak references. However, weak references are defeated if the 
> value - directly or indirectly - refers back strongly to the key. In the case 
> with storing a Context[], we had a strong reference from value to key via the 
> path value = Context[] instance -> Context[].class -> Context.class -> Context.
> threadLocalCx = key.


But why the reference from Context.threadLocalCx prevents in your case Rhino
classes from garbage collection? If ThreadLocal value is reachable from Thread
only through week reference, it should allow to GC classes as long as the last
strong reference to them and their class loader is gone.

Comment 8

[Attila Szegedi]

(In reply to comment #7)
> 
> But why the reference from Context.threadLocalCx prevents in your case Rhino
> classes from garbage collection? If ThreadLocal value is reachable from Thread
> only through week reference, it should allow to GC classes as long as the last
> strong reference to them and their class loader is gone.

No, only the keys in the WeakHashMap are weakly reachable (in our case, the 
ThreadLocal object). The values in WeakHashMap are strongly reachable. And if 
the value is a Context[], it'll establish a strong reference to the ThreadLocal 
key, as outlined in comment #5, preventing it to be GC-ed for the lifetime of 
the thread.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
>... only the keys in the WeakHashMap are weakly reachable (in our case, the 
> ThreadLocal object). The values in WeakHashMap are strongly reachable. And if 
> the value is a Context[], it'll establish a strong reference to the ThreadLocal 
> key, as outlined in comment #5, preventing it to be GC-ed for the lifetime of 
> the thread.

I will add then comments about using Object[], not Context[] after I will finish
with bug 281247.