Classpath leak redux #253
Classpath leak redux #253
Conversation
… classes from URLs; need to hold Class results weakly too.
…and analyzing non-Groovy classes loaded from JARs too.
| String cp = CharacterPredicates.class.getProtectionDomain().getCodeSource().getLocation().toString(); // some JAR | ||
| p.getPublishersList().add(new TestGroovyRecorder( | ||
| new SecureGroovyScript(GroovyMemoryLeakTest.class.getName()+".register(this)", false, Collections.singletonList(new ClasspathEntry(cp))))); | ||
| String cp = GroovyMemoryLeakTest.class.getResource("somejar.jar").toString(); |
There was a problem hiding this comment.
So the problem with the test was that the previous class wasn't actually loaded from a separate jar?
There was a problem hiding this comment.
There were two problems with the previous revision of the test:
- The test did not actually load any classes from the specified classpath. So, it reproduced a leak when creating the outer cache, but not when adding entries to the inner cache. (Or perhaps dealt only with the resource cache, which does not suffer from the values-refers-to-key problem specific to the class cache fixed here, since a
URLholds no interesting references.) - The JAR file I used (
commons-textor something) was in the test classpath (Maventestscope), so any attempts to load classes from it would actually go throughsun.misc.AppClassLoaderinstead.
There was a problem hiding this comment.
reproduced a leak when creating the outer cache, but not when adding entries to the inner cache. (Or perhaps dealt only with the resource cache…
Amendment: the class cache normally has a bunch of entries, including positives like Object or WorkflowScript as well as negatives for all sorts of Groovy nonsense. But in the previous test, none of the positive entries were for classes actually defined in the URLClassLoader—they were defined by lower loaders—so the fact that they were strongly held from the cache was irrelevant.
There was a problem hiding this comment.
To make it clearer still: when you actually load a class from the classpath, there was until this fix a routine mistake of the sort that the WeakHashMap Javadoc warns about: a weakly held URLClassLoader key but then a strongly held Class value which in turn strongly holds the key. Pending my longstanding favorite JDK-6389107, there is no easy universal fix for this problem. Nor is there any feasible way that I am aware of to mechanically detect the mistake at runtime (some profilers detect it for WeakHashMap, probably not for Guava caches); the MemoryAssert shown here can find it, if you think to write the right test!
| } else if (loader instanceof ClasspathURLClassLoader) { | ||
| Collection<Class<?>> loadedClasses = ((ClasspathURLClassLoader) loader).loadedClasses; | ||
| synchronized (loadedClasses) { | ||
| loadedClasses = new ArrayList<>(loadedClasses); |
There was a problem hiding this comment.
Do you need to call ClasspathURLClassLoader.loadedClasses.clear() here? It's not obvious to me why the collection of strong references to the classes in ClasspathURLClassLoader is not problematic. Is it because we are only concerned about external references in caches, etc., and so any references from the class loaders to the classes they loaded are fine so long as the class loader is not strongly referenced anywhere?
There was a problem hiding this comment.
ClassLoader already has
// The classes loaded by this class loader. The only purpose of this table
// is to keep the classes from being GC'ed until the loader is GC'ed.
private final Vector<Class<?>> classes = new Vector<>();but it is inaccessible, so we need a custom subclass that makes a similar list accessible. GroovyClassLoader happens to have such a method already, which we were using. Also having a custom subclass helps us identify a loader created earlier by SecureGroovyScript (with a nonempty classpath), so that we know when to terminate the recursive parent search (normally it ends at UberClassLoader but the API does permit a plugin to pass in some other base loader, typically a PluginClassLoader).
...in/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java
Outdated
Show resolved
Hide resolved
...in/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java
Outdated
Show resolved
Hide resolved
| private static <T> LoadingCache<ClassLoader, Cache<String, T>> makeParentCache() { | ||
| return CacheBuilder.newBuilder().weakKeys().build(new CacheLoader<ClassLoader, Cache<String, T>>() { | ||
| private static <T> LoadingCache<ClassLoader, Cache<String, T>> makeParentCache(boolean weakValues) { | ||
| CacheBuilder<Object, Object> builder = CacheBuilder.newBuilder().weakKeys(); |
There was a problem hiding this comment.
Will again be a merge conflict with #160.
There was a problem hiding this comment.
Yes (assuming Caffeine also support weakValues), just FYI so it does not come as a surprise.
...in/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java
Outdated
Show resolved
Hide resolved
...in/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java
Outdated
Show resolved
Hide resolved
…ovy/SandboxResolvingClassLoader.java Co-Authored-By: Devin Nusbaum <dwnusbaum@users.noreply.github.com>
#252 was not enough; there was still a leak through
Classvalues in the cache. Also part of jenkinsci/workflow-cps-plugin#289 needed to be ported (and adapted) in order to clear some soft references in JAR classpath mode.