From a55b77b90e65e4b3b028daf922e3d1eadddbf7d1 Mon Sep 17 00:00:00 2001 From: John Wagenleitner Date: Tue, 3 May 2016 16:49:43 -0700 Subject: [PATCH 1/2] GROOVY-7683 - Test (do not merge) for demonstration purposes only --- src/test/groovy/bugs/Groovy7683Bug.groovy | 76 +++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 src/test/groovy/bugs/Groovy7683Bug.groovy diff --git a/src/test/groovy/bugs/Groovy7683Bug.groovy b/src/test/groovy/bugs/Groovy7683Bug.groovy new file mode 100644 index 00000000000..558203fd57e --- /dev/null +++ b/src/test/groovy/bugs/Groovy7683Bug.groovy @@ -0,0 +1,76 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 groovy.bugs + +import org.codehaus.groovy.reflection.ClassInfo + +import java.lang.ref.PhantomReference +import java.lang.ref.ReferenceQueue + +/** + * NOTE: Test uses multiple calls to System.gc() and throws an OutOfMemoryError in attempt to + * demonstrate changes fix the issue. This test should not be included if the PR is accepted. + */ +class Groovy7683Bug extends GroovyTestCase { + + private static final int NUM_OBJECTS = 31 + + ReferenceQueue classLoaderQueue = new ReferenceQueue() + ReferenceQueue> classQueue = new ReferenceQueue>() + ReferenceQueue classInfoQueue = new ReferenceQueue() + + // Used to keep a hard reference to the PhantomReferences so they are not collected + List refList = new ArrayList(NUM_OBJECTS) + + void testLeak() { + assert !Boolean.getBoolean('groovy.use.classvalue') + for (int i = 0; i < NUM_OBJECTS; i++) { + GroovyClassLoader gcl = new GroovyClassLoader() + Class scriptClass = gcl.parseClass("int myvar = " + i) + ClassInfo ci = ClassInfo.getClassInfo(scriptClass) + PhantomReference classLoaderRef = new PhantomReference<>(gcl, classLoaderQueue) + PhantomReference> classRef = new PhantomReference>(scriptClass, classQueue) + PhantomReference classInfoRef = new PhantomReference(ci, classInfoQueue) + refList.add(classLoaderRef) + refList.add(classRef) + refList.add(classInfoRef) + System.gc() + } + System.gc() + // Encourage GC to collect soft references + try { throw new OutOfMemoryError() } catch(OutOfMemoryError oom) { } + System.gc() + + // Ensure that at least 90% of objects should have been collected, we can't guarantee 100% because + // System.gc() is not guaranteed to run on each call. + int targetCollectedCount = Math.floor(NUM_OBJECTS * 0.9f) + assert queueSize(classLoaderQueue) >= targetCollectedCount //GroovyClassLoaders not collected by GC + assert queueSize(classQueue) >= targetCollectedCount //Script Classes not collected by GC + assert queueSize(classInfoQueue) >= targetCollectedCount //ClassInfo objects not collected by GC + } + + private int queueSize(ReferenceQueue queue) { + int size = 0 + while (queue.poll() != null) { + ++size + } + return size + } + +} \ No newline at end of file From f7c688e101af77170448cf0be9103c4e6dac3f74 Mon Sep 17 00:00:00 2001 From: John Wagenleitner Date: Tue, 3 May 2016 16:51:03 -0700 Subject: [PATCH 2/2] GROOVY-7683 - Memory leak when using Groovy as JSR-223 scripting language --- .../codehaus/groovy/reflection/ClassInfo.java | 84 ++++++++++--------- .../reflection/GroovyClassValuePreJava7.java | 10 +++ 2 files changed, 53 insertions(+), 41 deletions(-) diff --git a/src/main/org/codehaus/groovy/reflection/ClassInfo.java b/src/main/org/codehaus/groovy/reflection/ClassInfo.java index 2c0d8f499f0..faa599864b5 100644 --- a/src/main/org/codehaus/groovy/reflection/ClassInfo.java +++ b/src/main/org/codehaus/groovy/reflection/ClassInfo.java @@ -25,6 +25,7 @@ import org.codehaus.groovy.util.*; import org.codehaus.groovy.vmplugin.VMPluginFactory; +import java.lang.ref.WeakReference; import java.math.BigDecimal; import java.math.BigInteger; import java.util.*; @@ -32,16 +33,23 @@ /** * Handle for all information we want to keep about the class + *

+ * This class handles caching internally and its advisable to not store + * references directly to objects of this class. The static factory method + * {@link ClassInfo#getClassInfo(Class)} should be used to retrieve an instance + * from the cache. Internally the {@code Class} associated with a {@code ClassInfo} + * instance is kept as {@link WeakReference}, so it not safe to reference + * and instance without the Class being either strongly or softly reachable. * * @author Alex.Tkachman */ -public class ClassInfo { +public class ClassInfo implements Finalizable { private final LazyCachedClassRef cachedClassRef; private final LazyClassLoaderRef artifactClassLoader; private final LockableObject lock = new LockableObject(); public final int hash = -1; - private final Class klazz; + private final WeakReference> klazz; private final AtomicInteger version = new AtomicInteger(); @@ -68,11 +76,7 @@ public ClassInfo computeValue(Class type) { private static final GlobalClassSet globalClassSet = new GlobalClassSet(); ClassInfo(Class klazz) { - this.klazz = klazz; - if (ClassInfo.DebugRef.debug) - new DebugRef(klazz); - new ClassInfoCleanup(this); - + this.klazz = new WeakReference>(klazz); cachedClassRef = new LazyCachedClassRef(softBundle, this); artifactClassLoader = new LazyClassLoaderRef(softBundle, this); } @@ -103,6 +107,19 @@ public static void clearModifiedExpandos() { } } + /** + * Returns the {@code Class} associated with this {@code ClassInfo}. + *

+ * This method can return {@code null} if the {@code Class} is no longer reachable + * through any strong or soft references. A non-null return value indicates that this + * {@code ClassInfo} is valid. + * + * @return the {@code Class} associated with this {@code ClassInfo}, else {@code null} + */ + public final Class getTheClass() { + return klazz.get(); + } + public CachedClass getCachedClass() { return cachedClassRef.get(); } @@ -222,7 +239,7 @@ private MetaClass getMetaClassUnderLock() { return answer; } - answer = mccHandle.create(klazz, metaClassRegistry); + answer = mccHandle.create(klazz.get(), metaClassRegistry); answer.initialize(); if (GroovySystem.isKeepJavaMetaClasses()) { @@ -248,6 +265,17 @@ private static boolean isValidWeakMetaClass(MetaClass metaClass, MetaClassRegist return (!enableGloballyOn || cachedAnswerIsEMC); } + /** + * Returns the {@code MetaClass} for the {@code Class} associated with this {@code ClassInfo}. + * If no {@code MetaClass} exists one will be created. + *

+ * It is not safe to call this method without a {@code Class} associated with this {@code ClassInfo}. + * It is advisable to aways retrieve a ClassInfo instance from the cache by using the static + * factory method {@link ClassInfo#getClassInfo(Class)} to ensure the referenced Class is + * strongly reachable. + * + * @return a {@code MetaClass} instance + */ public final MetaClass getMetaClass() { MetaClass answer = getMetaClassForClass(); if (answer != null) return answer; @@ -375,7 +403,7 @@ private static class LazyCachedClassRef extends LazyReference { } public CachedClass initValue() { - return createCachedClass(info.klazz, info); + return createCachedClass(info.klazz.get(), info); } } @@ -388,43 +416,17 @@ private static class LazyClassLoaderRef extends LazyReference { - - public ClassInfoCleanup(ClassInfo classInfo) { - super(weakBundle, classInfo); - } - public void finalizeRef() { - ClassInfo classInfo = get(); - classInfo.setStrongMetaClass(null); - classInfo.cachedClassRef.clear(); - classInfo.artifactClassLoader.clear(); - } + @Override + public void finalizeReference() { + setStrongMetaClass(null); + cachedClassRef.clear(); + artifactClassLoader.clear(); } - private static class DebugRef extends ManagedReference { - public static final boolean debug = false; - - private static final AtomicInteger count = new AtomicInteger(); - - final String name; - - public DebugRef(Class klazz) { - super(softBundle, klazz); - name = klazz == null ? "" : klazz.getName(); - count.incrementAndGet(); - } - - public void finalizeRef() { - //System.out.println(name + " unloaded " + count.decrementAndGet() + " classes kept"); - super.finalizeReference(); - } - } - private static class GlobalClassSet { private final ManagedLinkedList items = new ManagedLinkedList(weakBundle); diff --git a/src/main/org/codehaus/groovy/reflection/GroovyClassValuePreJava7.java b/src/main/org/codehaus/groovy/reflection/GroovyClassValuePreJava7.java index e569a23873a..56ad0c16bc5 100644 --- a/src/main/org/codehaus/groovy/reflection/GroovyClassValuePreJava7.java +++ b/src/main/org/codehaus/groovy/reflection/GroovyClassValuePreJava7.java @@ -18,6 +18,7 @@ */ package org.codehaus.groovy.reflection; +import org.codehaus.groovy.util.Finalizable; import org.codehaus.groovy.util.ManagedConcurrentMap; import org.codehaus.groovy.util.ReferenceBundle; @@ -40,6 +41,15 @@ public EntryWithValue(GroovyClassValuePreJava7Segment segment, Class key, int public void setValue(T value) { if(value!=null) super.setValue(value); } + + @Override + public void finalizeReference() { + T value = getValue(); + if (value instanceof Finalizable) { + ((Finalizable) value).finalizeReference(); + } + super.finalizeReference(); + } } private class GroovyClassValuePreJava7Segment extends ManagedConcurrentMap.Segment,T> {