Merge pull request #370 from jglick/ClassValue

Use `ClassValue` for caching

core/src/main/java/org/kohsuke/stapler/AnnotationHandler.java

@@ -23,12 +23,11 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.commons.beanutils.Converter;
 
 import javax.servlet.ServletException;
 import java.lang.annotation.Annotation;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 
 /**
  * Handles stapler parameter annotations by determining what values to inject for a method call.
@@ -68,20 +67,6 @@ static Object handle(StaplerRequest request, Annotation[] annotations, String pa
         for (Annotation a : annotations) {
             Class<? extends Annotation> at = a.annotationType();
             AnnotationHandler h = HANDLERS.get(at);
-            if (h==null) {
-                InjectedParameter ip = at.getAnnotation(InjectedParameter.class);
-                if (ip!=null) {
-                    try {
-                        h = ip.value().newInstance();
-                    } catch (InstantiationException | IllegalAccessException e) {
-                        throw new ServletException("Failed to instantiate parameter injector for "+at,e);
-                    }
-                } else {
-                    h = NOT_HANDLER;
-                }
-                AnnotationHandler prev = HANDLERS.putIfAbsent(at, h);
-                if (prev!=null) h=prev;
-            }
             if (h==NOT_HANDLER)
                 continue;
             return h.parse(request,a,targetType,parameterName);
@@ -90,7 +75,22 @@ static Object handle(StaplerRequest request, Annotation[] annotations, String pa
         return null; // probably we should report an error
     }
 
-    private static final ConcurrentMap<Class<? extends Annotation>,AnnotationHandler> HANDLERS = new ConcurrentHashMap<>();
+    private static final ClassValue<AnnotationHandler> HANDLERS = new ClassValue<AnnotationHandler>() {
+        @SuppressFBWarnings(value = "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", justification = "shut up")
+        @Override
+        protected AnnotationHandler computeValue(Class<?> at) {
+            InjectedParameter ip = at.getAnnotation(InjectedParameter.class);
+            if (ip != null) {
+                try {
+                    return ip.value().newInstance();
+                } catch (InstantiationException | IllegalAccessException e) {
+                    throw new RuntimeException("Failed to instantiate parameter injector for " + at, e);
+                }
+            } else {
+                return NOT_HANDLER;
+            }
+        }
+    };
 
     private static final AnnotationHandler NOT_HANDLER = new AnnotationHandler() {
         @Override

core/src/main/java/org/kohsuke/stapler/ClassLoaderValue.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2022, CloudBees, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without modification, are permitted provided
+ * that the following conditions are met:
+ *
+ *     * Redistributions of source code must retain the above copyright notice, this list of
+ *       conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright notice, this list of
+ *       conditions and the following disclaimer in the documentation and/or other materials
+ *       provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS
+ * OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
+ * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA,
+ * OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
+ * IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.kohsuke.stapler;
+
+import java.lang.reflect.Proxy;
+
+/**
+ * Like {@link ClassValue} but for a whole {@link ClassLoader}.
+ */
+abstract class ClassLoaderValue<T> {
+
+    private final ClassValue<T> storage = new ClassValue<T>() {
+        @Override
+        protected T computeValue(Class<?> type) {
+            return ClassLoaderValue.this.computeValue(type.getClassLoader());
+        }
+    };
+
+    protected abstract T computeValue(ClassLoader cl);
+
+    // A Class and its ClassLoader strongly refer to one another,
+    // so we can delegate to ClassValue if we can supply a Class loaded by a given ClassLoader,
+    // which is most easily done by asking for a Proxy of an otherwise unused interface.
+    public interface X {}
+
+    public final T get(ClassLoader cl) {
+        try {
+            cl.loadClass(X.class.getName());
+            // OK, X is visible to cl, can use trick
+        } catch (ClassNotFoundException x) {
+            // fallback, no caching; could use WeakHashMap though typically values would strongly hold keys so both could leak
+            return computeValue(cl);
+        }
+        Class<?> type = Proxy.getProxyClass(cl, X.class);
+        assert type.getClassLoader() == cl;
+        return storage.get(type);
+    }
+
+}

core/src/main/java/org/kohsuke/stapler/MetaClassLoader.java

@@ -25,12 +25,11 @@
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
-import java.util.Map;
 import java.io.File;
+import java.lang.reflect.Proxy;
 import java.net.URLClassLoader;
 import java.net.URL;
 import java.net.MalformedURLException;
-import java.util.HashMap;
 
 /**
  * The stapler version of the {@link ClassLoader} object,
@@ -51,14 +50,7 @@ public static MetaClassLoader get(ClassLoader cl) {
         if(cl ==null)
             return null; // if no parent, delegate to the debug loader if available.
 
-        synchronized(classMap) {
-            MetaClassLoader mc = classMap.get(cl);
-            if(mc==null) {
-                mc = new MetaClassLoader(cl);
-                classMap.put(cl,mc);
-            }
-            return mc;
-        }
+        return classMap.get(cl);
     }
 
     /**
@@ -69,10 +61,13 @@ public static MetaClassLoader get(ClassLoader cl) {
 
     /**
      * All {@link MetaClass}es.
-     *
-     * Note that this permanently holds a strong reference to its key, i.e. is a memory leak.
      */
-    private static final Map<ClassLoader,MetaClassLoader> classMap = new HashMap<>();
+    private static final ClassLoaderValue<MetaClassLoader> classMap = new ClassLoaderValue<MetaClassLoader>() {
+        @Override
+        protected MetaClassLoader computeValue(ClassLoader cl) {
+            return new MetaClassLoader(cl);
+        }
+    };
 
     static {
         debugLoader = createDebugLoader();

core/src/main/java/org/kohsuke/stapler/TearOffSupport.java

@@ -26,8 +26,6 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 import java.lang.reflect.InvocationTargetException;
-import java.util.HashMap;
-import java.util.Map;
 
 /**
  * Allows "tear-off" objects to be linked to the parent object.
@@ -38,44 +36,49 @@
  *
  * @author Kohsuke Kawaguchi
  */
-@SuppressFBWarnings(value = "UG_SYNC_SET_UNSYNC_GET", justification = "Get is intentionally unsynchronized.")
 public abstract class TearOffSupport {
-    private volatile Map<Class,Object> tearOffs;
-
-    public final <T> T getTearOff(Class<T> t) {
-        Map<Class,Object> m = tearOffs;
-        if(m==null)     return null;
-        return (T)m.get(t);
-    }
-
-    public final <T> T loadTearOff(Class<T> t) {
-        T o = getTearOff(t);
-        if(o==null) {
+    private final ClassValue<Object> tearOffs = new ClassValue<Object>() {
+        @SuppressFBWarnings(value = "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", justification = "shut up")
+        @Override
+        protected Object computeValue(Class<?> type) {
             try {
-                o = t.getConstructor(getClass()).newInstance(this);
-                setTearOff(t,o);
+                return type.getConstructor(TearOffSupport.this.getClass()).newInstance(TearOffSupport.this);
             } catch (InstantiationException e) {
                 throw new InstantiationError(e.getMessage());
             } catch (IllegalAccessException e) {
                 throw new IllegalAccessError(e.getMessage());
             } catch (InvocationTargetException e) {
                 Throwable ex = e.getTargetException();
-                if(ex instanceof RuntimeException)
-                    throw (RuntimeException)ex;
-                if(ex instanceof Error)
-                    throw (Error)ex;
+                if (ex instanceof RuntimeException) {
+                    throw (RuntimeException) ex;
+                }
+                if (ex instanceof Error) {
+                    throw (Error) ex;
+                }
                 throw new Error(e);
             } catch (NoSuchMethodException e) {
                 throw new NoSuchMethodError(e.getMessage());
             }
         }
-        return o;
+    };
+
+    /**
+     * @deprecated Unused? Use {@link #loadTearOff}.
+     */
+    @Deprecated
+    public final <T> T getTearOff(Class<T> t) {
+        return loadTearOff(t);
+    }
+
+    public final <T> T loadTearOff(Class<T> t) {
+        return t.cast(tearOffs.get(t));
     }
 
-    public synchronized <T> void setTearOff(Class<T> type, T instance) {
-        Map<Class,Object> m = tearOffs;
-        Map<Class,Object> r = m!=null ? new HashMap<>(tearOffs) : new HashMap<>();
-        r.put(type,instance);
-        tearOffs = r;
+    /**
+     * @deprecated Unused?
+     */
+    @Deprecated
+    public <T> void setTearOff(Class<T> type, T instance) {
+        throw new UnsupportedOperationException();
     }
 }

core/src/main/java/org/kohsuke/stapler/WebApp.java

@@ -44,6 +44,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.CopyOnWriteArrayList;
+import org.kohsuke.stapler.lang.KlassNavigator;
 
 /**
  * Object scoped to the entire webapp. Mostly used for configuring behavior of Stapler.
@@ -80,8 +81,9 @@ public static WebApp get(ServletContext context) {
     public final ServletContext context;
 
     /**
-     * Duck-type wrappers for the given class.
+     * @deprecated Unused?
      */
+    @Deprecated
     public final Map<Class,Class[]> wrappers = new HashMap<>();
 
     /**
@@ -117,10 +119,8 @@ public static WebApp get(ServletContext context) {
 
     /**
      * All {@link MetaClass}es.
-     *
-     * Note that this permanently holds a strong reference to its key, i.e. is a memory leak.
      */
-    private final Map<Klass<?>,MetaClass> classMap = new HashMap<>();
+    private volatile ClassValue<MetaClass> classMap;
 
     /**
      * Handles objects that are exported.
@@ -224,13 +224,23 @@ public MetaClass getMetaClass(Class c) {
 
     public MetaClass getMetaClass(Klass<?> c) {
         if(c==null)     return null;
-        synchronized(classMap) {
-            MetaClass mc = classMap.get(c);
-            if(mc==null) {
-                mc = new MetaClass(this,c);
-                classMap.put(c,mc);
+        if (c.navigator == KlassNavigator.JAVA) {
+            ClassValue<MetaClass> _classMap;
+            synchronized (this) {
+                if (classMap == null) {
+                    classMap = new ClassValue<MetaClass>() {
+                        @Override
+                        protected MetaClass computeValue(Class<?> c) {
+                            return new MetaClass(WebApp.this, Klass.java(c));
+                        }
+                    };
+                }
+                _classMap = classMap;
             }
-            return mc;
+            return _classMap.get(c.toJavaClass());
+        } else {
+            // probably now impossible outside tests
+            return new MetaClass(this,c);
         }
     }
 
@@ -261,22 +271,14 @@ public Klass<?> getKlass(Object o) {
     }
 
     /**
-     * Convenience maintenance method to clear all the cached scripts for the given tearoff type.
-     *
-     * <p>
-     * This is useful when you want to have the scripts reloaded into the live system without
-     * the performance penalty of {@link MetaClass#NO_CACHE}.
-     *
-     * @see MetaClass#NO_CACHE
+     * @deprecated Unused?
      */
+    @Deprecated
     public void clearScripts(Class<? extends AbstractTearOff> clazz) {
-        synchronized (classMap) {
-            for (MetaClass v : classMap.values()) {
-                AbstractTearOff t = v.getTearOff(clazz);
-                if (t!=null)
-                    t.clearScripts();
-            }
-        }
+        // ClassValue cannot enumerate entries.
+        // If really needed, could be implemented by keeping a WeakHashMap<Class, Boolean>
+        // of classes added to classMap by getMetaClass that we could enumerate.
+        clearMetaClassCache();
     }
 
     /**
@@ -286,10 +288,9 @@ public void clearScripts(Class<? extends AbstractTearOff> clazz) {
      * Take care that the generation of MetaClass information takes a bit of time and so 
      * this call should not be called too often
      */
-    public void clearMetaClassCache(){
-        synchronized (classMap) {
-            classMap.clear();
-        }
+    public synchronized void clearMetaClassCache(){
+        // No ClassValue.clear() method, so need to just null it out instead.
+        classMap = null;
     }
 
     void addStaplerServlet(String servletName, Stapler servlet) {

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

@@ -40,6 +40,7 @@ public class ModelBuilder {
      * Registration happens in {@link Model#Model(ModelBuilder, Class, Class, String)} so that cyclic references
      * are handled correctly.
      */
+    // TODO unclear how to convert to ClassValue given registration from parent
     /*package*/ final Map<Class, Model> models = new ConcurrentHashMap<>();
 
     @NonNull

core/src/main/java/org/kohsuke/stapler/lang/Klass.java

@@ -16,7 +16,8 @@
  * To support other JVM languages that use their own specific types to represent a class
  * (such as JRuby and Jython), we now use this object instead of {@link Class}. This allows
  * us to reuse much of the logic of class traversal/resource lookup across different languages.
- * 
+ * <em>But</em> after the removal of JRuby support, in practice this is used only for {@link Class}.
+ * <p>
  * This is a convenient tuple so that we can pass around a single argument instead of two.
  *
  * @param <C>

core/src/main/java/org/kohsuke/stapler/lang/KlassNavigator.java

@@ -17,7 +17,8 @@
 
 /**
  * Strategy pattern to provide navigation across class-like objects in other languages of JVM.
- *
+ * <p>
+ * After removal of JRuby support, {@link #JAVA} is the only implementation.
  * <p>
  * Implementations should be stateless and typically a singleton.
  *