Skip to content

Commit

Permalink
[FIXED JENKINS-12124] Occasionally errors loading plugin classes sinc…
Browse files Browse the repository at this point in the history 
…e 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.
  • Loading branch information
@jglick
jglick committed Feb 21, 2014
1 parent 8482756 commit 898f1f7
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 46 deletions.
3 changes: 3 additions & 0 deletions changelog.html
View file
Expand Up @@ -59,6 +59,9 @@
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>
Expand Down
20 changes: 5 additions & 15 deletions core/src/main/java/hudson/ClassicPluginStrategy.java
View file
Expand Up @@ -56,7 +56,6 @@
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;
Expand All @@ -72,7 +71,6 @@
import java.util.logging.Logger;

public class ClassicPluginStrategy implements PluginStrategy {
private final ClassLoaderReflectionToolkit clt = new ClassLoaderReflectionToolkit();

/**
* Filter for jar files.
Expand Down Expand Up @@ -569,10 +567,10 @@ protected Class<?> findClass(String name) throws ClassNotFoundException {
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
}
}
Expand All @@ -596,15 +594,11 @@ protected Enumeration<URL> findResources(String name) throws IOException {
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);
Expand All @@ -622,14 +616,10 @@ protected Enumeration<URL> findResources(String name) throws IOException {
@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);
Expand Down
21 changes: 5 additions & 16 deletions core/src/main/java/hudson/PluginManager.java
View file
Expand Up @@ -82,7 +82,6 @@
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;
Expand Down Expand Up @@ -941,8 +940,6 @@ public final class UberClassLoader extends ClassLoader {
*/
private ConcurrentMap<String, WeakReference<Class>> generatedClasses = new ConcurrentHashMap<String, WeakReference<Class>>();

private ClassLoaderReflectionToolkit clt = new ClassLoaderReflectionToolkit();

public UberClassLoader() {
super(PluginManager.class.getClassLoader());
}
Expand All @@ -963,11 +960,11 @@ protected Class<?> findClass(String name) throws ClassNotFoundException {
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
}
}
Expand All @@ -987,15 +984,11 @@ protected Class<?> findClass(String name) throws ClassNotFoundException {
@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);
Expand All @@ -1010,13 +1003,9 @@ protected URL findResource(String name) {
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)));
Expand Down
106 changes: 91 additions & 15 deletions core/src/main/java/jenkins/ClassLoaderReflectionToolkit.java
View file
@@ -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);
Expand All @@ -29,8 +29,85 @@ public 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);
Expand All @@ -39,6 +116,8 @@ public Class findLoadedClass(ClassLoader cl, String name) throws InvocationTarge
}
}

/** @deprecated unsafe */
@Deprecated
public Class findClass(ClassLoader cl, String name) throws InvocationTargetException {
try {
return (Class)FIND_CLASS.invoke(cl,name);
Expand All @@ -47,6 +126,8 @@ public Class findClass(ClassLoader cl, String name) throws InvocationTargetExcep
}
}

/** @deprecated unsafe */
@Deprecated
public URL findResource(ClassLoader cl, String name) throws InvocationTargetException {
try {
return (URL)FIND_RESOURCE.invoke(cl,name);
Expand All @@ -55,6 +136,8 @@ public URL findResource(ClassLoader cl, String name) throws InvocationTargetExce
}
}

/** @deprecated unsafe */
@Deprecated
public Enumeration<URL> findResources(ClassLoader cl, String name) throws InvocationTargetException {
try {
return (Enumeration<URL>)FIND_RESOURCES.invoke(cl,name);
Expand All @@ -63,11 +146,4 @@ public Enumeration<URL> findResources(ClassLoader cl, String name) throws Invoca
}
}

// private void check(InvocationTargetException e) {
// Throwable t = e.getTargetException();
// if (t instanceof Error)
// throw (Error)t;
// if (t instanceof RuntimeException)
// throw (RuntimeException)t;
// }
}

0 comments on commit 898f1f7

Please sign in to comment.