diff --git a/src/main/java/hudson/remoting/RemoteClassLoader.java b/src/main/java/hudson/remoting/RemoteClassLoader.java index ff6ab072b..94464aa4b 100644 --- a/src/main/java/hudson/remoting/RemoteClassLoader.java +++ b/src/main/java/hudson/remoting/RemoteClassLoader.java @@ -168,48 +168,81 @@ then the class file image is wasted.) cr = prefetchedClasses.remove(name); if (cr == null) { LOGGER.log(Level.FINER, "fetch3({0})", name); - Map all = proxy.fetch3(name); - synchronized (prefetchedClasses) { - /** - * Converts {@link ClassFile2} to {@link ClassReference} with minimal - * proxy creation. This creates a reference to {@link ClassLoader}, so - * it shoudn't be kept beyond the scope of single {@link #findClass(String)} call. - */ - class ClassReferenceBuilder { - private final Map classLoaders = new HashMap(); - - ClassReference toRef(ClassFile2 cf) { - int n = cf.classLoader; - - ClassLoader cl = classLoaders.get(n); - if (cl==null) - classLoaders.put(n,cl = channel.importedClassLoaders.get(n)); - - return new ClassReference(cl,cf.image); - } - } - ClassReferenceBuilder crf = new ClassReferenceBuilder(); - - for (Map.Entry entry : all.entrySet()) { - String cn = entry.getKey(); - ClassFile2 cf = entry.getValue(); - ClassReference ref = crf.toRef(cf); - - if (cn.equals(name)) { - cr = ref; - } else { - // where we remember the prefetch is sensitive to who references it, - // because classes need not be transitively visible in Java - if (cf.referer!=null) - ref.rememberIn(cn, crf.toRef(cf.referer).classLoader); - else - ref.rememberIn(cn, this); - - LOGGER.log(Level.FINER, "prefetch {0} -> {1}", new Object[]{name, cn}); + + boolean interrupted = false; + try { + // 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. + + while (true) { + try { + if (TESTING_CLASS_REFERENCE_LOAD != null) { + TESTING_CLASS_REFERENCE_LOAD.run(); + } + + Map all = proxy.fetch3(name); + synchronized (prefetchedClasses) { + /** + * Converts {@link ClassFile2} to {@link ClassReference} with minimal + * proxy creation. This creates a reference to {@link ClassLoader}, so + * it shoudn't be kept beyond the scope of single {@link #findClass(String)} call. + */ + class ClassReferenceBuilder { + private final Map classLoaders = new HashMap(); + + ClassReference toRef(ClassFile2 cf) { + int n = cf.classLoader; + + ClassLoader cl = classLoaders.get(n); + if (cl==null) + classLoaders.put(n,cl = channel.importedClassLoaders.get(n)); + + return new ClassReference(cl,cf.image); + } + } + ClassReferenceBuilder crf = new ClassReferenceBuilder(); + + for (Map.Entry entry : all.entrySet()) { + String cn = entry.getKey(); + ClassFile2 cf = entry.getValue(); + ClassReference ref = crf.toRef(cf); + + if (cn.equals(name)) { + cr = ref; + } else { + // where we remember the prefetch is sensitive to who references it, + // because classes need not be transitively visible in Java + if (cf.referer!=null) + ref.rememberIn(cn, crf.toRef(cf.referer).classLoader); + else + ref.rememberIn(cn, this); + + LOGGER.log(Level.FINER, "prefetch {0} -> {1}", new Object[]{name, cn}); + } + + ref.rememberIn(cn, ref.classLoader); + } + } + break; + } catch (RemotingSystemException x) { + if (x.getCause() instanceof InterruptedException) { + // 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 + } + throw x; } - ref.rememberIn(cn, ref.classLoader); + // no code is allowed to reach here } + } finally { + // process the interrupt later. + if (interrupted) + Thread.currentThread().interrupt(); } assert cr != null; @@ -239,9 +272,8 @@ ClassReference toRef(ClassFile2 cf) { while (true) { try { - if (TESTING) { - Thread.sleep(1000); - } + if (TESTING_CLASS_LOAD != null) TESTING_CLASS_LOAD.run(); + if (c!=null) return c; // TODO: check inner class handling @@ -312,11 +344,12 @@ private static Object _getClassLoadingLock(RemoteClassLoader rcl, String name) { return rcl; } /** - * Induce a large delay in {@link RemoteClassLoader#findClass(String)} to allow + * Intercept {@link RemoteClassLoader#findClass(String)} to allow unittests to be written. * - * See JENKINS-6604 + * See JENKINS-6604 and similar issues */ - static boolean TESTING; + static Runnable TESTING_CLASS_LOAD; + static Runnable TESTING_CLASS_REFERENCE_LOAD; private Class loadClassFile(String name, byte[] bytes) { if (bytes.length < 8) { diff --git a/src/test/java/hudson/remoting/ClassRemotingTest.java b/src/test/java/hudson/remoting/ClassRemotingTest.java index 3da00f463..447bf02c3 100644 --- a/src/test/java/hudson/remoting/ClassRemotingTest.java +++ b/src/test/java/hudson/remoting/ClassRemotingTest.java @@ -29,9 +29,10 @@ import org.objectweb.asm.commons.EmptyVisitor; import java.io.IOException; -import java.util.concurrent.CancellationException; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; /** * Test class image forwarding. @@ -94,7 +95,7 @@ public void testRaceCondition() throws Throwable { assertEquals(child2, c2.getClass().getClassLoader()); assertEquals(parent, c2.getClass().getSuperclass().getClassLoader()); ExecutorService svc = Executors.newFixedThreadPool(2); - RemoteClassLoader.TESTING = true; + RemoteClassLoader.TESTING_CLASS_LOAD = new SleepForASec(); try { java.util.concurrent.Future f1 = svc.submit(new java.util.concurrent.Callable() { public Object call() throws Exception { @@ -109,49 +110,91 @@ public Object call() throws Exception { f1.get(); f2.get(); } finally { - RemoteClassLoader.TESTING = false; + RemoteClassLoader.TESTING_CLASS_LOAD = null; + } + } + + private static final class SleepForASec implements Runnable { + @Override + public void run() { + try { + Thread.sleep(1000); + } catch (InterruptedException e) { + // Nothing + } } } @Bug(19453) - public void testInterruption() throws Exception { + public void testInterruptionOfClassCreation() throws Exception { DummyClassLoader parent = new DummyClassLoader(TestLinkage.B.class); final DummyClassLoader child1 = new DummyClassLoader(parent, TestLinkage.A.class); final DummyClassLoader child2 = new DummyClassLoader(child1, TestLinkage.class); final Callable c = (Callable) child2.load(TestLinkage.class); assertEquals(child2, c.getClass().getClassLoader()); - RemoteClassLoader.TESTING = true; + RemoteClassLoader.TESTING_CLASS_LOAD = new InterruptThirdInvocation(); try { - ExecutorService svc = Executors.newSingleThreadExecutor(); - java.util.concurrent.Future f1 = svc.submit(new java.util.concurrent.Callable() { - public Object call() throws Exception { - try { - return channel.call(c); - } catch (Throwable t) { - throw new Exception(t); - } - } - }); + java.util.concurrent.Future f1 = scheduleCallableLoad(c); + + try { + f1.get(); + } catch (ExecutionException ex) { + // Expected + } + + // verify that classes that we tried to load aren't irrevocably damaged and it's still available + assertEquals(String.class, channel.call(c).getClass()); + } finally { + RemoteClassLoader.TESTING_CLASS_LOAD = null; + } + } + + @Bug(36991) + public void testInterruptionOfClassReferenceCreation() throws Exception { + DummyClassLoader parent = new DummyClassLoader(TestLinkage.B.class); + final DummyClassLoader child1 = new DummyClassLoader(parent, TestLinkage.A.class); + final DummyClassLoader child2 = new DummyClassLoader(child1, TestLinkage.class); + final Callable c = (Callable) child2.load(TestLinkage.class); + assertEquals(child2, c.getClass().getClassLoader()); + RemoteClassLoader.TESTING_CLASS_REFERENCE_LOAD = new InterruptThirdInvocation(); - // 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); + try { + Future f1 = scheduleCallableLoad(c); - f1.cancel(true); try { - Object o = f1.get(); - assertEquals(String.class, o.getClass()); - /* TODO we really want to fail here, but this method gets run 4×, and the last 3× it gets to this point: - fail(o.toString()); - */ - } catch (CancellationException x) { - // good + f1.get(); + } catch (ExecutionException ex) { + // Expected } // verify that classes that we tried to load aren't irrevocably damaged and it's still available - assertNotNull(channel.call(c)); + assertEquals(String.class, channel.call(c).getClass()); } finally { - RemoteClassLoader.TESTING = false; + RemoteClassLoader.TESTING_CLASS_REFERENCE_LOAD = null; + } + } + + private Future scheduleCallableLoad(final Callable c) { + ExecutorService svc = Executors.newSingleThreadExecutor(); + return svc.submit(new java.util.concurrent.Callable() { + public Object call() throws Exception { + try { + return channel.call(c); + } catch (Throwable t) { + throw new Exception(t); + } + } + }); + } + + private static class InterruptThirdInvocation implements Runnable { + private int invocationCount = 0; + @Override + public void run() { + invocationCount++; + if (invocationCount == 3) { + Thread.currentThread().interrupt(); + } } }