Provides better error reporting through the WaitableExecutor.

Creates a new exception for command line errors, and make sure this
gets reported from any methods that calls it.

Also fixes issues in the waitable executor around canceling tasks
and returning the original exception instead of returning it
wrapped in the ExecutionException.

Change-Id: Ib8bc40a50274e4be251b4ad29f0e7512ffc3459b
(cherry picked from commit 5a3de6461e135ce13c713fe2368aa7f179067125)

5 files changed, 135 insertions, 49 deletions

diff --git a/sdk-common/src/main/java/com/android/ide/common/internal/AaptRunner.java b/sdk-common/src/main/java/com/android/ide/common/internal/AaptRunner.java
index 848c35d943..ea9afdf95d 100644
--- a/sdk-common/src/main/java/com/android/ide/common/internal/AaptRunner.java
+++ b/sdk-common/src/main/java/com/android/ide/common/internal/AaptRunner.java
@@ -38,8 +38,10 @@ public class AaptRunner {
      * @param to the output file
      * @throws IOException
      * @throws InterruptedException
+     * @throws LoggedErrorException
      */
-    public void crunchPng(File from, File to) throws IOException, InterruptedException {
+    public void crunchPng(File from, File to)
+            throws IOException, InterruptedException, LoggedErrorException {
         String[] command = new String[] {
             mAaptLocation,
             "s",
diff --git a/sdk-common/src/main/java/com/android/ide/common/internal/CommandLineRunner.java b/sdk-common/src/main/java/com/android/ide/common/internal/CommandLineRunner.java
index 906ddc0fae..902e65ffaf 100644
--- a/sdk-common/src/main/java/com/android/ide/common/internal/CommandLineRunner.java
+++ b/sdk-common/src/main/java/com/android/ide/common/internal/CommandLineRunner.java
@@ -20,6 +20,7 @@ import com.android.annotations.Nullable;
 import com.android.sdklib.util.GrabProcessOutput;
 import com.android.utils.ILogger;
 import com.google.common.base.Joiner;
+import com.google.common.collect.Lists;
 
 import java.io.IOException;
 import java.util.List;
@@ -31,6 +32,7 @@ public class CommandLineRunner {
     private class OutputGrabber implements GrabProcessOutput.IProcessOutput {
 
         private boolean mFoundError = false;
+        private List<String> mErrors = Lists.newArrayList();
 
         @Override
         public void out(@Nullable String line) {
@@ -43,6 +45,7 @@ public class CommandLineRunner {
         public void err(@Nullable String line) {
             if (line != null) {
                 mLogger.error(null /*throwable*/, line);
+                mErrors.add(line);
                 mFoundError = true;
             }
         }
@@ -56,37 +59,33 @@ public class CommandLineRunner {
         mLogger = logger;
     }
 
-    public void runCmdLine(List<String> command) throws IOException, InterruptedException {
+    public void runCmdLine(List<String> command)
+            throws IOException, InterruptedException, LoggedErrorException {
         String[] cmdArray = command.toArray(new String[command.size()]);
         runCmdLine(cmdArray);
     }
 
-    public void runCmdLine(String[] command) throws IOException, InterruptedException {
+    public void runCmdLine(String[] command)
+            throws IOException, InterruptedException, LoggedErrorException {
         printCommand(command);
 
         // launch the command line process
         Process process = Runtime.getRuntime().exec(command);
 
         // get the output and return code from the process
-        if (grabProcessOutput(process) != 0) {
-            throw new RuntimeException(String.format("Running %s failed. See output", command[0]));
-        }
-    }
-
-    /**
-     * Get the stderr output of a process and return when the process is done.
-     * @param process The process to get the output from
-     * @return the process return code.
-     * @throws InterruptedException
-     */
-    private int grabProcessOutput(final Process process) throws InterruptedException {
-
         OutputGrabber grabber = new OutputGrabber();
 
-        return GrabProcessOutput.grabProcessOutput(
+        int returnCode = GrabProcessOutput.grabProcessOutput(
                 process,
                 GrabProcessOutput.Wait.WAIT_FOR_READERS, // we really want to make sure we get all the output!
                 grabber);
+
+        if (returnCode != 0) {
+            throw new LoggedErrorException(
+                    returnCode,
+                    grabber.mErrors,
+                    Joiner.on(' ').join(command));
+        }
     }
 
     private void printCommand(String[] command) {
diff --git a/sdk-common/src/main/java/com/android/ide/common/internal/LoggedErrorException.java b/sdk-common/src/main/java/com/android/ide/common/internal/LoggedErrorException.java
new file mode 100644
index 0000000000..053e4f36d0
--- /dev/null
+++ b/sdk-common/src/main/java/com/android/ide/common/internal/LoggedErrorException.java
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2013 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.ide.common.internal;
+
+import com.android.annotations.NonNull;
+import com.android.annotations.Nullable;
+
+import java.util.List;
+
+/**
+ */
+public class LoggedErrorException extends Exception {
+
+    private final int mCmdLineError;
+    private final List<String> mOutput;
+    private final String mCmdLine;
+
+    public LoggedErrorException(
+                      int error,
+            @NonNull  List<String> output,
+            @Nullable String cmdLine) {
+        mCmdLineError = error;
+        mOutput = output;
+        mCmdLine = cmdLine;
+    }
+
+    public LoggedErrorException(@NonNull List<String> output) {
+        this(0, output, null);
+    }
+
+    public int getCmdLineError() {
+        return mCmdLineError;
+    }
+
+    @NonNull
+    public List<String> getOutput() {
+        return mOutput;
+    }
+
+    public String getCmdLine() {
+        return mCmdLine;
+    }
+
+    @Override
+    public String getMessage() {
+        StringBuilder sb = new StringBuilder();
+        sb.append("Failed to run command:\n\t").append(mCmdLine).append('\n');
+        sb.append("Error Code:\n\t").append(mCmdLineError).append('\n');
+        if (!mOutput.isEmpty()) {
+            sb.append("Output:\n");
+            for (String line : mOutput) {
+                sb.append('\t').append(line).append('\n');
+            }
+        }
+
+        return sb.toString();
+    }
+}
diff --git a/sdk-common/src/main/java/com/android/ide/common/internal/WaitableExecutor.java b/sdk-common/src/main/java/com/android/ide/common/internal/WaitableExecutor.java
index 1b414a4af3..b2cdd199cc 100644
--- a/sdk-common/src/main/java/com/android/ide/common/internal/WaitableExecutor.java
+++ b/sdk-common/src/main/java/com/android/ide/common/internal/WaitableExecutor.java
@@ -16,8 +16,10 @@
 package com.android.ide.common.internal;
 
 import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CompletionService;
 import java.util.concurrent.ExecutionException;
@@ -32,15 +34,14 @@ import java.util.concurrent.Future;
  * Tasks are submitted as {@link Callable} with {@link #execute(java.util.concurrent.Callable)}.
  *
  * After executing all tasks, it is possible to wait on them with
- * {@link #waitForTasksWithQuickFail()}, or {@link #waitForAllTasks()}.
+ * {@link #waitForTasksWithQuickFail(boolean)}, or {@link #waitForAllTasks()}.
  *
  * This class is not Thread safe!
  */
 public class WaitableExecutor<T> {
 
     private final CompletionService<T> mCompletionService;
-
-    private int mCount = 0;
+    private final Set<Future<T>> mFutureSet = Sets.newHashSet();
 
     public WaitableExecutor(int nThreads) {
         if (nThreads < 1) {
@@ -62,8 +63,7 @@ public class WaitableExecutor<T> {
      * @param runnable the callable to run.
      */
     public void execute(Callable<T> runnable) {
-        mCompletionService.submit(runnable);
-        mCount++;
+        mFutureSet.add(mCompletionService.submit(runnable));
     }
 
     /**
@@ -74,17 +74,38 @@ public class WaitableExecutor<T> {
      * If you want to get the results of all tasks (result and/or exception), use
      * {@link #waitForAllTasks()}
      *
+     * @param cancelRemaining if true, and a task fails, cancel all remaining tasks.
+     *
      * @return a list of all the return values from the tasks.
      *
      * @throws InterruptedException if this thread was interrupted. Not if the tasks were interrupted.
-     * @throws ExecutionException if a task fails with an interruption. The cause if the original exception.
      */
-    public List<T> waitForTasksWithQuickFail() throws InterruptedException, ExecutionException {
-        List<T> results = Lists.newArrayListWithCapacity(mCount);
-        for (int i = 0 ; i < mCount ; i++) {
-            Future<T> result = mCompletionService.take();
-            // Get the result from the task. If the task threw an exception just throw it too.
-            results.add(result.get());
+    public List<T> waitForTasksWithQuickFail(boolean cancelRemaining) throws InterruptedException,
+            LoggedErrorException {
+        List<T> results = Lists.newArrayListWithCapacity(mFutureSet.size());
+        try {
+            while (!mFutureSet.isEmpty()) {
+                Future<T> future = mCompletionService.take();
+
+                assert mFutureSet.contains(future);
+                mFutureSet.remove(future);
+
+                // Get the result from the task. If the task threw an exception,
+                // this will throw it, wrapped in an ExecutionException, caught below.
+                results.add(future.get());
+            }
+        } catch (ExecutionException e) {
+            if (cancelRemaining) {
+                cancelAllTasks();
+            }
+
+            // get the original exception adn throw that one.
+            Throwable cause = e.getCause();
+            if (cause instanceof LoggedErrorException) {
+                throw (LoggedErrorException) cause;
+            } else {
+                throw new RuntimeException(cause);
+            }
         }
 
         return results;
@@ -117,12 +138,16 @@ public class WaitableExecutor<T> {
      * @throws InterruptedException if this thread was interrupted. Not if the tasks were interrupted.
      */
     public List<TaskResult<T>> waitForAllTasks() throws InterruptedException {
-        List<TaskResult<T>> results = Lists.newArrayListWithCapacity(mCount);
-        for (int i = 0 ; i < mCount ; i++) {
-            Future<T> task = mCompletionService.take();
+        List<TaskResult<T>> results = Lists.newArrayListWithCapacity(mFutureSet.size());
+        while (!mFutureSet.isEmpty()) {
+            Future<T> future = mCompletionService.take();
+
+            assert mFutureSet.contains(future);
+            mFutureSet.remove(future);
+
             // Get the result from the task.
             try {
-                results.add(TaskResult.withValue(task.get()));
+                results.add(TaskResult.withValue(future.get()));
             } catch (ExecutionException e) {
                 // the original exception thrown by the task is the cause of this one.
                 Throwable cause = e.getCause();
@@ -144,13 +169,8 @@ public class WaitableExecutor<T> {
      * Cancel all remaining tasks.
      */
     public void cancelAllTasks() {
-        while (true) {
-            Future<T> task = mCompletionService.poll();
-            if (task != null) {
-                task.cancel(true /*mayInterruptIfRunning*/);
-            } else {
-                break;
-            }
+        for (Future<T> future : mFutureSet) {
+            future.cancel(true /*mayInterruptIfRunning*/);
         }
     }
 }
diff --git a/sdk-common/src/main/java/com/android/ide/common/res2/MergeWriter.java b/sdk-common/src/main/java/com/android/ide/common/res2/MergeWriter.java
index 2b7ba6b9be..0b7cca815b 100644
--- a/sdk-common/src/main/java/com/android/ide/common/res2/MergeWriter.java
+++ b/sdk-common/src/main/java/com/android/ide/common/res2/MergeWriter.java
@@ -46,16 +46,9 @@ public abstract class MergeWriter<I extends DataItem> implements MergeConsumer<I
         try {
             postWriteAction();
 
-            getExecutor().waitForTasksWithQuickFail();
-        } catch (InterruptedException e) {
-            // if this thread was cancelled we need to cancel the rest of the executor tasks.
-            getExecutor().cancelAllTasks();
+            getExecutor().waitForTasksWithQuickFail(true);
+        } catch (Exception e) {
             throw new ConsumerException(e);
-        } catch (ExecutionException e) {
-            // if a task fail, we also want to cancel the rest of the tasks.
-            mExecutor.cancelAllTasks();
-            // and return the first error
-            throw new ConsumerException(e.getCause());
         }
     }