Skip to content
Permalink
Browse files

[FIXED JENKINS-9017]

Fixed incorrect de-allocation of a classloader from the exported object
table.

The fix is a defense-in-depth; it prevents classloaders referenced in
the object graph from doubly released, then we also make it impossible
for bugs like this to deallocate the key classloader.
  • Loading branch information
kohsuke authored and stephenc committed May 19, 2011
1 parent c617653 commit 688e81cbc38690c67e73729b1d48e1d1bea8acab
@@ -58,6 +58,9 @@
<!-- Record your changes in the trunk here. -->
<div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
<ul class=image>
<li class=bug>
Fixed a <tt>ClassCastException</tt> caused by multiple loading of the same class in different classloaders.
(<a href="http://issues.jenkins-ci.org/browse/JENKINS-9017">issue 9017</a>)
<li class=rfe>
Jenkins has a new logo, thanks to Charles Lowell at The Frontside
<li class=bug>
@@ -336,6 +336,11 @@ public void onClosed(Channel c, IOException cause) {
log.println("WARNING: "+remoteFs+" looks suspiciously like Windows path. Maybe you meant "+remoteFs.replace('\\','/')+"?");
FilePath root = new FilePath(channel,getNode().getRemoteFS());

// reference counting problem is known to happen, such as JENKINS-9017, and so as a preventive measure
// we pin the base classloader so that it'll never get GCed. When this classloader gets released,
// it'll have a catastrophic impact on the communication.
channel.pinClassLoader(getClass().getClassLoader());

channel.call(new SlaveInitializer());
channel.call(new WindowsSlaveInstaller(remoteFs));
for (ComputerListener cl : ComputerListener.all())
@@ -513,9 +513,9 @@ public Thread newThread(Runnable r) {
logger.log(Level.WARNING, "Unable to send GC command",e);
}

// proxy will unexport this instance when it's GC-ed on the remote machine.
final int id = export(instance);
return RemoteInvocationHandler.wrap(null,id,type,userProxy,exportedObjects.isRecording());
// proxy will unexport this instance when it's GC-ed on the remote machine, so don't unexport on our side automatically at the end of a call.
final int id = export(instance,false);
return RemoteInvocationHandler.wrap(null, id, type, userProxy, exportedObjects.isRecording());
}

/*package*/ int export(Object instance) {
@@ -534,6 +534,22 @@ public Thread newThread(Runnable r) {
exportedObjects.unexportByOid(id);
}

/**
* Increase reference count so much to effectively prevent de-allocation.
*
* @see ExportTable.Entry#pin()
*/
public void pin(Object instance) {
exportedObjects.pin(instance);
}

/**
* {@linkplain #pin(Object) Pin down} the exported classloader.
*/
public void pinClassLoader(ClassLoader cl) {
RemoteClassLoader.pin(cl,this);
}

/**
* Preloads jar files on the remote side.
*
@@ -67,7 +67,6 @@
// force the computation of the stack trace in a Java friendly data structure,
// so that the call stack can be seen from the heap dump after the fact.
allocationTrace.getStackTrace();
addRef();

table.put(id,this);
reverse.put(object,this);
@@ -77,6 +76,16 @@ void addRef() {
referenceCount++;
}

/**
* Increase reference count so much to effectively prevent de-allocation.
* If the reference counting is correct, we just need to increment by one,
* but this makes it safer even in case of some reference counting errors
* (and we can still detect the problem by comparing the reference count with the magic value.
*/
void pin() {
referenceCount += Integer.MAX_VALUE/2;
}

void release() {
if(--referenceCount==0) {
table.remove(id);
@@ -155,8 +164,7 @@ public synchronized int export(T t, boolean notifyListener) {
Entry e = reverse.get(t);
if(e==null)
e = new Entry(t);
else
e.addRef();
e.addRef();

if(notifyListener) {
ExportList l = lists.get();
@@ -166,6 +174,13 @@ public synchronized int export(T t, boolean notifyListener) {
return e.id;
}

/*package*/ synchronized void pin(T t) {
Entry e = reverse.get(t);
if(e==null)
e = new Entry(t);
e.pin();
}

public synchronized T get(int id) {
Entry e = table.get(id);
if(e!=null) return e.object;
@@ -352,6 +352,16 @@ public static IClassLoader export(ClassLoader cl, Channel local) {
return local.export(IClassLoader.class, new ClassLoaderProxy(cl,local), false);
}

public static void pin(ClassLoader cl, Channel local) {
if (cl instanceof RemoteClassLoader) {
// check if this is a remote classloader from the channel
final RemoteClassLoader rcl = (RemoteClassLoader) cl;
int oid = RemoteInvocationHandler.unwrap(rcl.proxy, local);
if(oid!=-1) return;
}
local.pin(new ClassLoaderProxy(cl,local));
}

/**
* Exports and just returns the object ID, instead of obtaining the proxy.
*/

0 comments on commit 688e81c

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