[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)

core/src/main/java/hudson/Launcher.java

@@ -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.
@@ -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);
+                    }
                 }
             }
         }
@@ -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;
 
@@ -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

core/src/main/java/hudson/Proc.java

@@ -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.
@@ -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) {
@@ -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);
+                }
             }
         }
 
@@ -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
+    }
 }