From 898f1f76a37e1c69cf38df718a5d3899544ebb44 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 21 Feb 2014 14:55:18 -0500 Subject: [PATCH] [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 | 3 + .../java/hudson/ClassicPluginStrategy.java | 20 +--- core/src/main/java/hudson/PluginManager.java | 21 +--- .../jenkins/ClassLoaderReflectionToolkit.java | 106 +++++++++++++++--- 4 files changed, 104 insertions(+), 46 deletions(-) diff --git a/changelog.html b/changelog.html index bca6e585fe..5698e2a07a 100644 --- a/changelog.html +++ b/changelog.html @@ -59,6 +59,9 @@ Upcoming changes Build history widget only showed the last day of builds. (Due to JENKINS-20892, even with this fix at most 20 builds are shown.) (issue 21159) +
  • + Random class loading error mostly known to affect static analysis plugins. + (issue 12124)
  • Slave started from Java Web Start can now install itself as a systemd service.
  • diff --git a/core/src/main/java/hudson/ClassicPluginStrategy.java b/core/src/main/java/hudson/ClassicPluginStrategy.java index 9aac065495..8279fb8ca1 100644 --- a/core/src/main/java/hudson/ClassicPluginStrategy.java +++ b/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 result = new HashSet(); if (PluginManager.FAST_LOOKUP) { - try { for (PluginWrapper pw : getTransitiveDependencies()) { - Enumeration urls = clt.findResources(pw.classLoader, name); + Enumeration 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); diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java index dc657a9a0b..ace1d44d41 100644 --- a/core/src/main/java/hudson/PluginManager.java +++ b/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> generatedClasses = new ConcurrentHashMap>(); - 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 findResources(String name) throws IOException { List resources = new ArrayList(); 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))); diff --git a/core/src/main/java/jenkins/ClassLoaderReflectionToolkit.java b/core/src/main/java/jenkins/ClassLoaderReflectionToolkit.java index 8461a98e9e..49b2b76908 100644 --- a/core/src/main/java/jenkins/ClassLoaderReflectionToolkit.java +++ b/core/src/main/java/jenkins/ClassLoaderReflectionToolkit.java @@ -1,22 +1,22 @@ 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 Object invoke(Method method, Class 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 _findResources(ClassLoader cl, String name) throws IOException { + return (Enumeration) 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 findResources(ClassLoader cl, String name) throws InvocationTargetException { try { return (Enumeration)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; -// } } -- GitLab