change: - 'ProcessResult' should no longer access the Process.

SVN: 6244

common/source/java/ch/systemsx/cisd/common/process/ProcessExecutionHelper.java

@@ -22,6 +22,7 @@ import java.io.IOException;
 import java.io.InputStreamReader;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 import org.apache.commons.io.IOUtils;
@@ -202,45 +203,58 @@ public final class ProcessExecutionHelper
     {
         assert millisoWaitForCompletion > 0L : "Unspecified time out.";
         final ProcessWatchdog processWatchdog = new ProcessWatchdog(millisoWaitForCompletion);
+        Process process = null;
         try
         {
-            final Process process = launchProcess(commandLine);
+            process = launchProcess(commandLine);
             processWatchdog.start(process);
             try
             {
                 process.waitFor();
                 processWatchdog.stop();
+                return createResult(commandLine, process, processWatchdog.isProcessKilled(),
+                        readProcessOutputLines(process, machineLog));
             } catch (final InterruptedException e)
             {
-                process.destroy();
                 operationLog.warn(String.format("Execution of %s interrupted after timeout.",
                         commandLine));
+                return createResult(commandLine, process, processWatchdog.isProcessKilled(),
+                        Collections.<String> emptyList());
             }
-            return createResult(commandLine, process, processWatchdog.isProcessKilled());
         } catch (final IOException ex)
         {
             return createNotStartedResult(commandLine, ex);
+        } finally
+        {
+            closeStreams(process);
+            process.destroy();
         }
     }
 
     private final ProcessResult runWithoutWatchdog(final List<String> commandLine)
     {
+        Process process = null;
         try
         {
-            final Process process = launchProcess(commandLine);
+            process = launchProcess(commandLine);
             try
             {
                 process.waitFor();
+                return createResult(commandLine, process, false, readProcessOutputLines(process,
+                        machineLog));
             } catch (final InterruptedException e)
             {
-                process.destroy();
                 operationLog.warn(String.format("Execution of %s interrupted after timeout.",
                         commandLine));
+                return createResult(commandLine, process, true, Collections.<String> emptyList());
             }
-            return createResult(commandLine, process, false);
         } catch (final IOException ex)
         {
             return createNotStartedResult(commandLine, ex);
+        } finally
+        {
+            closeStreams(process);
+            process.destroy();
         }
     }
 
@@ -252,11 +266,7 @@ public final class ProcessExecutionHelper
         {
             operationLog.debug("Executing command: " + commandLine);
         }
-        // NOTE 2008-02-04, Tomasz Pylak: This operation can get blocked. I've observed it when ln
-        // was executed on NAS
-        // file system mounted locally.
-        final Process process = processBuilder.start();
-        return process;
+        return processBuilder.start();
     }
 
     private final ProcessResult createNotStartedResult(final List<String> commandLine,
@@ -267,20 +277,29 @@ public final class ProcessExecutionHelper
     }
 
     private final ProcessResult createResult(final List<String> commandLine,
-            final Process processOrNull, final boolean isInterrupted)
+            final Process processOrNull, final boolean isInterrupted, final List<String> outputLines)
     {
         if (processOrNull == null)
         {
             return ProcessResult.createNotStarted(commandLine, operationLog, machineLog);
         } else
         {
+            final List<String> lines;
+            if (outputLines == null)
+            {
+                lines = readProcessOutputLines(processOrNull, machineLog);
+            } else
+            {
+                lines = outputLines;
+            }
             if (isInterrupted)
             {
                 return ProcessResult.createWaitingInterrupted(processOrNull, commandLine,
-                        operationLog, machineLog);
+                        operationLog, machineLog, lines);
             } else
             {
-                return ProcessResult.create(processOrNull, commandLine, operationLog, machineLog);
+                return ProcessResult.create(processOrNull, commandLine, operationLog, machineLog,
+                        lines);
             }
         }
     }
@@ -296,7 +315,6 @@ public final class ProcessExecutionHelper
             result = runWithoutWatchdog(cmd);
         }
         result.log();
-        result.destroyProcess();
         return result.isOK();
     }
 
@@ -352,4 +370,19 @@ public final class ProcessExecutionHelper
             }
         }
     }
+
+    /**
+     * Close the streams belonging to given <var>Process</var>.
+     */
+    private final static void closeStreams(final Process processOrNull)
+    {
+        if (processOrNull == null)
+        {
+            return;
+        }
+        IOUtils.closeQuietly(processOrNull.getInputStream());
+        IOUtils.closeQuietly(processOrNull.getOutputStream());
+        IOUtils.closeQuietly(processOrNull.getErrorStream());
+    }
+
 }

common/source/java/ch/systemsx/cisd/common/process/ProcessResult.java

@@ -17,7 +17,7 @@
 package ch.systemsx.cisd.common.process;
 
 import java.io.File;
-import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 
 import org.apache.log4j.Level;
@@ -28,11 +28,10 @@ import org.apache.log4j.Logger;
  * <p>
  * Since the process output can only ever be read once from a process, it need to be kept around if
  * it is needed more than once. This is what this class is good for.
+ * </p>
  */
 public final class ProcessResult
 {
-    private final Process processOrNull; // null if not started
-
     private final boolean hasBlocked;
 
     private final List<String> commandLine;
@@ -43,63 +42,69 @@ public final class ProcessResult
 
     private final Logger machineLog;
 
-    private List<String> outputLines;
+    private final int exitValue;
 
-    // process finished or was terminated after timeout
-    public static ProcessResult create(final Process process, final List<String> commandLine,
-            final Logger operationLog, final Logger machineLog)
+    private final List<String> outputLines;
+
+    private final boolean run;
+
+    /**
+     * Creates a <code>ProcessResult</code> for a process which normally terminates.
+     */
+    public final static ProcessResult create(final Process process, final List<String> commandLine,
+            final Logger operationLog, final Logger machineLog, final List<String> outputLines)
     {
-        return new ProcessResult(process, false, commandLine, operationLog, machineLog);
+        return new ProcessResult(process, false, commandLine, operationLog, machineLog, outputLines);
     }
 
-    // process could not start at all
+    /**
+     * Creates a <code>ProcessResult</code> for a process which did not start at all.
+     */
     public static ProcessResult createNotStarted(final List<String> commandLine,
             final Logger operationLog, final Logger machineLog)
     {
-        return new ProcessResult(null, false, commandLine, operationLog, machineLog);
+        return new ProcessResult(null, false, commandLine, operationLog, machineLog, Collections
+                .<String> emptyList());
     }
 
-    // process started, but was blocked and could not be terminated, so we stopped waiting for it
+    /**
+     * Creates a <code>ProcessResult</code> for a process which blocked and could not be
+     * terminated. So we stopped waiting for it.
+     */
     public static ProcessResult createWaitingInterrupted(final Process process,
-            final List<String> commandLine, final Logger operationLog, final Logger machineLog)
+            final List<String> commandLine, final Logger operationLog, final Logger machineLog,
+            final List<String> outputLines)
     {
-        return new ProcessResult(process, true, commandLine, operationLog, machineLog);
+        return new ProcessResult(process, true, commandLine, operationLog, machineLog, outputLines);
     }
 
     private ProcessResult(final Process processOrNull, final boolean hasBlocked,
-            final List<String> commandLine, final Logger operationLog, final Logger machineLog)
+            final List<String> commandLine, final Logger operationLog, final Logger machineLog,
+            final List<String> outputLines)
     {
         this.commandLine = commandLine;
         this.commandName = new File(commandLine.get(0)).getName();
-        this.processOrNull = processOrNull;
         this.hasBlocked = hasBlocked;
         this.operationLog = operationLog;
         this.machineLog = machineLog;
-        this.outputLines = null;
+        this.exitValue = getExitValue(processOrNull, hasBlocked);
+        this.outputLines = outputLines;
+        this.run = processOrNull != null;
     }
 
-    /**
-     * Calls the {@link Process#destroy()} method which explicitly closes some file handles.
-     * <p>
-     * <i>Note that one must not call {#link {@link #getProcessOutput()} for the first time after
-     * this method has been called.</i>
-     * <p>
-     * Whether it is necessary to call this method depends on the JRE. For some JREs there occur
-     * {@link IOException}s with code 24 ("Too many open files") when running processes with high
-     * frequency without calling this method.
-     */
-    public void destroyProcess()
+    private final static int getExitValue(final Process processOrNull, final boolean hasBlocked)
     {
-        if (processOrNull != null)
+        if (processOrNull != null && hasBlocked == false)
         {
-            processOrNull.destroy();
+            return processOrNull.exitValue();
         }
+        return ProcessExecutionHelper.NO_EXIT_VALUE;
     }
 
     /**
      * Returns the command line that belongs to this process.
      */
-    public List<String> getCommandLine()
+    public final List<String> getCommandLine()
     {
         return commandLine;
     }
@@ -107,7 +112,7 @@ public final class ProcessResult
     /**
      * Returns the name of the command that belongs to this process.
      */
-    public String getCommandName()
+    public final String getCommandName()
     {
         return commandName;
     }
@@ -115,27 +120,17 @@ public final class ProcessResult
     /**
      * Returns the lines of the process output.
      */
-    public List<String> getProcessOutput()
+    public final List<String> getProcessOutput()
     {
-        if (outputLines == null)
-        {
-            outputLines = ProcessExecutionHelper.readProcessOutputLines(processOrNull, machineLog);
-        }
         return outputLines;
     }
 
-    public int exitValue()
+    public final int exitValue()
     {
-        if (hasBlocked == false && processOrNull != null)
-        {
-            return processOrNull.exitValue();
-        } else
-        {
-            return ProcessExecutionHelper.NO_EXIT_VALUE;
-        }
+        return exitValue;
     }
 
-    public boolean isOK()
+    public final boolean isOK()
     {
         return exitValue() == ProcessExecutionHelper.EXIT_VALUE_OK;
     }
@@ -143,31 +138,31 @@ public final class ProcessResult
     /**
      * Returns <code>true</code> if the process has been run at all.
      */
-    public boolean isRun()
+    public final boolean isRun()
     {
-        return processOrNull != null;
+        return run;
     }
 
     /**
      * Returns <code>true</code> if the process could not been terminated after the timeout and we
      * stopped waiting for it.
      */
-    public boolean hasBlocked()
+    public final boolean hasBlocked()
     {
         return hasBlocked;
     }
 
     /**
-     * Returns <code>true</code> if the process has been terminated on the Operating System level.
+     * Returns <code>true</code> if the process has been terminated on the <i>Operating System</i>
+     * level.
      */
-    public boolean isTerminated()
+    public final boolean isTerminated()
     {
         return ProcessExecutionHelper.isProcessTerminated(exitValue());
     }
 
-    public void log()
+    public final void log()
     {
-
         if (isOK() == false)
         {
             logProcessExitValue(Level.WARN);
@@ -179,7 +174,7 @@ public final class ProcessResult
         }
     }
 
-    private void logProcessExitValue(final Level logLevel)
+    private final void logProcessExitValue(final Level logLevel)
     {
         if (isRun() == false)
         {
@@ -199,7 +194,7 @@ public final class ProcessResult
         }
     }
 
-    private void logProcessOutput(final Level logLevel)
+    private final void logProcessOutput(final Level logLevel)
     {
         assert logLevel != null;