Make ExecUtil more robust

bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/exec/ExecUtil.java

@@ -14,12 +14,19 @@
 
 import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.InputStreamReader;
-import java.util.Arrays;
-import java.util.List;
+import java.io.StringWriter;
+import java.lang.ProcessBuilder.Redirect;
+import java.time.Duration;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.openhab.core.common.ThreadPoolManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -28,87 +35,101 @@
  *
  * @author Pauli Anttila - Initial contribution
  * @author Kai Kreuzer - added exception logging
+ * @author Connor Petty - rewrote
  */
+@NonNullByDefault
 public class ExecUtil {
 
     private static Logger logger = LoggerFactory.getLogger(ExecUtil.class);
 
-    /**
-     * Use this to separate between command and parameter, and also between parameters.
-     */
-    public static final String CMD_LINE_DELIMITER = "@@";
+    private static ExecutorService executor = ThreadPoolManager.getPool("ExecUtil");
 
     /**
      * <p>
-     * Executes <code>commandLine</code>. Sometimes (especially observed on MacOS) the commandLine isn't executed
-     * properly. In that cases another exec-method is to be used. To accomplish this please use the special delimiter
-     * '<code>@@</code>'. If <code>commandLine</code> contains this delimiter it is split into a String list and the
-     * special exec-method is used.
+     * Executes <code>commandLine</code>.
      *
      * <p>
      * A possible {@link IOException} gets logged but no further processing is done.
      *
      * @param commandLine the command line to execute
      */
-    public static void executeCommandLine(String commandLine) {
-        internalExecute(commandLine);
+    public static void executeCommandLine(String... commandLine) {
+        try {
+            new ProcessBuilder(commandLine).redirectError(Redirect.DISCARD).redirectOutput(Redirect.DISCARD).start();
+        } catch (IOException e) {
+            logger.warn("Error occurred when executing commandLine '{}'", commandLine, e);
+        }
     }
 
     /**
      * <p>
-     * Executes <code>commandLine</code> and return its result. Sometimes (especially observed on MacOS) the commandLine
-     * isn't executed properly. In that cases another exec-method is to be used. To accomplish this please use the
-     * special delimiter '<code>@@</code>'. If <code>commandLine</code> contains this delimiter it is split into a
-     * String list and the special exec-method is used.
+     * Executes <code>commandLine</code> and return its result.
      *
      * <p>
      * A possible {@link IOException} gets logged but no further processing is done.
      *
+     * @param timeout timeout for execution in milliseconds, 0 to wait indefinitely
      * @param commandLine the command line to execute
-     * @param timeout timeout for execution in milliseconds
      * @return response data from executed command line or <code>null</code> if an error occurred
      */
-    public static String executeCommandLineAndWaitResponse(String commandLine, int timeout) {
-        final Process process = internalExecute(commandLine);
-        if (process != null) {
-            try {
-                process.waitFor(timeout, TimeUnit.MILLISECONDS);
-                int exitCode = process.exitValue();
-                final StringBuilder result = new StringBuilder();
-                try (final BufferedReader reader = new BufferedReader(
-                        new InputStreamReader(process.getInputStream()))) {
-                    String line = "";
-                    while ((line = reader.readLine()) != null) {
-                        result.append(line).append("\n");
-                    }
+    public static @Nullable String executeCommandLineAndWaitResponse(@Nullable Duration timeout,
+            String... commandLine) {
+
+        Process processTemp = null;
+        Future<String> outputFuture = null;
+        Future<String> errorFuture = null;
+        cleanup: try {
+            Process process = processTemp = new ProcessBuilder(commandLine).start();
+
+            outputFuture = executor.submit(() -> {
+                try (InputStream inputStream = process.getInputStream();
+                        BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) {
+                    StringWriter output = new StringWriter();
+                    reader.transferTo(output);
+                    return output.toString();
                 }
-                logger.debug("exit code '{}', result '{}'", exitCode, result);
-                return result.toString();
-            } catch (IOException e) {
-                logger.debug("I/O exception occurred when executing commandLine '{}'", commandLine, e);
-            } catch (InterruptedException e) {
+            });
+
+            errorFuture = executor.submit(() -> {
+                try (InputStream inputStream = process.getErrorStream();
+                        BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) {
+                    StringWriter output = new StringWriter();
+                    reader.transferTo(output);
+                    return output.toString();
+                }
+            });
+            int exitCode;
+            if (timeout == null) {
+                exitCode = process.waitFor();
+            } else if (process.waitFor(timeout.toMillis(), TimeUnit.MILLISECONDS)) {
+                exitCode = process.exitValue();
+            } else {
                 logger.warn("Timeout occurred when executing commandLine '{}'", commandLine);
-                logger.debug("{}", e.getMessage(), e);
+                break cleanup;
             }
-        }
-        return null;
-    }
-
-    private static @Nullable Process internalExecute(String commandLine) {
-        final Process process;
-        try {
-            if (commandLine.contains(CMD_LINE_DELIMITER)) {
-                final List<String> cmdArray = Arrays.asList(commandLine.split(CMD_LINE_DELIMITER));
-                process = new ProcessBuilder(cmdArray).start();
-                logger.debug("executed commandLine '{}'", cmdArray);
+            if (exitCode == 0) {
+                return outputFuture.get();
             } else {
-                process = new ProcessBuilder(commandLine).start();
-                logger.debug("executed commandLine '{}'", commandLine);
+                logger.debug("exit code '{}', result '{}', errors '{}'", exitCode, outputFuture.get(),
+                        errorFuture.get());
+                return null;
             }
+        } catch (ExecutionException e) {
+            logger.warn("Error occurred when executing commandLine '{}'", commandLine, e.getCause());
+        } catch (InterruptedException e) {
+            logger.debug("commandLine '{}' was interrupted", commandLine, e);
         } catch (IOException e) {
-            logger.debug("couldn't execute commandLine '{}'", commandLine, e);
-            return null;
+            logger.warn("Error occurred when executing commandLine '{}'", commandLine, e);
         }
-        return process;
+        if (processTemp != null && processTemp.isAlive()) {
+            processTemp.destroyForcibly();
+        }
+        if (outputFuture != null) {
+            outputFuture.cancel(true);
+        }
+        if (errorFuture != null) {
+            errorFuture.cancel(true);
+        }
+        return null;
     }
 }

bundles/org.openhab.core.io.net/src/test/java/org/openhab/core/io/net/exec/ExecUtilTest.java

@@ -14,6 +14,8 @@
 
 import static org.junit.jupiter.api.Assertions.*;
 
+import java.time.Duration;
+
 import org.junit.jupiter.api.Test;
 
 /**
@@ -26,7 +28,7 @@ public class ExecUtilTest {
     @Test
     public void testBasicExecuteCommandLine() {
         if (isWindowsSystem()) {
-            ExecUtil.executeCommandLine("cmd@@/c@@dir");
+            ExecUtil.executeCommandLine("cmd", "/c", "dir");
         } else {
             ExecUtil.executeCommandLine("ls");
         }
@@ -36,9 +38,9 @@ public void testBasicExecuteCommandLine() {
     public void testBasicExecuteCommandLineAndWaitResponse() {
         final String result;
         if (isWindowsSystem()) {
-            result = ExecUtil.executeCommandLineAndWaitResponse("cmd@@/c@@dir", 1000);
+            result = ExecUtil.executeCommandLineAndWaitResponse(Duration.ofSeconds(1), "cmd", "/c", "dir");
         } else {
-            result = ExecUtil.executeCommandLineAndWaitResponse("ls", 1000);
+            result = ExecUtil.executeCommandLineAndWaitResponse(Duration.ofSeconds(1), "ls");
         }
         assertNotNull(result);
         assertNotEquals("", result);
@@ -48,9 +50,9 @@ public void testBasicExecuteCommandLineAndWaitResponse() {
     public void testExecuteCommandLineAndWaitResponseWithArguments() {
         final String result;
         if (isWindowsSystem()) {
-            result = ExecUtil.executeCommandLineAndWaitResponse("cmd@@/c@@echo@@test", 1000);
+            result = ExecUtil.executeCommandLineAndWaitResponse(Duration.ofSeconds(1), "cmd", "/c", "echo", "test");
         } else {
-            result = ExecUtil.executeCommandLineAndWaitResponse("echo@@'test'", 1000);
+            result = ExecUtil.executeCommandLineAndWaitResponse(Duration.ofSeconds(1), "echo", "'test'");
         }
         assertNotNull(result);
         assertNotEquals("test", result);

bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/actions/Exec.java

@@ -13,6 +13,7 @@
 package org.openhab.core.model.script.actions;
 
 import java.io.IOException;
+import java.time.Duration;
 
 import org.openhab.core.io.net.exec.ExecUtil;
 
@@ -26,40 +27,34 @@ public class Exec {
 
     /**
      * <p>
-     * Executes <code>commandLine</code>. Sometimes (especially observed on MacOS) the commandLine isn't executed
-     * properly. In that cases another exec-method is to be used. To accomplish this please use the special delimiter '
-     * <code>@@</code>'. If <code>commandLine</code> contains this delimiter it is split into a String[] array and the
-     * special exec-method is used.
-     * 
+     * Executes <code>commandLine</code>.
+     *
      * <p>
      * A possible {@link IOException} gets logged but no further processing is done.
      *
      * @param commandLine
      *            the command line to execute
      * @see http://www.peterfriese.de/running-applescript-from-java/
      */
-    static public void executeCommandLine(String commandLine) {
+    public static void executeCommandLine(String... commandLine) {
         ExecUtil.executeCommandLine(commandLine);
     }
 
     /**
      * <p>
-     * Executes <code>commandLine</code>. Sometimes (especially observed on MacOS) the commandLine isn't executed
-     * properly. In that cases another exec-method is to be used. To accomplish this please use the special delimiter '
-     * <code>@@</code>'. If <code>commandLine</code> contains this delimiter it is split into a String[] array and the
-     * special exec-method is used.
-     * 
+     * Executes <code>commandLine</code>.
+     *
      * <p>
      * A possible {@link IOException} gets logged but no further processing is done.
      *
+     * @param timeout
+     *            timeout for execution, if null will wait indefinitely
      * @param commandLine
      *            the command line to execute
-     * @param timeout
-     *            timeout for execution in milliseconds
      * @return response data from executed command line
      */
-    static public String executeCommandLine(String commandLine, int timeout) {
-        return ExecUtil.executeCommandLineAndWaitResponse(commandLine, timeout);
+    public static String executeCommandLine(Duration timeout, String... commandLine) {
+        return ExecUtil.executeCommandLineAndWaitResponse(timeout, commandLine);
     }
 
 }