[FIXED JENKINS-12124] Occasionally errors loading plugin classes since it is...

[FIXED JENKINS-12124] Occasionally errors loading plugin classes since it is expected that findClass (and findLoadedClass) are called under synchronization.
The problem was masked by a blind assumption that an InvocationTargetException was in fact wrapping a ClassNotFoundException.
Many thanks to @gotwarlost for demonstrating how to reproduce the problem and diagnosing the cause.

changelog.html

@@ -59,6 +59,9 @@ Upcoming changes</a>
     Build history widget only showed the last day of builds.
     (Due to JENKINS-20892, even with this fix at most 20 builds are shown.)
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-21159">issue 21159</a>)
+  <li class=bug>
+    Random class loading error mostly known to affect static analysis plugins.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-12124">issue 12124</a>)
   <li class=rfe>
     Slave started from Java Web Start can now install itself as a systemd service.
   <li class=rfe>

core/src/main/java/hudson/ClassicPluginStrategy.java

@@ -56,7 +56,6 @@ import java.io.FileInputStream;
 import java.io.FilenameFilter;
 import java.io.IOException;
 import java.lang.reflect.Field;
-import java.lang.reflect.InvocationTargetException;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -72,7 +71,6 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 public class ClassicPluginStrategy implements PluginStrategy {
-    private final ClassLoaderReflectionToolkit clt = new ClassLoaderReflectionToolkit();
 
     /**
      * Filter for jar files.
@@ -569,10 +567,10 @@ public class ClassicPluginStrategy implements PluginStrategy {
             if (PluginManager.FAST_LOOKUP) {
                 for (PluginWrapper pw : getTransitiveDependencies()) {
                     try {
-                        Class c = clt.findLoadedClass(pw.classLoader,name);
+                        Class<?> c = ClassLoaderReflectionToolkit._findLoadedClass(pw.classLoader, name);
                         if (c!=null)    return c;
-                        return clt.findClass(pw.classLoader,name);
-                    } catch (InvocationTargetException e) {
+                        return ClassLoaderReflectionToolkit._findClass(pw.classLoader, name);
+                    } catch (ClassNotFoundException e) {
                         //not found. try next
                     }
                 }
@@ -596,15 +594,11 @@ public class ClassicPluginStrategy implements PluginStrategy {
             HashSet<URL> result = new HashSet<URL>();
 
             if (PluginManager.FAST_LOOKUP) {
-                try {
                     for (PluginWrapper pw : getTransitiveDependencies()) {
-                        Enumeration<URL> urls = clt.findResources(pw.classLoader, name);
+                        Enumeration<URL> urls = ClassLoaderReflectionToolkit._findResources(pw.classLoader, name);
                         while (urls != null && urls.hasMoreElements())
                             result.add(urls.nextElement());
                     }
-                } catch (InvocationTargetException e) {
-                    throw new Error(e);
-                }
             } else {
                 for (Dependency dep : dependencies) {
                     PluginWrapper p = pluginManager.getPlugin(dep.shortName);
@@ -622,14 +616,10 @@ public class ClassicPluginStrategy implements PluginStrategy {
         @Override
         protected URL findResource(String name) {
             if (PluginManager.FAST_LOOKUP) {
-                try {
                     for (PluginWrapper pw : getTransitiveDependencies()) {
-                        URL url = clt.findResource(pw.classLoader,name);
+                        URL url = ClassLoaderReflectionToolkit._findResource(pw.classLoader, name);
                         if (url!=null)    return url;
                     }
-                } catch (InvocationTargetException e) {
-                    throw new Error(e);
-                }
             } else {
                 for (Dependency dep : dependencies) {
                     PluginWrapper p = pluginManager.getPlugin(dep.shortName);

core/src/main/java/hudson/PluginManager.java

@@ -82,7 +82,6 @@ import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.ref.WeakReference;
-import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.URL;
 import java.util.ArrayList;
@@ -941,8 +940,6 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas
          */
         private ConcurrentMap<String, WeakReference<Class>> generatedClasses = new ConcurrentHashMap<String, WeakReference<Class>>();
 
-        private ClassLoaderReflectionToolkit clt = new ClassLoaderReflectionToolkit();
-
         public UberClassLoader() {
             super(PluginManager.class.getClassLoader());
         }
@@ -963,11 +960,11 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas
             if (FAST_LOOKUP) {
                 for (PluginWrapper p : activePlugins) {
                     try {
-                        Class c = clt.findLoadedClass(p.classLoader,name);
+                        Class<?> c = ClassLoaderReflectionToolkit._findLoadedClass(p.classLoader, name);
                         if (c!=null)    return c;
                         // calling findClass twice appears to cause LinkageError: duplicate class def
-                        return clt.findClass(p.classLoader,name);
-                    } catch (InvocationTargetException e) {
+                        return ClassLoaderReflectionToolkit._findClass(p.classLoader, name);
+                    } catch (ClassNotFoundException e) {
                         //not found. try next
                     }
                 }
@@ -987,15 +984,11 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas
         @Override
         protected URL findResource(String name) {
             if (FAST_LOOKUP) {
-                try {
                     for (PluginWrapper p : activePlugins) {
-                        URL url = clt.findResource(p.classLoader,name);
+                        URL url = ClassLoaderReflectionToolkit._findResource(p.classLoader, name);
                         if(url!=null)
                             return url;
                     }
-                } catch (InvocationTargetException e) {
-                    throw new Error(e);
-                }
             } else {
                 for (PluginWrapper p : activePlugins) {
                     URL url = p.classLoader.getResource(name);
@@ -1010,13 +1003,9 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas
         protected Enumeration<URL> findResources(String name) throws IOException {
             List<URL> resources = new ArrayList<URL>();
             if (FAST_LOOKUP) {
-                try {
                     for (PluginWrapper p : activePlugins) {
-                        resources.addAll(Collections.list(clt.findResources(p.classLoader, name)));
+                        resources.addAll(Collections.list(ClassLoaderReflectionToolkit._findResources(p.classLoader, name)));
                     }
-                } catch (InvocationTargetException e) {
-                    throw new Error(e);
-                }
             } else {
                 for (PluginWrapper p : activePlugins) {
                     resources.addAll(Collections.list(p.classLoader.getResources(name)));

core/src/main/java/jenkins/ClassLoaderReflectionToolkit.java

 package jenkins;
 
+import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.URL;
 import java.util.Enumeration;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 /**
- * Reflection access to various {@link ClassLoader} methods.
- *
- * @author Kohsuke Kawaguchi
+ * Reflective access to various {@link ClassLoader} methods which are otherwise {@code protected}.
  */
+@SuppressWarnings({"unchecked", "rawtypes"})
 public class ClassLoaderReflectionToolkit {
-    /**
-     * ClassLoader.findClass(String) for a call that bypasses access modifier.
-     */
-    private final Method FIND_CLASS, FIND_LOADED_CLASS, FIND_RESOURCE, FIND_RESOURCES;
 
-    public ClassLoaderReflectionToolkit() {
+    private static final Method FIND_CLASS, FIND_LOADED_CLASS, FIND_RESOURCE, FIND_RESOURCES, GET_CLASS_LOADING_LOCK;
+
+    static {
         try {
             FIND_CLASS = ClassLoader.class.getDeclaredMethod("findClass",String.class);
             FIND_CLASS.setAccessible(true);
@@ -29,8 +29,85 @@ public class ClassLoaderReflectionToolkit {
         } catch (NoSuchMethodException e) {
             throw new AssertionError(e);
         }
+        Method gCLL = null;
+        try {
+            gCLL = ClassLoader.class.getDeclaredMethod("getClassLoadingLock", String.class);
+            gCLL.setAccessible(true);
+        } catch (NoSuchMethodException x) {
+            // OK, Java 6
+        }
+        GET_CLASS_LOADING_LOCK = gCLL;
+    }
+
+    private static <T extends Exception> Object invoke(Method method, Class<T> exception, Object obj, Object... args) throws T {
+        try {
+            return method.invoke(obj, args);
+        } catch (IllegalAccessException x) {
+            throw new AssertionError(x);
+        } catch (InvocationTargetException x) {
+            Throwable x2 = x.getCause();
+            if (x2 instanceof RuntimeException) {
+                throw (RuntimeException) x2;
+            } else if (x2 instanceof Error) {
+                throw (Error) x2;
+            } else if (exception.isInstance(x2)) {
+                throw exception.cast(x2);
+            } else {
+                throw new AssertionError(x2);
+            }
+        }
+    }
+
+    private static Object getClassLoadingLock(ClassLoader cl, String name) {
+        if (GET_CLASS_LOADING_LOCK != null) {
+            return invoke(GET_CLASS_LOADING_LOCK, RuntimeException.class, cl, name);
+        } else {
+            // Java 6 expects you to always synchronize on this.
+            return cl;
+        }
     }
 
+    /**
+     * Calls {@link ClassLoader#findLoadedClass} while holding {@link ClassLoader#getClassLoadingLock}.
+     * @since 1.553
+     */
+    public static @CheckForNull Class<?> _findLoadedClass(ClassLoader cl, String name) {
+        synchronized (getClassLoadingLock(cl, name)) {
+            return (Class) invoke(FIND_LOADED_CLASS, RuntimeException.class, cl, name);
+        }
+    }
+
+    /**
+     * Calls {@link ClassLoader#findClass} while holding {@link ClassLoader#getClassLoadingLock}.
+     * @since 1.553
+     */
+    public static @Nonnull Class<?> _findClass(ClassLoader cl, String name) throws ClassNotFoundException {
+        synchronized (getClassLoadingLock(cl, name)) {
+            return (Class) invoke(FIND_CLASS, ClassNotFoundException.class, cl, name);
+        }
+    }
+
+    /**
+     * Calls {@link ClassLoader#findResource}.
+     * @since 1.553
+     */
+    public static @CheckForNull URL _findResource(ClassLoader cl, String name) {
+        return (URL) invoke(FIND_RESOURCE, RuntimeException.class, cl, name);
+    }
+
+    /**
+     * Calls {@link ClassLoader#findResources}.
+     * @since 1.553
+     */
+    public static @Nonnull Enumeration<URL> _findResources(ClassLoader cl, String name) throws IOException {
+        return (Enumeration<URL>) invoke(FIND_RESOURCES, IOException.class, cl, name);
+    }
+
+    /** @deprecated unsafe */
+    @Deprecated public ClassLoaderReflectionToolkit() {}
+
+    /** @deprecated unsafe */
+    @Deprecated
     public Class findLoadedClass(ClassLoader cl, String name) throws InvocationTargetException {
         try {
             return (Class)FIND_LOADED_CLASS.invoke(cl,name);
@@ -39,6 +116,8 @@ public class ClassLoaderReflectionToolkit {
         }
     }
 
+    /** @deprecated unsafe */
+    @Deprecated
     public Class findClass(ClassLoader cl, String name) throws InvocationTargetException {
         try {
             return (Class)FIND_CLASS.invoke(cl,name);
@@ -47,6 +126,8 @@ public class ClassLoaderReflectionToolkit {
         }
     }
 
+    /** @deprecated unsafe */
+    @Deprecated
     public URL findResource(ClassLoader cl, String name) throws InvocationTargetException {
         try {
             return (URL)FIND_RESOURCE.invoke(cl,name);
@@ -55,6 +136,8 @@ public class ClassLoaderReflectionToolkit {
         }
     }
 
+    /** @deprecated unsafe */
+    @Deprecated
     public Enumeration<URL> findResources(ClassLoader cl, String name) throws InvocationTargetException {
         try {
             return (Enumeration<URL>)FIND_RESOURCES.invoke(cl,name);
@@ -63,11 +146,4 @@ public class ClassLoaderReflectionToolkit {
         }
     }
 
-//    private void check(InvocationTargetException e) {
-//        Throwable t = e.getTargetException();
-//        if (t instanceof Error)
-//            throw (Error)t;
-//        if (t instanceof RuntimeException)
-//            throw (RuntimeException)t;
-//    }
 }