Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-23271] - Process statuses of Remote process join() operation…
…s directly inside methods (#2653)

* [JENKINS-23271] - Process statuses of Remote process join() operations directly inside methods

* [JENKINS-23271] - Also prevent the issue when the kill() command is the last call in the usage sequence

(cherry picked from commit 2989335)
  • Loading branch information
oleg-nenashev authored and olivergondza committed Dec 8, 2016
1 parent fcfd271 commit 5500e5c
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 15 deletions.
51 changes: 40 additions & 11 deletions core/src/main/java/hudson/Launcher.java
Expand Up @@ -59,6 +59,7 @@
import java.util.logging.Logger;

import static org.apache.commons.io.output.NullOutputStream.NULL_OUTPUT_STREAM;
import hudson.Proc.ProcWithJenkins23271Patch;

/**
* Starts a process.
Expand Down Expand Up @@ -393,15 +394,27 @@ public int join() throws IOException, InterruptedException {
// 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});

if (procHolderForJoin instanceof ProcWithJenkins23271Patch) {
return procHolderForJoin.join();
} else {
// Fallback to the internal handling logic
if (!(procHolderForJoin instanceof LocalProc)) {
// We consider that the process may be at risk of JENKINS-23271
LOGGER.log(Level.FINE, "Process {0} of type {1} is neither {2} nor instance of {3}. "
+ "If this process operates with Jenkins agents via remote invocation, you may get into JENKINS-23271",
new Object[] {procHolderForJoin, procHolderForJoin.getClass(), LocalProc.class, ProcWithJenkins23271Patch.class});
}
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");
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 {0} has not finished after the join() method completion", procHolderForJoin);
}
}
}
}
Expand Down Expand Up @@ -990,7 +1003,7 @@ public Void call() throws RuntimeException {
private static final long serialVersionUID = 1L;
}

public static final class ProcImpl extends Proc {
public static final class ProcImpl extends Proc implements ProcWithJenkins23271Patch {
private final RemoteProcess process;
private final IOTriplet io;

Expand All @@ -1001,12 +1014,28 @@ public ProcImpl(RemoteProcess process) {

@Override
public void kill() throws IOException, InterruptedException {
process.kill();
try {
process.kill();
} finally {
if (this.isAlive()) { // Should never happen but this forces Proc to not be removed and early GC by escape analysis
LOGGER.log(Level.WARNING, "Process {0} has not really finished after the kill() method execution", this);
}
}
}

@Override
public int join() throws IOException, InterruptedException {
return process.join();
try {
final int returnCode = process.join();
if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.log(Level.FINER, "Process {0} has finished with the return code {1}", new Object[]{this, returnCode});
}
return returnCode;
} finally {
if (this.isAlive()) { // Should never happen but this forces Proc to not be removed and early GC by escape analysis
LOGGER.log(Level.WARNING, "Process {0} has not really finished after the join() method completion", this);
}
}
}

@Override
Expand Down
32 changes: 28 additions & 4 deletions core/src/main/java/hudson/Proc.java
Expand Up @@ -49,6 +49,8 @@
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* External process wrapper.
Expand Down Expand Up @@ -431,7 +433,7 @@ private static String calcName(String[] cmd) {
* @deprecated as of 1.399. Replaced by {@link Launcher.RemoteLauncher.ProcImpl}
*/
@Deprecated
public static final class RemoteProc extends Proc {
public static final class RemoteProc extends Proc implements ProcWithJenkins23271Patch {
private final Future<Integer> process;

public RemoteProc(Future<Integer> process) {
Expand All @@ -440,23 +442,34 @@ public RemoteProc(Future<Integer> process) {

@Override
public void kill() throws IOException, InterruptedException {
process.cancel(true);
try {
process.cancel(true);
} finally {
if (this.isAlive()) { // Should never happen but this forces Proc to not be removed and early GC by escape analysis
// TODO: Report exceptions if they happen?
LOGGER.log(Level.WARNING, "Process {0} has not really finished after the kill() method execution", this);
}
}
}

@Override
public int join() throws IOException, InterruptedException {
try {
return process.get();
} catch (InterruptedException e) {
// aborting. kill the process
process.cancel(true);
LOGGER.log(Level.FINE, String.format("Join operation has been interrupted for the process %s. Killing the process", this), e);
kill();
throw e;
} catch (ExecutionException e) {
if(e.getCause() instanceof IOException)
throw (IOException)e.getCause();
throw new IOException("Failed to join the process",e);
} catch (CancellationException x) {
return -1;
} finally {
if (this.isAlive()) { // Should never happen but this forces Proc to not be removed and early GC by escape analysis
LOGGER.log(Level.WARNING, "Process {0} has not really finished after the join() method completion", this);
}
}
}

Expand Down Expand Up @@ -486,4 +499,15 @@ public OutputStream getStdin() {
* Debug switch to have the thread display the process it's waiting for.
*/
public static boolean SHOW_PID = false;

/**
* An instance of {@link Proc}, which has an internal workaround for JENKINS-23271.
* It presumes that the instance of the object is guaranteed to be used after the {@link Proc#join()} call.
* See <a href="https://issues.jenkins-ci.org/browse/JENKINS-23271">JENKINS-23271></a>
* @author Oleg Nenashev
*/
@Restricted(NoExternalUse.class)
public interface ProcWithJenkins23271Patch {
// Empty marker interface
}
}

0 comments on commit 5500e5c

Please sign in to comment.