Skip to content

Commit

Permalink
Fix named instantiator leaking memory
Browse files Browse the repository at this point in the history
This commit reworks the named object instantiator so that it doesn't
leak memory, by locating the cache on the `VisibleURLClassLoader`
instance whenever possible. Previously, it retained strong references
to classes, meaning that any class loaded via a script classloader
would be strongly referenced and would prevent the loader from being
collected. With this change, the cache is now located on the loader
itself.

If, for some reason, the loader is not a known classloader, we will
still use a global cache, but this shouldn't leak memory anymore
since in this case it's likely the affected classes come from Gradle
core itself.

Fixes #8142
  • Loading branch information
melix committed Jan 3, 2019
1 parent 0c70d91 commit 0217dbf
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 7 deletions.
Expand Up @@ -16,12 +16,15 @@

package org.gradle.internal.classloader;

import org.gradle.internal.Cast;
import org.gradle.internal.Factory;
import org.gradle.internal.classpath.ClassPath;

import java.net.URL;
import java.net.URLClassLoader;
import java.util.Arrays;
import java.util.Collection;
import java.util.EnumMap;
import java.util.List;

public class VisitableURLClassLoader extends URLClassLoader implements ClassLoaderHierarchy {
Expand All @@ -33,6 +36,28 @@ public class VisitableURLClassLoader extends URLClassLoader implements ClassLoad
}
}

private final EnumMap<UserData, Object> userData = new EnumMap<UserData, Object>(UserData.class);

public enum UserData {
NAMED_OBJECT_INSTANTIATOR
}

/**
* This method can be used to store user data that should live among with this classloader
* @param user the consumer
* @param onMiss called to create the initial data, when not found
* @param <T> the type of data
* @return user data
*/
public synchronized <T> T getUserData(UserData user, Factory<T> onMiss) {
if (userData.containsKey(user)) {
return Cast.uncheckedCast(userData.get(user));
}
T value = onMiss.create();
userData.put(user, value);
return value;
}

// TODO:lptr When we drop Java 8 support we can switch to using ClassLoader.getName() instead of storing our own
private final String name;

Expand Down
Expand Up @@ -24,7 +24,9 @@
import org.gradle.api.GradleException;
import org.gradle.api.Named;
import org.gradle.api.reflect.ObjectInstantiationException;
import org.gradle.internal.Factory;
import org.gradle.internal.UncheckedException;
import org.gradle.internal.classloader.VisitableURLClassLoader;
import org.gradle.model.internal.asm.AsmClassGenerator;
import org.gradle.model.internal.inspect.FormattingValidationProblemCollector;
import org.gradle.model.internal.inspect.ValidationProblemCollector;
Expand All @@ -37,7 +39,10 @@
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;

import static org.objectweb.asm.Opcodes.*;
import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
import static org.objectweb.asm.Opcodes.V1_5;

public class NamedObjectInstantiator {
public static final NamedObjectInstantiator INSTANCE = new NamedObjectInstantiator();
Expand All @@ -54,17 +59,38 @@ public class NamedObjectInstantiator {
private static final String[] EMPTY_STRINGS = new String[0];
private static final String CONSTRUCTOR_NAME = "<init>";

// Currently retains strong references to types
private final LoadingCache<Class<?>, LoadingCache<String, Object>> values = CacheBuilder.newBuilder().build(new CacheLoader<Class<?>, LoadingCache<String, Object>>() {
private final Factory<LoadingCache<Class<?>, LoadingCache<String, Object>>> cacheFactory = new Factory<LoadingCache<Class<?>, LoadingCache<String, Object>>>() {
@Override
public LoadingCache<String, Object> load(Class<?> type) {
return CacheBuilder.newBuilder().build(loaderFor(type));
public LoadingCache<Class<?>, LoadingCache<String, Object>> create() {
return newValuesCache();
}
});
};

// Currently retains strong references to types
private final LoadingCache<Class<?>, LoadingCache<String, Object>> leakyValues = newValuesCache();

private LoadingCache<Class<?>, LoadingCache<String, Object>> newValuesCache() {
return CacheBuilder.newBuilder()
.weakKeys()
.build(new CacheLoader<Class<?>, LoadingCache<String, Object>>() {
@Override
public LoadingCache<String, Object> load(Class<?> type) {
return CacheBuilder.newBuilder().build(loaderFor(type));
}
});
}

private LoadingCache<Class<?>, LoadingCache<String, Object>> getCacheScope(Class<?> type) {
ClassLoader classLoader = type.getClassLoader();
if (classLoader instanceof VisitableURLClassLoader) {
return ((VisitableURLClassLoader) classLoader).getUserData(VisitableURLClassLoader.UserData.NAMED_OBJECT_INSTANTIATOR, cacheFactory);
}
return leakyValues;
}

public <T extends Named> T named(final Class<T> type, final String name) throws ObjectInstantiationException {
try {
return type.cast(values.getUnchecked(type).getUnchecked(name));
return type.cast(getCacheScope(type).getUnchecked(type).getUnchecked(name));
} catch (UncheckedExecutionException e) {
throw new ObjectInstantiationException(type, e.getCause());
}
Expand Down
Expand Up @@ -77,4 +77,25 @@ class ClassLoaderLeakAvoidanceSoakTest extends AbstractIntegrationSpec {
assert succeeds("myTask")
}
}

def "named object instantiator should not prevent classloader from being collected"() {
given:
buildFile << """
interface Foo0 extends Named {
}
task myTask() {
doLast {
def instance = project.objects.named(Foo0, new String(new byte[10 * 1024 * 1024], "UTF-8"))
println "\${instance.class.name}"
}
}
"""

expect:
for(int i = 0; i < 35; i++) {
buildFile.text = buildFile.text.replace("Foo$i", "Foo${i + 1}")
executer.withBuildJvmOpts("-Xmx64m", "-XX:+HeapDumpOnOutOfMemoryError", "-XX:MaxMetaspaceSize=64m")
assert succeeds("myTask")
}
}
}

0 comments on commit 0217dbf

Please sign in to comment.