From 71535b53ea497bfe05fbf5dfc40c004d2b45bdc3 Mon Sep 17 00:00:00 2001 From: Matthew Khouzam Date: Tue, 10 Jan 2017 14:26:59 -0500 Subject: [PATCH] common.core: clean up ProcessUtils a bit While the code does work, it has some issues that could potentially be problems with the code if not used in the way it should. * Extract magic number to constants This is simple, it makes modifying the behaviour easier. * Explicitly fill code blocks for catch blocks Improve code readability. It makes it clear that it is intentional. * Remove redundant modifier on interfaces An interface is always static, there is no need to add the keyword "static" to it. * Add charsets to inputstream This explicitly expects the default encoding (UTF8). However, this is not the ideal solution, it would be much better, later to have an interface with the charset passed in it. It can be used if we want to launch a process in another machine. Change-Id: I5ef919e724dc28d9e73c5276811af88b4de12f67 Signed-off-by: Matthew Khouzam Reviewed-on: https://git.eclipse.org/r/88407 Reviewed-by: Hudson CI Reviewed-by: Alexandre Montplaisir Tested-by: Alexandre Montplaisir --- .../META-INF/MANIFEST.MF | 3 ++- .../common/core/process/ProcessUtils.java | 25 +++++++++++++------ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/common/org.eclipse.tracecompass.common.core/META-INF/MANIFEST.MF b/common/org.eclipse.tracecompass.common.core/META-INF/MANIFEST.MF index afb10aff6d..e37c2c729f 100644 --- a/common/org.eclipse.tracecompass.common.core/META-INF/MANIFEST.MF +++ b/common/org.eclipse.tracecompass.common.core/META-INF/MANIFEST.MF @@ -17,4 +17,5 @@ Export-Package: org.eclipse.tracecompass.common.core, org.eclipse.tracecompass.common.core.math, org.eclipse.tracecompass.common.core.process, org.eclipse.tracecompass.internal.common.core;x-internal:=true -Import-Package: com.google.common.collect +Import-Package: com.google.common.base, + com.google.common.collect diff --git a/common/org.eclipse.tracecompass.common.core/src/org/eclipse/tracecompass/common/core/process/ProcessUtils.java b/common/org.eclipse.tracecompass.common.core/src/org/eclipse/tracecompass/common/core/process/ProcessUtils.java index 479988c68f..1338079d4c 100644 --- a/common/org.eclipse.tracecompass.common.core/src/org/eclipse/tracecompass/common/core/process/ProcessUtils.java +++ b/common/org.eclipse.tracecompass.common.core/src/org/eclipse/tracecompass/common/core/process/ProcessUtils.java @@ -26,6 +26,8 @@ import org.eclipse.core.runtime.Status; import org.eclipse.jdt.annotation.Nullable; import org.eclipse.tracecompass.internal.common.core.Activator; +import com.google.common.base.Charsets; + /** * Common utility methods for launching external processes and retrieving their * output. @@ -35,6 +37,8 @@ import org.eclipse.tracecompass.internal.common.core.Activator; */ public final class ProcessUtils { + private static final int PROGRESS_DURATION = 1000; + private ProcessUtils() {} /** @@ -51,7 +55,7 @@ public final class ProcessUtils { builder.redirectErrorStream(true); Process p = builder.start(); - try (BufferedReader br = new BufferedReader(new InputStreamReader(p.getInputStream()));) { + try (BufferedReader br = new BufferedReader(new InputStreamReader(p.getInputStream(), Charsets.UTF_8));) { List output = new LinkedList<>(); /* @@ -78,7 +82,7 @@ public final class ProcessUtils { * {@link #getOutputFromCommandCancellable}. */ @FunctionalInterface - public static interface OutputReaderFunction { + public interface OutputReaderFunction { /** * Handle the output of the process. This can include reporting progress @@ -132,7 +136,7 @@ public final class ProcessUtils { Thread cancellerThread = null; try { - monitor.beginTask(mainTaskName, 1000); + monitor.beginTask(mainTaskName, PROGRESS_DURATION); ProcessBuilder builder = new ProcessBuilder(command); builder.redirectErrorStream(false); @@ -143,7 +147,7 @@ public final class ProcessUtils { cancellerThread = new Thread(cancellerRunnable); cancellerThread.start(); - try (BufferedReader stdoutReader = new BufferedReader(new InputStreamReader(p.getInputStream()));) { + try (BufferedReader stdoutReader = new BufferedReader(new InputStreamReader(p.getInputStream(), Charsets.UTF_8));) { List lines = readerFunction.readOutput(stdoutReader, monitor); @@ -170,8 +174,8 @@ public final class ProcessUtils { } if (stderrOutput.isEmpty()) { /* - * At least say "no output", so an error message actually - * shows up. + * At least say "no output", so an error message + * actually shows up. */ status.add(new Status(IStatus.ERROR, Activator.instance().getPluginId(), Messages.ProcessUtils_ErrorNoOutput)); } @@ -193,6 +197,9 @@ public final class ProcessUtils { try { cancellerThread.join(); } catch (InterruptedException e) { + /* + * If it is interrupted, process is terminated. + */ } } @@ -206,6 +213,7 @@ public final class ProcessUtils { */ private static class CancellableRunnable implements Runnable { + private static final int SLEEP_DURATION = 500; private final Process fProcess; private final IProgressMonitor fMonitor; @@ -224,13 +232,16 @@ public final class ProcessUtils { public void run() { try { while (!fIsFinished) { - Thread.sleep(500); + Thread.sleep(SLEEP_DURATION); if (fMonitor.isCanceled()) { fProcess.destroy(); return; } } } catch (InterruptedException e) { + /* + * If it is interrupted, process is terminated. + */ } } -- 2.34.1