[JENKINS-59587] Use weakValues on the inner caches in SandboxResolvingClassLoader.parentClassCache instead of on the outer cache

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java

@@ -23,9 +23,9 @@ class SandboxResolvingClassLoader extends ClassLoader {
 
     private static final Logger LOGGER = Logger.getLogger(SandboxResolvingClassLoader.class.getName());
 
-    private static final LoadingCache<ClassLoader, Cache<String, Class<?>>> parentClassCache = makeParentCache(true);
+    static final LoadingCache<ClassLoader, Cache<String, Class<?>>> parentClassCache = makeParentCache(true);
 
-    private static final LoadingCache<ClassLoader, Cache<String, Optional<URL>>> parentResourceCache = makeParentCache(false);
+    static final LoadingCache<ClassLoader, Cache<String, Optional<URL>>> parentResourceCache = makeParentCache(false);
 
     SandboxResolvingClassLoader(ClassLoader parent) {
         super(parent);
@@ -38,7 +38,7 @@ class SandboxResolvingClassLoader extends ClassLoader {
      * This value is non-null, not a legitimate return value
      * (no script should be trying to load this implementation detail), and strongly held.
      */
-    private static final Class<?> CLASS_NOT_FOUND = Unused.class;
+    static final Class<?> CLASS_NOT_FOUND = Unused.class;
     private static final class Unused {}
 
     @Override protected synchronized Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
@@ -93,12 +93,20 @@ private static <T> T load(LoadingCache<ClassLoader, Cache<String, T>> cache, Str
         });
     }
 
-    private static <T> LoadingCache<ClassLoader, Cache<String, T>> makeParentCache(boolean weakValues) {
-        Caffeine<Object, Object> builder = Caffeine.newBuilder().weakKeys();
-        if (weakValues) {
-            builder = builder.weakValues();
+    private static <T> LoadingCache<ClassLoader, Cache<String, T>> makeParentCache(boolean weakValuesInnerCache) {
+        // The outer cache has weak keys, so that we do not leak class loaders, but strong values, because the
+        // inner caches are only referenced by the outer cache internally.
+        Caffeine<Object, Object> outerBuilder = Caffeine.newBuilder().weakKeys();
+        // The inner cache has strong keys, since they are just strings, and expires entries 15 minutes after they are
+        // added to the cache, so that classes defined by dynamically installed plugins become available even if there
+        // were negative cache hits prior to the installation (ideally this would be done with a listener). The values
+        // for the inner cache may be weak if needed, for example parentClassCache uses weak values to avoid leaking
+        // classes and their loaders.
+        Caffeine<Object, Object> innerBuilder = Caffeine.newBuilder().expireAfterWrite(Duration.ofMinutes(15));
+        if (weakValuesInnerCache) {
+            innerBuilder.weakValues();
         }
 
-        return builder.build(parentLoader -> Caffeine.newBuilder()./* allow new plugins to be used, and clean up memory */expireAfterWrite(Duration.ofMinutes(15)).build());
+        return outerBuilder.build(parentLoader -> innerBuilder.build());
     }
 }

src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java

@@ -0,0 +1,59 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2019 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;
+
+import org.junit.Test;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxResolvingClassLoader.CLASS_NOT_FOUND;
+import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxResolvingClassLoader.parentClassCache;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+
+public class SandboxResolvingClassLoaderTest {
+
+    private final ClassLoader parentLoader = SandboxResolvingClassLoaderTest.class.getClassLoader();
+    private final SandboxResolvingClassLoader loader = new SandboxResolvingClassLoader(parentLoader);
+
+    @Test public void classCacheDoesNotHoldClassValuesTooWeakly() throws Exception {
+        // Load a class that does exist.
+        assertThat(loader.loadClass("java.lang.String", false), equalTo(String.class));
+        // Load a class that does not exist.
+        try {
+            loader.loadClass("this.does.not.Exist", false);
+            fail("Class should not exist");
+        } catch (ClassNotFoundException e) {
+            assertThat(e.getMessage(), containsString("this.does.not.Exist"));
+        }
+        // The result of both of the class loading attempts should exist in the cache.
+        assertThat(parentClassCache.get(parentLoader).getIfPresent("java.lang.String"), equalTo(String.class));
+        assertThat(parentClassCache.get(parentLoader).getIfPresent("this.does.not.Exist"), equalTo(CLASS_NOT_FOUND));
+        // Make sure that both of the entries are still in the cache after a GC.
+        System.gc();
+        assertThat(parentClassCache.get(parentLoader).getIfPresent("java.lang.String"), equalTo(String.class));
+        assertThat(parentClassCache.get(parentLoader).getIfPresent("this.does.not.Exist"), equalTo(CLASS_NOT_FOUND));
+    }
+}