GROOVY-7683 - Memory leak when using Groovy as JSR-223 scripting language

[jwagenleitner]

I am unsure what if any problems making the Class a WeakReference might have but thought I'd put this out there for review.

I also tested against GROOVY-7646 which was consistently throwing OOME after <2 mins prior to these changes and ran to completion with changes.

Another thing I noticed in looking at this class is that uses a PhantomReference and attempts to use the value from get(). However, this will always return null so the code path never executes.

[blackdrag]

a couple of comments:
(1) The PhantomReference: I think you are right, the code as it is makes no sense, instead it should maybe use a WeakReference... but looking more at it, I think that check is actually not needed and recentThreadMapRef should be removed and keep only the else block of that if condition. I don't think we really do need that kind of optimization at that point, especially with those questionable semantics
(2) As for finalizeReference vs finalizeRef. That is in fact a good find.
(3) Because of the sensitive nature of things here it would be actually good to have those 3 changes (method rename, change to recentThreadMapRef, weakly reference the class) as separate issue, which we can test in isolation then, to know what change will help and what not.

[jwagenleitner]

(1) The PhantomReference:

I will look more closely at that threalocal local map and submit a separate PR. At the very least I do think the else and PhantomReference should be removed.

(2) As for finalizeReference vs finalizeRef.

I have submitted a separate PR #220 for the finalizeReference changes

(3) Because of the sensitive nature of things here it would be actually good to have those 3 changes (method rename, change to recentThreadMapRef, weakly reference the class) as separate issue

If the method renames look good I will rebase this PR and will send in another for the local map.

[blackdrag]

At the very least I do think the else and PhantomReference should be removed.

Funny, that we see this different... you want to remove the PhantomReference, I want to remove the whole cache aspect of the reference. The result is to get the ThreadLocal every time in my case and not try to get the map directly, in case the current thread and the referenced thread are the same.

Anyway if the thread is referenced, it must not be a strong reference. That's why WeakReference seems to be the next best to me, if we don't remove the whole logic. I mean current logic is to more or less ignore that map reference and go through the ThreadLocal.

[jwagenleitner]

I don't see the purpose of the ThreadLocal and LocalMap at all. It seems to just proxy through to the globalClassValue and I don't ever see it getting set with any values. So while you say remove the whole cache I am thinking remove the whole ThreadLocal and LocalMap altogether. But it's 2am here so maybe a fresh look in the morning will change my mind.

[jwagenleitner]

Submitted PR #222 to address the PhantomReference/ThreadLocalMap issue. I could not figure out what the use of a threadlocal cache would be other than maybe to eliminate/reduce lock contention when accessing the global cache. However, I think it would introduce several potential problems such as another potential to hold refs to class/classinfo and possibly causing problems in thread pool environments.

As it stands I don't think the current implementation is working as a local cache so rather than address the PhantomReference and its use in the ThreadLocal subclass it seemed like removal was the way to go.

[jwagenleitner]

This change bothers me since changing the klazz to be a weak reference means it could disappear any time during the ClassInfo's lifetime. Since the lazy refs that access the klazz are softly held, the klazz should probably be at least a soft reference.

[blackdrag]

I too would like to see if the other changes fix the memory leak already and then skip the weak reference change. In theory you will need the class to access the ClassInfo, thus the class cannot have been collected in that case. That is if you want to do anything useful with the ClassInfo. Of course it could still happen if you do not use the class to get the ClassInfo object... and while rare, we may introduce null exceptions in parts of the code, that is not aware of it, plus triggering this in only rare cases

[jwagenleitner]

I tested 7683 and 7646 with the combined changes from PR's #219 / #220 / #222 but without the WeakReference for ClassInfo.klazz (i.e., with a hard ref) and still see the leak in 7683 and the OOME in 7646.

I also test with SoftReferences for both ClassInfo.klazz and the GroovyClassValuePreJava7 map and it also works as does using a WeakReference. Though I would need to do some more checking with the SoftRef.

I do think a WeakReference is correct in this case, just not sure what cases may possibly ever result in an NPE and what if any handling should be done for it if any.

[PascalSchumacher]

So I guess we should commit this change?

Maybe I even fixes some to of the other memory leak issues reported in jira.

@melix What is you opinion on this?

[jwagenleitner]

There is a thread on the mlvm-dev list (http://mail.openjdk.java.net/pipermail/mlvm-dev/2016-January/006563.html) about the ClassValue bug that is related to this and some good information was provided by Peter. I'm hoping to get some time to take a closer look and see if it might be possible to revert back to using ClassValue as the default if available. But even with CV without these changes we'd still have a leak with jdk6.

It would be nice if we could merge PR #220 since these changes rely on those being in place.

[jwagenleitner]

I also tested these changes against the test script provided in GROOVY-7621 and do not get OOME and all iterations complete.

[jwagenleitner]

Rebased to merge changes that were commited as part of PR #220.

[PascalSchumacher]

So shouldn't this be merged?

[jochenberger]

I wonder why the Jenkins build failed so horribly. Can we trigger a rebuild, maybe by rebasing the PR?

[jwagenleitner]

Rebased and CI failed for jdk6 and passed for jdk7. This is partly because the test in this PR (the one that failed) is trying to force a full GC and for jdk6 the failure was GroovyClassLoaders not collected by GC expected:<31> but was:<26>.

[blackdrag]

Why did you remove this call?

[jwagenleitner]

ClassInfo#finalizeReference calls setStrongMetaClass(null) which in turn calls replaceWeakMetaClassRef(null) which calls weakRef#clear which is on ManagedReference. This causes a recursive call back into the queue can can lead to stackoverflow if there are lots of ClassInfo ManagedReferences in the Reference Queue.

A related PR that attempts to prevent this reentrant processing is PR #298.

[blackdrag]

ok, I +1ed PR #298

[jwagenleitner]

Thanks for reviewing that other PR. I merged that PR and will rebase this and leave the call to manager.removeStallEntries().

[jwagenleitner]

I cleaned up the PR based on the merging of PR #298 and added javadocs with cautions about ensuring the Class is strongly/softly reachable. Also added a getTheClass() method as suggested.

[jwagenleitner]

@blackdrag just wondering what you think about the state of this PR, is there something else you think needs to be addressed? Would be nice to get it in time for a 2.4.7/2.5.0 beta, but if you think the weak ref will cause problems then I can close this.

There has been some recent discussion on the dev mailing list about this under the title Improve Groovy class loading performance and memory management, wanted to link to it from this PR for reference.

[jkandasa]

@blackdrag @jwagenleitner Could you please review this PR, If there is no comments can you merge this one? We are waiting for this PR. Thank you!

[henryptung]

Maybe I'm thinking about this the wrong way, but shouldn't we instead be leaving ClassInfo strongly referencing Class, but try to establish a circular reference loop between ClassInfo and Class? Then we could just make the map weak-valued, and both sides will become weakly-referenced simultaneously. Like others, I think a ClassInfo that may or may not reference its corresponding Class is super-prone to surprising errors.

UPDATE: Never mind, guessing that's not actually possible, and weakly referencing Class is probably the next best thing.

[blackdrag]

How do you make for example String reference ClassInfo?

[henryptung]

@blackdrag I was thinking of what the JVM ClassValue is. It has the same bug as this one, just in reverse (weak hash map of ClassValue to stored value, but stored value cannot reference ClassValue). AFAICT, there's no natural JVM mechanism that allows a mapping (a, b) -> c where both a and b are collectable and c can strongly reference both a and b. (Makes me think Java should have a LinkingReference<A, B> which, if alive, makes A strongly reference B, but meh.)

That said, if I understand correctly, ClassInfo will store the MetaClass from Groovy, right? Even if we make the Class<?> a weak reference, wouldn't adding a strong reference to the MetaClass (in Groovy script code) just reproduce the problem? What about a reference to another Class<?> from the same classloader as klazz? I assume we can't make the MetaClass a weak reference, lest we forget the bindings from the script at runtime.

[jwagenleitner]

I believe the default metaclass is weak so it won't cause a problem with GC. Only when methods are added and the default is replaced by an ExpandoMetaclass does it become strongly referenced. In that case it may require the programmer to indicate the metaclass is no longer needed by setting it to null. And as long as another class from the same class loader is referenced then all classes from that CL will be strongly referenced (by the CL)

[henryptung]

Haven't been here in a while, but I just want to comment here regarding how my use case looks now with more context. In particular, I want to have a job runner which executes scripts loaded dynamically; the job runner should provide Groovy on classpath and provide a clean execution slate to each script as it comes in, for statelessness.

Thinking back to this bug, it seems like the current way metaclass is implemented will cause any metaclass modifications in the script to "build up" over time, particularly modifications to base classes. Is this intended, that changing a metaclass constitutes "static state" from Groovy's perspective? Or could we have the notion of a GroovyRuntime instance encapsulating state for execution of one (or more related) scripts, but which can be swapped for a new, fresh instance when desired.

Not sure which way to go makes more sense, but would be interested in a word from Groovy expert folks about whether metaclass manipulation is considered "permanent static modification" or "contextual static modification", with replaceable context. Maybe we could support both in a Groovy 3.0? 😛

EDIT: There's also an element of thread-safety here, which adds to what I'm thinking about. Currently, if I understand correctly, metaclass modifications are visible across threads, causing (potentially unexpected) behavior. If that is the case, I'm OK with the fix as is (default weak metaclass) and recommending my users to avoid all metaclass manipulation, but want to make sure I'm understanding this correctly.