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 <matthew.khouzam@ericsson.com>
Reviewed-on: https://git.eclipse.org/r/88407
Reviewed-by: Hudson CI
Reviewed-by: Alexandre Montplaisir <alexmonthy@efficios.com>
Tested-by: Alexandre Montplaisir <alexmonthy@efficios.com>

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 afb10aff6dc0d6759b27eb8da6a5f3f4619f8697..e37c2c729f846713a710090beca3d26a9cc71dd6 100644 (file)
--- 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 479988c68f56f698b6b3f94b8c790421243a8b09..1338079d4c058ecac0170e18bc6bd8b25e7d9b74 100644 (file)
--- 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<String> 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<String> 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.
+                 */
             }
         }