Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-23271] - Prevent early deallocation of the Proc instance by …
…GC in ProcStarter#join() (#2635)

* [JENKINS-23271] - Prevent the prelimimary deallocation of the Proc instance by GC

It is a hackish way, which likely prevents a preliminary deallocation of the spawned RemoteProc instance, which we see in JENKINS-23271. Proc instance was not actually required in the original code since we were creating and using RemoteInvocationHandler wrapper only, and the theory discussed with @stephenc was that object gets removed by Java8 garbage collector before we get into join().

This fix enforces the persistency of ProcStarter#start() result by adding logging and the enforced volatile field (maybe the last one is not really required, but JIT compiler in Java implementations may be smart enough to skip unused loggers)

This is a pretty old fix from August, which has been soak tested on my instance for several weeks (mid-August => Jenkins World). On the reference instance (just a small Jenkins instance with 4 agents and very frequent builds with CommandInterpreter steps) I saw 2 failures over the period. On the fixed instance - 0. It does not proof anything, but at least the fix was soak tested a bit

* [JENKINS-23271] - Get rid of the procHolderForJoin field

* [JENKINS-23271] - Also put the check into the finally statement as @stephenc proposed

* Remove assert

(cherry picked from commit fd6c6af)
  • Loading branch information
oleg-nenashev authored and olivergondza committed Dec 8, 2016
1 parent dacbe03 commit fcfd271
Showing 1 changed file with 19 additions and 1 deletion.
20 changes: 19 additions & 1 deletion core/src/main/java/hudson/Launcher.java
Expand Up @@ -42,6 +42,8 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.CheckForNull;
import javax.annotation.concurrent.GuardedBy;
import java.io.BufferedOutputStream;
import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -383,9 +385,25 @@ public Proc start() throws IOException {

/**
* Starts the process and waits for its completion.
* @return Return code of the invoked process
* @throws IOException Operation error (e.g. remote call failure)
* @throws InterruptedException The process has been interrupted
*/
public int join() throws IOException, InterruptedException {
return start().join();
// The logging around procHolderForJoin prevents the preliminary object deallocation we saw in JENKINS-23271
final Proc procHolderForJoin = start();
LOGGER.log(Level.FINER, "Started the process {0}", procHolderForJoin);
try {
final int returnCode = procHolderForJoin.join();
if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.log(Level.FINER, "Process {0} has finished with the return code {1}", new Object[]{procHolderForJoin, returnCode});
}
return returnCode;
} finally {
if (procHolderForJoin.isAlive()) { // Should never happen but this forces Proc to not be removed and early GC by escape analysis
LOGGER.log(Level.WARNING, "Process not finished after call to join() completed");
}
}
}

/**
Expand Down

0 comments on commit fcfd271

Please sign in to comment.