From 29893354a52efac20efcb4e0e91723f3bd889059 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Sat, 3 Dec 2016 16:55:35 +0100 Subject: [PATCH] [JENKINS-23271] - Process statuses of Remote process join() operations 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 --- core/src/main/java/hudson/Launcher.java | 51 +++++++++++++++++++------ core/src/main/java/hudson/Proc.java | 32 ++++++++++++++-- 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/hudson/Launcher.java b/core/src/main/java/hudson/Launcher.java index 3f8c24d3a4..4e8d0b58ab 100644 --- a/core/src/main/java/hudson/Launcher.java +++ b/core/src/main/java/hudson/Launcher.java @@ -59,6 +59,7 @@ import java.util.logging.Level; import java.util.logging.Logger; import static org.apache.commons.io.output.NullOutputStream.NULL_OUTPUT_STREAM; +import hudson.Proc.ProcWithJenkins23271Patch; /** * Starts a process. @@ -393,15 +394,27 @@ public abstract class Launcher { // 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); + } } } } @@ -990,7 +1003,7 @@ public abstract class Launcher { 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; @@ -1001,12 +1014,28 @@ public abstract class Launcher { @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 diff --git a/core/src/main/java/hudson/Proc.java b/core/src/main/java/hudson/Proc.java index a2d1729827..abe104462a 100644 --- a/core/src/main/java/hudson/Proc.java +++ b/core/src/main/java/hudson/Proc.java @@ -49,6 +49,8 @@ import java.util.concurrent.Future; 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. @@ -431,7 +433,7 @@ public abstract class Proc { * @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 process; public RemoteProc(Future process) { @@ -440,7 +442,14 @@ public abstract class Proc { @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 @@ -448,8 +457,8 @@ public abstract class Proc { 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) @@ -457,6 +466,10 @@ public abstract class Proc { 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); + } } } @@ -486,4 +499,15 @@ public abstract class Proc { * 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 JENKINS-23271> + * @author Oleg Nenashev + */ + @Restricted(NoExternalUse.class) + public interface ProcWithJenkins23271Patch { + // Empty marker interface + } }