diff --git a/src/main/java/hudson/remoting/RemoteClassLoader.java b/src/main/java/hudson/remoting/RemoteClassLoader.java
index 4cd91a673..9a5d09f05 100644
--- a/src/main/java/hudson/remoting/RemoteClassLoader.java
+++ b/src/main/java/hudson/remoting/RemoteClassLoader.java
@@ -32,6 +32,7 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
+import java.io.InterruptedIOException;
import java.io.ObjectStreamException;
import java.io.Serializable;
import java.net.MalformedURLException;
@@ -73,6 +74,44 @@ final class RemoteClassLoader extends URLClassLoader {
private static final Logger LOGGER = Logger.getLogger(RemoteClassLoader.class.getName());
+ interface Interruptible {
+ void run() throws InterruptedException;
+ }
+
+ /**
+ * Intercept {@link RemoteClassLoader#loadRemoteClass(String, Channel, ClassReference, RemoteClassLoader)} for unit tests.
+ * See JENKINS-6604 and similar issues.
+ * Should not be used for any other purpose.
+ */
+ static Interruptible TESTING_CLASS_LOAD;
+ /**
+ * Intercept {@link RemoteClassLoader#prefetchClassReference(String, Channel)} for unit tests.
+ * Should not be used for any other purpose.
+ */
+ static Interruptible TESTING_CLASS_REFERENCE_LOAD;
+ /**
+ * Intercept {@link RemoteClassLoader#findResource(String)} for unit tests.
+ * Should not be used for any other purpose.
+ */
+ static Interruptible TESTING_RESOURCE_LOAD;
+
+ /**
+ * The amount of time to sleep before retrying an interrupted class load.
+ * This sleep keeps it from hammering the channel if there is a failure.
+ * The default value is 100 (ms).
+ */
+ static int RETRY_SLEEP_DURATION_MILLISECONDS = Integer.getInteger(RemoteClassLoader.class.getName() + "retrySleepDurationMilliseconds", 100);
+ /**
+ * The total number of retries for an interrupted class load.
+ * This makes the operation retry for an extended period of time but eventually timeout.
+ * Combined with the default value for RETRY_SLEEP_DURATION_MILLISECONDS this gives a default
+ * timeout of about 10 minutes, which is much less than the former (infinite) retry but still a significant
+ * amount of time.
+ *
+ * Setting this to zero keeps retrying forever.
+ */
+ static int MAX_RETRIES = Integer.getInteger(RemoteClassLoader.class.getName() + "maxRetries", 6000);
+
/**
* Proxy to the code running on remote end.
*
@@ -232,139 +271,140 @@ private Class loadRemoteClass(String name, Channel channel, ClassReference cr
synchronized (rcl.getClassLoadingLock(name)) {
Class c = rcl.findLoadedClass(name);
- 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_LOAD != null) {
- TESTING_CLASS_LOAD.run();
- }
+ int tries = 0;
+ while (true) {
+ try {
+ invokeClassLoadTestingHookIfNeeded();
- if (c != null) {
- return c;
- }
+ if (c != null) {
+ return c;
+ }
- // TODO: check inner class handling
- Future 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
- }
+ // TODO: check inner class handling
+ Future 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) {
+ // 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 | RemotingSystemException x) {
+ tries++;
+ if (shouldRetry(x, tries)) {
// 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;
+ sleepForRetry();
+ LOGGER.finer("Handling interrupt while loading remote class. Current retry count = " + tries + ", maximum = " + MAX_RETRIES);
+ continue;
}
-
- // no code is allowed to reach here
+ break;
}
- } finally {
- // process the interrupt later.
- if (interrupted)
- Thread.currentThread().interrupt();
}
+ throw new ClassNotFoundException("Could not load class " + name + " after " + tries + " tries.");
+ }
+ }
+
+ private void invokeClassLoadTestingHookIfNeeded() throws InterruptedException {
+ // Testing support only.
+ if (TESTING_CLASS_LOAD != null) {
+ TESTING_CLASS_LOAD.run();
}
}
+ private boolean shouldRetry(Throwable e, int tries) {
+ return isRetryException(e) && hasMoreRetries(tries);
+ }
+
+ private boolean hasMoreRetries(int tries) {
+ return MAX_RETRIES <= 0 || tries <= MAX_RETRIES;
+ }
+
+ private boolean isRetryException(Throwable e) {
+ return e instanceof InterruptedException
+ || (e instanceof RemotingSystemException
+ && (e.getCause() instanceof InterruptedException
+ || e.getCause() instanceof InterruptedIOException));
+ }
+
private ClassReference prefetchClassReference(String name, Channel channel) throws ClassNotFoundException {
ClassReference cr;
cr = prefetchedClasses.remove(name);
if (cr == null) {
LOGGER.log(Level.FINER, "fetch3({0})", name);
- 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 shouldn'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);
+ int tries = 0;
+ while (true) {
+ try {
+ invokeClassReferenceLoadTestingHookIfNeeded();
+
+ 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 shouldn'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));
}
- }
- 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;
+ 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 {
- // 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, this);
}
- ref.rememberIn(cn, ref.classLoader);
+ 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;
}
-
- // no code is allowed to reach here
- }
- } finally {
- // process the interrupt later.
- if (interrupted) {
- Thread.currentThread().interrupt();
+ break;
+ } catch (InterruptedException | RemotingSystemException x) {
+ tries++;
+ if (shouldRetry(x, tries)) {
+ // 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.
+ sleepForRetry();
+ LOGGER.finer("Handling interrupt while fetching class reference. Current retry count = " + tries + ", maximum = " + MAX_RETRIES);
+ continue;
+ }
+ throw determineRemotingSystemException(x);
}
+
+ // no code is allowed to reach here
}
assert cr != null;
@@ -375,13 +415,12 @@ ClassReference toRef(ClassFile2 cf) {
return cr;
}
- /**
- * Intercept {@link RemoteClassLoader#findClass(String)} to allow unit tests to be written.
- *
- * See JENKINS-6604 and similar issues
- */
- static Runnable TESTING_CLASS_LOAD;
- static Runnable TESTING_CLASS_REFERENCE_LOAD;
+ private void invokeClassReferenceLoadTestingHookIfNeeded() throws InterruptedException {
+ // Testing support only.
+ if (TESTING_CLASS_REFERENCE_LOAD != null) {
+ TESTING_CLASS_REFERENCE_LOAD.run();
+ }
+ }
/**
* Loads class from the byte array.
@@ -450,38 +489,67 @@ public URL findResource(String name) {
return url;
}
- try {
- if (resourceMap.containsKey(name)) {
- URLish f = resourceMap.get(name);
- if (f == null) {
- return null; // no such resource
+ int tries = 0;
+ while (true) {
+ try {
+ if (resourceMap.containsKey(name)) {
+ URLish f = resourceMap.get(name);
+ if (f == null) {
+ return null; // no such resource
+ }
+ URL u = f.toURL();
+ if (u != null) {
+ return u;
+ }
}
- URL u = f.toURL();
- if (u != null) {
- return u;
+
+ invokeResourceLoadTestingHookIfNeeded();
+
+ long startTime = System.nanoTime();
+
+ ResourceFile r = proxy.getResource2(name);
+ ResourceImageRef image = null;
+ if (r != null) {
+ image = r.image;
}
- }
- long startTime = System.nanoTime();
+ channel.resourceLoadingTime.addAndGet(System.nanoTime() - startTime);
+ channel.resourceLoadingCount.incrementAndGet();
+ if (image == null) {
+ resourceMap.put(name, null);
+ return null;
+ }
- ResourceFile r = proxy.getResource2(name);
- ResourceImageRef image = null;
- if (r != null) {
- image = r.image;
+ URLish res = image.resolveURL(channel, name).get();
+ resourceMap.put(name, res);
+ return res.toURL();
+ } catch (IOException | ExecutionException e) {
+ throw new Error("Unable to load resource " + name, e);
+ } catch (InterruptedException | RemotingSystemException x) {
+ tries++;
+ if (shouldRetry(x, tries)) {
+ // 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.
+ sleepForRetry();
+ LOGGER.finer("Handling interrupt while finding resource. Current retry count = " + tries + ", maximum = " + MAX_RETRIES);
+ continue;
+ }
+ throw determineRemotingSystemException(x);
}
- channel.resourceLoadingTime.addAndGet(System.nanoTime() - startTime);
- channel.resourceLoadingCount.incrementAndGet();
- if (image == null) {
- resourceMap.put(name, null);
- return null;
- }
+ // no code is allowed to reach here
+ }
+ }
- URLish res = image.resolveURL(channel, name).get();
- resourceMap.put(name, res);
- return res.toURL();
- } catch (IOException | InterruptedException | ExecutionException e) {
- throw new Error("Unable to load resource " + name, e);
+ private RemotingSystemException determineRemotingSystemException(Exception x) {
+ return x instanceof RemotingSystemException ? (RemotingSystemException) x : new RemotingSystemException(x);
+ }
+
+ private void invokeResourceLoadTestingHookIfNeeded() throws InterruptedException {
+ // Testing support only.
+ if (TESTING_RESOURCE_LOAD != null) {
+ TESTING_RESOURCE_LOAD.run();
}
}
@@ -513,37 +581,67 @@ public Enumeration findResources(String name) throws IOException {
// the challenge is how to combine the list from local jars
// and the remote list
- Vector v = resourcesMap.get(name);
- if (v != null) {
- Vector urls = toURLs(v);
- if (urls != null) {
- return urls.elements();
- }
- }
+ int tries = 0;
+ while (true) {
+ try {
+ Vector v = resourcesMap.get(name);
+ if (v != null) {
+ Vector urls = toURLs(v);
+ if (urls != null) {
+ return urls.elements();
+ }
+ }
- long startTime = System.nanoTime();
- ResourceFile[] images = proxy.getResources2(name);
- channel.resourceLoadingTime.addAndGet(System.nanoTime() - startTime);
- channel.resourceLoadingCount.incrementAndGet();
+ invokeResourceLoadTestingHookIfNeeded();
- v = new Vector<>();
- for (ResourceFile image : images) {
- try {
- // getResources2 always give us ResourceImageBoth so
- // .get() shouldn't block
- v.add(image.image.resolveURL(channel, name).get());
- } catch (InterruptedException | ExecutionException e) {
- throw new Error("Failed to load resources " + name, e);
+ long startTime = System.nanoTime();
+ ResourceFile[] images = proxy.getResources2(name);
+ channel.resourceLoadingTime.addAndGet(System.nanoTime() - startTime);
+ channel.resourceLoadingCount.incrementAndGet();
+
+ v = new Vector<>();
+ for (ResourceFile image : images) {
+ try {
+ // getResources2 always give us ResourceImageBoth so
+ // .get() shouldn't block
+ v.add(image.image.resolveURL(channel, name).get());
+ } catch (InterruptedException | ExecutionException e) {
+ throw new Error("Failed to load resources " + name, e);
+ }
+ }
+ resourcesMap.put(name, v);
+
+ Vector resURLs = toURLs(v);
+ if (resURLs == null) {
+ // TODO: Better than NPE, but ideally needs correct error propagation from URLish
+ throw new IOException("One of the URLish objects cannot be converted to URL");
+ }
+ return resURLs.elements();
+ } catch (InterruptedException | RemotingSystemException x) {
+ tries++;
+ if (shouldRetry(x, tries)) {
+ // 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.
+ sleepForRetry();
+ LOGGER.finer("Handling interrupt while finding resource. Current retry count = " + tries + ", maximum = " + MAX_RETRIES);
+ continue;
+ }
+ throw determineRemotingSystemException(x);
}
+
+ // no code is allowed to reach here
}
- resourcesMap.put(name, v);
+ }
- Vector resURLs = toURLs(v);
- if (resURLs == null) {
- // TODO: Better than NPE, but ideally needs correct error propagation from URLish
- throw new IOException("One of the URLish objects cannot be converted to URL");
+ private void sleepForRetry() {
+ try {
+ if (RETRY_SLEEP_DURATION_MILLISECONDS > 0) {
+ Thread.sleep(RETRY_SLEEP_DURATION_MILLISECONDS);
+ }
+ } catch (InterruptedException e) {
+ // Not much to do if we can't sleep. Run through the tries more quickly.
}
- return resURLs.elements();
}
/**
diff --git a/src/test/java/hudson/remoting/ClassRemoting2Test.java b/src/test/java/hudson/remoting/ClassRemoting2Test.java
new file mode 100644
index 000000000..176ee2bb2
--- /dev/null
+++ b/src/test/java/hudson/remoting/ClassRemoting2Test.java
@@ -0,0 +1,290 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package hudson.remoting;
+
+import junit.framework.Test;
+import org.junit.After;
+import org.jvnet.hudson.test.Issue;
+
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+
+/**
+ * Remote class loading tests that don't work with the full set of test runners
+ * specified in RmiTestBase for various reasons. These tests may not be valid in
+ * all runner configurations.
+ *
+ * For example, tests that depend on the MULTI_CLASSLOADER capability don't work
+ * with Capability.NONE. Tests that are forked in a different JVM cannot use
+ * test resources.
+ *
+ * The full suite of runners may test configurations that are no longer interesting
+ * or applicable. It doesn't make sense to expect class loading to work with
+ * Capability.NONE.
+ */
+@WithRunner({
+ InProcessRunner.class,
+ NioSocketRunner.class,
+ NioPipeRunner.class,
+})
+public class ClassRemoting2Test extends RmiTestBase {
+
+ @After
+ public void tearDown() throws Exception {
+ super.tearDown();
+ RemoteClassLoader.TESTING_CLASS_REFERENCE_LOAD = null;
+ RemoteClassLoader.TESTING_CLASS_LOAD = null;
+ RemoteClassLoader.TESTING_RESOURCE_LOAD = null;
+ RemoteClassLoader.RETRY_SLEEP_DURATION_MILLISECONDS = 100;
+ }
+
+ @Issue("JENKINS-19453")
+ public void testSingleInterruptionOfClassCreation() 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 callable = (Callable) child2.load(TestLinkage.class);
+ assertEquals(child2, callable.getClass().getClassLoader());
+ RemoteClassLoader.RETRY_SLEEP_DURATION_MILLISECONDS = 1;
+ RemoteClassLoader.MAX_RETRIES = 10;
+ RemoteClassLoader.TESTING_CLASS_LOAD = new InterruptInvocation(3, 3);
+ Future