From 3e8792b8d2133b2b1d67e503f0f0e53d192de90f Mon Sep 17 00:00:00 2001 From: stustison Date: Tue, 31 Jan 2017 14:22:00 -0700 Subject: [PATCH] Replaced ProcessBuilder usage with commons-exec --- frontend-plugin-core/pom.xml | 6 + .../frontend/lib/InputStreamHandler.java | 65 ------- .../plugins/frontend/lib/NodeExecutor.java | 8 +- .../plugins/frontend/lib/NodeInstaller.java | 2 +- .../plugins/frontend/lib/ProcessExecutor.java | 165 ++++++++++++------ .../plugins/frontend/lib/YarnExecutor.java | 4 +- .../plugins/frontend/lib/YarnInstaller.java | 10 +- 7 files changed, 125 insertions(+), 135 deletions(-) delete mode 100644 frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/InputStreamHandler.java diff --git a/frontend-plugin-core/pom.xml b/frontend-plugin-core/pom.xml index 532ba7b7e..cd1682388 100644 --- a/frontend-plugin-core/pom.xml +++ b/frontend-plugin-core/pom.xml @@ -35,6 +35,12 @@ 1.3.2 + + org.apache.commons + commons-exec + 1.3 + + org.apache.httpcomponents httpclient diff --git a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/InputStreamHandler.java b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/InputStreamHandler.java deleted file mode 100644 index 198c3eec2..000000000 --- a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/InputStreamHandler.java +++ /dev/null @@ -1,65 +0,0 @@ -package com.github.eirslett.maven.plugins.frontend.lib; - -import org.slf4j.Logger; - -import java.io.BufferedReader; -import java.io.IOException; -import java.io.InputStream; -import java.io.InputStreamReader; - -final class InputStreamHandler extends Thread { - private interface LogLevelAgnosticLogger { - public void log(String value); - } - - private final InputStream inputStream; - private final LogLevelAgnosticLogger logger; - - private InputStreamHandler(InputStream inputStream, LogLevelAgnosticLogger logger) { - this.inputStream = inputStream; - this.logger = logger; - } - - public static InputStreamHandler logInfo(InputStream inputStream, Logger logger){ - return new InputStreamHandler(inputStream, infoLoggerFor(logger)); - } - - public static InputStreamHandler logError(InputStream inputStream, Logger logger){ - return new InputStreamHandler(inputStream, errorLoggerFor(logger)); - } - - public void run(){ - BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream)); - String line; - try { - while((line = reader.readLine()) != null) { - logger.log(line); - } - } catch (IOException e) { - logger.log(e.getMessage()); - } - } - - private static LogLevelAgnosticLogger infoLoggerFor(final Logger logger){ - return new LogLevelAgnosticLogger() { - @Override - public void log(String value) { - logger.info(value); - } - }; - } - - private static LogLevelAgnosticLogger errorLoggerFor(final Logger logger){ - return new LogLevelAgnosticLogger() { - @Override - public void log(String value) { - // fix #343 and #515 - if (value.startsWith("npm WARN ") || value.startsWith("warning ")) { - logger.warn(value); - } else { - logger.error(value); - } - } - }; - } -} diff --git a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NodeExecutor.java b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NodeExecutor.java index 47b1cae84..75585dbbb 100644 --- a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NodeExecutor.java +++ b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NodeExecutor.java @@ -1,11 +1,11 @@ package com.github.eirslett.maven.plugins.frontend.lib; -import org.slf4j.Logger; - import java.util.ArrayList; import java.util.List; import java.util.Map; +import org.slf4j.Logger; + final class NodeExecutor { private final ProcessExecutor executor; @@ -21,8 +21,8 @@ public NodeExecutor(NodeExecutorConfig config, List arguments, Map localPaths; - private final List command; - private final ProcessBuilder processBuilder; - private final Platform platform; - private final Map additionalEnvironment; + private final Map environment; + private CommandLine commandLine; + private final Executor executor; public ProcessExecutor(File workingDirectory, List paths, List command, Platform platform, Map additionalEnvironment){ - this.workingDirectory = workingDirectory; - this.localPaths = paths; - this.command = command; - this.platform = platform; - this.additionalEnvironment = additionalEnvironment; + this(workingDirectory, paths, command, platform, additionalEnvironment, 0); + } - this.processBuilder = createProcessBuilder(); + public ProcessExecutor(File workingDirectory, List paths, List command, Platform platform, Map additionalEnvironment, long timeoutInSeconds) { + this.environment = createEnvironment(paths, platform, additionalEnvironment); + this.commandLine = createCommandLine(command); + this.executor = createExecutor(workingDirectory, timeoutInSeconds); } - public String executeAndGetResult() throws ProcessExecutionException { - try { - final Process process = processBuilder.start(); - final String result = readString(process.getInputStream()); - final String error = readString(process.getErrorStream()); - final int exitValue = process.waitFor(); + public String executeAndGetResult(final Logger logger) throws ProcessExecutionException { + ByteArrayOutputStream stdout = new ByteArrayOutputStream(); + ByteArrayOutputStream stderr = new ByteArrayOutputStream(); - if(exitValue == 0){ - return result; - } else { - throw new ProcessExecutionException(result+" "+error); - } - } catch (IOException e) { - throw new ProcessExecutionException(e); - } catch (InterruptedException e) { - throw new ProcessExecutionException(e); + int exitValue = execute(logger, stdout, stderr); + if (exitValue == 0) { + return stdout.toString().trim(); + } else { + throw new ProcessExecutionException(stdout + " " + stderr); } } public int executeAndRedirectOutput(final Logger logger) throws ProcessExecutionException { + OutputStream stdout = new LoggerOutputStream(logger, 0); + OutputStream stderr = new LoggerOutputStream(logger, 1); + + return execute(logger, stdout, stderr); + } + + private int execute(final Logger logger, final OutputStream stdout, final OutputStream stderr) + throws ProcessExecutionException { + logger.debug("Executing command line {}", commandLine); try { - final Process process = processBuilder.start(); + ExecuteStreamHandler streamHandler = new PumpStreamHandler(stdout, stderr); + executor.setStreamHandler(streamHandler); - final Thread infoLogThread = InputStreamHandler.logInfo(process.getInputStream(), logger); - infoLogThread.start(); - final Thread errorLogThread = InputStreamHandler.logError(process.getErrorStream(), logger); - errorLogThread.start(); + int exitValue = executor.execute(commandLine, environment); + logger.debug("Exit value {}", exitValue); - int result = process.waitFor(); - infoLogThread.join(); - errorLogThread.join(); - return result; - } catch (IOException e) { + return exitValue; + } catch (ExecuteException e) { + if (executor.getWatchdog() != null && executor.getWatchdog().killedProcess()) { + throw new ProcessExecutionException("Process killed after timeout"); + } throw new ProcessExecutionException(e); - } catch (InterruptedException e) { + } catch (IOException e) { throw new ProcessExecutionException(e); } } - private ProcessBuilder createProcessBuilder(){ - ProcessBuilder pbuilder = new ProcessBuilder(command).directory(workingDirectory); - final Map environment = pbuilder.environment(); + private CommandLine createCommandLine(List command) { + CommandLine commmandLine = new CommandLine(command.get(0)); + + for (int i = 1;i < command.size();i++) { + String argument = command.get(i); + commmandLine.addArgument(argument, false); + } + + return commmandLine; + } + + private Map createEnvironment(List paths, Platform platform, Map additionalEnvironment) { + final Map environment = new HashMap<>(System.getenv()); String pathVarName = "PATH"; String pathVarValue = environment.get(pathVarName); if (platform.isWindows()) { - for (String key:environment.keySet()) { - if ("PATH".equalsIgnoreCase(key)) { - pathVarName = key; - pathVarValue = environment.get(key); + for (Map.Entry entry : environment.entrySet()) { + if ("PATH".equalsIgnoreCase(entry.getKey())) { + pathVarName = entry.getKey(); + pathVarValue = entry.getValue(); } } } @@ -94,24 +113,54 @@ private ProcessBuilder createProcessBuilder(){ if (pathVarValue != null) { pathBuilder.append(pathVarValue).append(File.pathSeparator); } - for (String path : localPaths) { - pathBuilder.insert(0, File.pathSeparator).insert(0, path); + for (String path : paths) { + pathBuilder.insert(0, File.pathSeparator).insert(0, path); } environment.put(pathVarName, pathBuilder.toString()); if (additionalEnvironment != null) { environment.putAll(additionalEnvironment); } - return pbuilder; + + return environment; + } + + private Executor createExecutor(File workingDirectory, long timeoutInSeconds) { + DefaultExecutor executor = new DefaultExecutor(); + executor.setWorkingDirectory(workingDirectory); + executor.setProcessDestroyer(new ShutdownHookProcessDestroyer()); // Fixes #41 + + if (timeoutInSeconds > 0) { + executor.setWatchdog(new ExecuteWatchdog(timeoutInSeconds * 1000)); + } + + return executor; } - private static String readString(InputStream processInputStream) throws IOException { - BufferedReader inputStream = new BufferedReader(new InputStreamReader(processInputStream)); - StringBuilder result = new StringBuilder(); - String line; - while((line = inputStream.readLine()) != null) { - result.append(line).append("\n"); + private static class LoggerOutputStream extends LogOutputStream { + private final Logger logger; + + LoggerOutputStream(Logger logger, int logLevel) { + super(logLevel); + this.logger = logger; + } + + @Override + public final void flush() { + // buffer processing on close() only + } + + @Override + protected void processLine(final String line, final int logLevel) { + if (logLevel == 0) { + logger.info(line); + } else { + if (line.startsWith("npm WARN ")) { + logger.warn(line); + } else { + logger.error(line); + } + } } - return result.toString().trim(); } } diff --git a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnExecutor.java b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnExecutor.java index 251e11284..72f8d687c 100644 --- a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnExecutor.java +++ b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnExecutor.java @@ -20,8 +20,8 @@ public YarnExecutor(YarnExecutorConfig config, List arguments, Utils.prepend(yarn, arguments), config.getPlatform(), additionalEnvironment); } - public String executeAndGetResult() throws ProcessExecutionException { - return executor.executeAndGetResult(); + public String executeAndGetResult(final Logger logger) throws ProcessExecutionException { + return executor.executeAndGetResult(logger); } public int executeAndRedirectOutput(final Logger logger) throws ProcessExecutionException { diff --git a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnInstaller.java b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnInstaller.java index 9aa4d8e27..9e203d20e 100644 --- a/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnInstaller.java +++ b/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/YarnInstaller.java @@ -1,13 +1,13 @@ package com.github.eirslett.maven.plugins.frontend.lib; -import org.apache.commons.io.FileUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.File; import java.io.IOException; import java.util.Arrays; +import org.apache.commons.io.FileUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class YarnInstaller { public static final String INSTALL_PATH = "/node/yarn"; @@ -75,7 +75,7 @@ private boolean yarnIsAlreadyInstalled() { File nodeFile = executorConfig.getYarnPath(); if (nodeFile.exists()) { final String version = - new YarnExecutor(executorConfig, Arrays.asList("--version"), null).executeAndGetResult().trim(); + new YarnExecutor(executorConfig, Arrays.asList("--version"), null).executeAndGetResult(logger).trim(); if (version.equals(yarnVersion.replaceFirst("^v", ""))) { logger.info("Yarn {} is already installed.", version);