From 0dfda1e7f60880899e20f8f799b1ca5ed105f0bc Mon Sep 17 00:00:00 2001 From: Connor Petty Date: Thu, 8 Oct 2020 09:38:04 -0700 Subject: [PATCH] Make ExecUtil more robust (#1700) Signed-off-by: Connor Petty --- .../openhab/core/io/net/exec/ExecUtil.java | 127 ++++++++++-------- .../core/io/net/exec/ExecUtilTest.java | 12 +- .../core/model/script/actions/Exec.java | 25 ++-- 3 files changed, 91 insertions(+), 73 deletions(-) diff --git a/bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/exec/ExecUtil.java b/bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/exec/ExecUtil.java index 71ae1087182..65aa94322c9 100644 --- a/bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/exec/ExecUtil.java +++ b/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 - replaced delimiter usage with argument array */ +@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"); /** *

- * Executes commandLine. 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 - * '@@'. If commandLine contains this delimiter it is split into a String list and the - * special exec-method is used. + * Executes commandLine. * *

* 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); + } } /** *

- * Executes commandLine 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 '@@'. If commandLine contains this delimiter it is split into a - * String list and the special exec-method is used. + * Executes commandLine and return its result. * *

* A possible {@link IOException} gets logged but no further processing is done. * + * @param timeout the max time to wait for a process to finish, null 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 null if an error occurred + * @return response data from executed command line or null if a timeout or 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 outputFuture = null; + Future 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 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; } } diff --git a/bundles/org.openhab.core.io.net/src/test/java/org/openhab/core/io/net/exec/ExecUtilTest.java b/bundles/org.openhab.core.io.net/src/test/java/org/openhab/core/io/net/exec/ExecUtilTest.java index eb581d48a9d..8c5373c035f 100644 --- a/bundles/org.openhab.core.io.net/src/test/java/org/openhab/core/io/net/exec/ExecUtilTest.java +++ b/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); diff --git a/bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/actions/Exec.java b/bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/actions/Exec.java index 84e34a76f13..040376251e1 100644 --- a/bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/actions/Exec.java +++ b/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,11 +27,8 @@ public class Exec { /** *

- * Executes commandLine. 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 ' - * @@'. If commandLine contains this delimiter it is split into a String[] array and the - * special exec-method is used. - * + * Executes commandLine. + * *

* A possible {@link IOException} gets logged but no further processing is done. * @@ -38,28 +36,25 @@ public class Exec { * 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); } /** *

- * Executes commandLine. 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 ' - * @@'. If commandLine contains this delimiter it is split into a String[] array and the - * special exec-method is used. - * + * Executes commandLine. + * *

* 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); } }