[FIXED 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.

src/main/java/hudson/remoting/RemoteClassLoader.java

@@ -214,29 +214,50 @@ ClassReference toRef(ClassFile2 cf) {
                     RemoteClassLoader rcl = (RemoteClassLoader) cl;
                     synchronized (_getClassLoadingLock(rcl, name)) {
                         Class<?> c = rcl.findLoadedClass(name);
+
+                        boolean interrupted = false;
                         try {
-                            if (TESTING) {
-                                Thread.sleep(1000);
-                            }
-                            if (c!=null)    return c;
+                            // the code in this try block may throw InterruptException, but findClass
+                            // method is supposed to be uninterruptible. So we catch interrupt exception
+                            // and just retry until it succeeds, but in the end we set the interrupt flag
+                            // back on to let the interrupt in the next earliest occasion.
 
-                            // TODO: check inner class handling
-                            Future<byte[]> img = cr.classImage.resolve(channel, name.replace('.', '/') + ".class");
-                            if (img.isDone()) {
+                            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 +296,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) {

src/test/java/hudson/remoting/ClassRemotingTest.java

@@ -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;

src/test/java/hudson/remoting/DummyClassLoader.java

@@ -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 {

src/test/java/hudson/remoting/TestLinkage.java

@@ -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;
     }