Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIXED JENKINS-36991] Do not give up on classloading if loading is interrupted #94

Merged
merged 2 commits into from
Aug 3, 2016
Merged

[FIXED JENKINS-36991] Do not give up on classloading if loading is interrupted #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
899ab4c
[FIXED JENKINS-36991] Do not give up on classloading if loading is in…
olivergondza Jul 28, 2016
5f24dcd
[JENKINS-36991] Propagate unrelated exceptions
olivergondza Jul 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 78 additions & 45 deletions src/main/java/hudson/remoting/RemoteClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String,ClassFile2> 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<Integer,ClassLoader> classLoaders = new HashMap<Integer, ClassLoader>();

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<String,ClassFile2> 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<String,ClassFile2> 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<Integer,ClassLoader> classLoaders = new HashMap<Integer, ClassLoader>();

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<String,ClassFile2> 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IllegalStateException?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have pointed me to the fact I do not propagate exception correctly in the catch. Though, after that is fixed, javac do not let me place the statement here as it will be unreachable statement.

}
} finally {
// process the interrupt later.
if (interrupted)
Thread.currentThread().interrupt();
}

assert cr != null;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
99 changes: 71 additions & 28 deletions src/test/java/hudson/remoting/ClassRemotingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<Object> f1 = svc.submit(new java.util.concurrent.Callable<Object>() {
public Object call() throws Exception {
Expand All @@ -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<Object, Exception> 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<Object> f1 = svc.submit(new java.util.concurrent.Callable<Object>() {
public Object call() throws Exception {
try {
return channel.call(c);
} catch (Throwable t) {
throw new Exception(t);
}
}
});
java.util.concurrent.Future<Object> 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<Object, Exception> 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<Object> 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<Object> scheduleCallableLoad(final Callable<Object, Exception> c) {
ExecutorService svc = Executors.newSingleThreadExecutor();
return svc.submit(new java.util.concurrent.Callable<Object>() {
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();
}
}
}

Expand Down