Skip to content
Permalink
Browse files

[JENKINS-19453] Propose fix: mask interrupt while loading a class

I think the crux of the issue is that, in general, a failed class loading
will never get retried. It is certainly the case when another class loading has triggered a
recurisve class loading as in this test case.

In #19 Jesse proposed to fix
this by dropping the whole RemoteClassLoader. But this will not work
since the said RemoteClassLoader cloud have loaded other classes that
might be already running. Dropping a classloader will not drop these
classes, and we end up just creating another classloader that loads
incompatible classes. That is a disaster waiting to happen.

In this fix, we simply make findClass non-interruptible. That is, if a
blocking remote operation gets interrupted, we catch that and simply
retry and refuse to give up. We'll remember to set the interrupt flag
back on, however, so that the interrupt signal doesn't disappear.

The net effect is as if the delivery of the interrupt was bit late than
usual. The way I see it, this is the only way to fix this.

The nesting of control structures here is horrible, but I can't think of
any better way to write this.
  • Loading branch information...
kohsuke committed Nov 9, 2013
1 parent c49d842 commit 4efb4e1dd5675a5d22993b09ebb65adab5baa9e0
@@ -214,29 +214,45 @@ ClassReference toRef(ClassFile2 cf) {
RemoteClassLoader rcl = (RemoteClassLoader) cl;
synchronized (_getClassLoadingLock(rcl, name)) {
Class<?> c = rcl.findLoadedClass(name);
try {
if (TESTING) {
Thread.sleep(1000);
}
if (c!=null) return c;

// TODO: check inner class handling
Future<byte[]> img = cr.classImage.resolve(channel, name.replace('.', '/') + ".class");
if (img.isDone()) {
boolean interrupted = false;
try {//
while (true) {
try {
return rcl.loadClassFile(name, img.get());
} catch (ExecutionException x) {
// failure to retrieve a jar shouldn't fail the classloading
if (TESTING) {
Thread.sleep(1000);
}
if (c!=null) return c;

// TODO: check inner class handling
Future<byte[]> img = cr.classImage.resolve(channel, name.replace('.', '/') + ".class");
if (img.isDone()) {
try {
return rcl.loadClassFile(name, img.get());
} catch (ExecutionException x) {
// failure to retrieve a jar shouldn't fail the classloading
}
}

// if the load activity is still pending, or if the load had failed,
// fetch just this class file
return rcl.loadClassFile(name, proxy.fetch(name));
} catch (IOException x) {
throw new ClassNotFoundException(name,x);
} catch (InterruptedException x) {
// pretend as if this operation is not interruptible.
// but we need to remember to set the interrupt flag back on
// before we leave this call.
interrupted = true;
continue; // JENKINS-19453: retry
}
}

// if the load activitiy is still pending, or if the load had failed,
// fetch just this class file
return rcl.loadClassFile(name, proxy.fetch(name));
} catch (IOException x) {
throw new ClassNotFoundException(name,x);
} catch (InterruptedException x) {
throw new ClassNotFoundException(name,x);
// no code is allowed to reach here
}
} finally {
// process the interrupt later.
if (interrupted)
Thread.currentThread().interrupt();
}
}
} else {
@@ -275,7 +291,11 @@ private static Object _getClassLoadingLock(RemoteClassLoader rcl, String name) {
}
return rcl;
}
/** JENKINS-6604 */
/**
* Induce a large delay in {@link RemoteClassLoader#findClass(String)} to allow
*
* See JENKINS-6604
*/
static boolean TESTING;

private Class<?> loadClassFile(String name, byte[] bytes) {
@@ -134,7 +134,11 @@ public void testInterruption() throws Exception {
}
}
});

// This is so that the first two class loads succeed but the third fails.
// A better test would use semaphores rather than timing (cf. the test before this one).
Thread.sleep(2500);

f1.cancel(true);
try {
Object o = f1.get();
@@ -145,6 +149,8 @@ public void testInterruption() throws Exception {
} catch (CancellationException x) {
// good
}

// verify that classes that we tried to load aren't irrevocably damaged and it's still available
assertNotNull(channel.call(c));
} finally {
RemoteClassLoader.TESTING = false;
@@ -37,6 +37,10 @@
* out of nowhere, to test {@link RemoteClassLoader} by creating a class
* that only exists on one side of the channel but not the other.
*
* <p>
* Given a class in a "remoting" package, this classloader is capable of loading the same version of the class
* in the "rem0ting" package.
*
* @author Kohsuke Kawaguchi
*/
class DummyClassLoader extends ClassLoader {
@@ -27,6 +27,9 @@
public class TestLinkage implements Callable {

public Object call() throws Throwable {
// force classloading of several classes
// when we run this test, we insert an artificial delay to each classloading
// so that we can intercept the classloading.
return A.field;
}

0 comments on commit 4efb4e1

Please sign in to comment.
You can’t perform that action at this time.