SDK Manager: Prevent 'adb start-server' from blocking

SDK Manager hangs on windows after it tries to stop
and restart ADB (e.g. when installing platform-tools).

It hangs when capturing the stdout/stderr pipes -- these
don't close automatically when the process has finished
and these are not interruptible streams.
One workaround is to not capture them, especially since
the output isn't really useful for the installer anyway.

Change-Id: I6554461dfffad2cc8ff0f1fe7d212fdee742e2e6

diff --git a/sdkmanager/libs/sdklib/src/com/android/sdklib/internal/repository/AdbWrapper.java b/sdkmanager/libs/sdklib/src/com/android/sdklib/internal/repository/AdbWrapper.java
index ba2d501..eb6a411 100755 (executable)
--- a/sdkmanager/libs/sdklib/src/com/android/sdklib/internal/repository/AdbWrapper.java
+++ b/sdkmanager/libs/sdklib/src/com/android/sdklib/internal/repository/AdbWrapper.java
@@ -18,14 +18,12 @@ package com.android.sdklib.internal.repository;
 \r
 import com.android.sdklib.SdkConstants;\r
 \r
-import java.io.BufferedReader;\r
 import java.io.File;\r
 import java.io.IOException;\r
-import java.io.InputStreamReader;\r
-import java.util.ArrayList;\r
 \r
 /**\r
  * A lightweight wrapper to start & stop ADB.\r
+ * This is <b>specific</b> to the SDK Manager install process.\r
  */\r
 public class AdbWrapper {\r
 \r
@@ -75,15 +73,17 @@ public class AdbWrapper {
         int status = -1;\r
 \r
         try {\r
-            String[] command = new String[2];\r
-            command[0] = mAdbOsLocation;\r
-            command[1] = "start-server"; //$NON-NLS-1$\r
-            proc = Runtime.getRuntime().exec(command);\r
+            ProcessBuilder processBuilder = new ProcessBuilder(\r
+                    mAdbOsLocation,\r
+                    "start-server"); //$NON-NLS-1$\r
+            proc = processBuilder.start();\r
+            status = proc.waitFor();\r
 \r
-            ArrayList<String> errorOutput = new ArrayList<String>();\r
-            ArrayList<String> stdOutput = new ArrayList<String>();\r
-            status = grabProcessOutput(proc, errorOutput, stdOutput,\r
-                    false /* waitForReaders */);\r
+            // Implementation note: normally on Windows we need to capture stderr/stdout\r
+            // to make sure the process isn't blocked if it's output isn't read. However\r
+            // in this case this happens to hang when reading stdout with no proper way\r
+            // to properly close the streams. On the other hand the output from start\r
+            // server is rather short and not very interesting so we just drop it.\r
 \r
         } catch (IOException ioe) {\r
             displayError("Unable to run 'adb': %1$s.", ioe.getMessage()); //$NON-NLS-1$\r
@@ -94,11 +94,13 @@ public class AdbWrapper {
         }\r
 \r
         if (status != 0) {\r
-            displayError("'adb start-server' failed."); //$NON-NLS-1$\r
+            displayError(String.format(\r
+                    "Starting ADB server failed (code %d).", //$NON-NLS-1$\r
+                    status));\r
             return false;\r
         }\r
 \r
-        display("'adb start-server' succeeded."); //$NON-NLS-1$\r
+        display("Starting ADB server succeeded."); //$NON-NLS-1$\r
 \r
         return true;\r
     }\r
@@ -122,6 +124,8 @@ public class AdbWrapper {
             command[1] = "kill-server"; //$NON-NLS-1$\r
             proc = Runtime.getRuntime().exec(command);\r
             status = proc.waitFor();\r
+\r
+            // See comment in startAdb about not needing/wanting to capture stderr/stdout.\r
         }\r
         catch (IOException ioe) {\r
             // we'll return false;\r
@@ -130,96 +134,19 @@ public class AdbWrapper {
             // we'll return false;\r
         }\r
 \r
-        if (status != 0) {\r
-            displayError("'adb kill-server' failed -- run manually if necessary."); //$NON-NLS-1$\r
+        // adb kill-server returns:\r
+        // 0 if adb was running and was correctly killed.\r
+        // 1 if adb wasn't running and thus wasn't killed.\r
+        // This error case is not worth reporting.\r
+\r
+        if (status != 0 && status != 1) {\r
+            displayError(String.format(\r
+                    "Stopping ADB server failed (code %d).", //$NON-NLS-1$\r
+                    status));\r
             return false;\r
         }\r
 \r
-        display("'adb kill-server' succeeded."); //$NON-NLS-1$\r
+        display("Stopping ADB server succeeded."); //$NON-NLS-1$\r
         return true;\r
     }\r
-\r
-    /**\r
-     * Get the stderr/stdout outputs of a process and return when the process is done.\r
-     * Both <b>must</b> be read or the process will block on windows.\r
-     * @param process The process to get the ouput from\r
-     * @param errorOutput The array to store the stderr output. cannot be null.\r
-     * @param stdOutput The array to store the stdout output. cannot be null.\r
-     * @param waitforReaders if true, this will wait for the reader threads.\r
-     * @return the process return code.\r
-     * @throws InterruptedException\r
-     */\r
-    private int grabProcessOutput(final Process process, final ArrayList<String> errorOutput,\r
-            final ArrayList<String> stdOutput, boolean waitforReaders)\r
-            throws InterruptedException {\r
-        assert errorOutput != null;\r
-        assert stdOutput != null;\r
-        // read the lines as they come. if null is returned, it's\r
-        // because the process finished\r
-        Thread t1 = new Thread("") { //$NON-NLS-1$\r
-            @Override\r
-            public void run() {\r
-                // create a buffer to read the stderr output\r
-                InputStreamReader is = new InputStreamReader(process.getErrorStream());\r
-                BufferedReader errReader = new BufferedReader(is);\r
-\r
-                try {\r
-                    while (true) {\r
-                        String line = errReader.readLine();\r
-                        if (line != null) {\r
-                            displayError("ADB Error: %1$s", line);\r
-                            errorOutput.add(line);\r
-                        } else {\r
-                            break;\r
-                        }\r
-                    }\r
-                } catch (IOException e) {\r
-                    // do nothing.\r
-                }\r
-            }\r
-        };\r
-\r
-        Thread t2 = new Thread("") { //$NON-NLS-1$\r
-            @Override\r
-            public void run() {\r
-                InputStreamReader is = new InputStreamReader(process.getInputStream());\r
-                BufferedReader outReader = new BufferedReader(is);\r
-\r
-                try {\r
-                    while (true) {\r
-                        String line = outReader.readLine();\r
-                        if (line != null) {\r
-                            displayError("ADB: %1$s", line);\r
-                            stdOutput.add(line);\r
-                        } else {\r
-                            break;\r
-                        }\r
-                    }\r
-                } catch (IOException e) {\r
-                    // do nothing.\r
-                }\r
-            }\r
-        };\r
-\r
-        t1.start();\r
-        t2.start();\r
-\r
-        // it looks like on windows process#waitFor() can return\r
-        // before the thread have filled the arrays, so we wait for both threads and the\r
-        // process itself.\r
-        if (waitforReaders) {\r
-            try {\r
-                t1.join();\r
-            } catch (InterruptedException e) {\r
-            }\r
-            try {\r
-                t2.join();\r
-            } catch (InterruptedException e) {\r
-            }\r
-        }\r
-\r
-        // get the return code from the process\r
-        return process.waitFor();\r
-    }\r
-\r
 }\r

diff --git a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/ISettingsPage.java b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/ISettingsPage.java
index 739c479..7d730d6 100755 (executable)
--- a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/ISettingsPage.java
+++ b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/ISettingsPage.java
@@ -49,7 +49,7 @@ public interface ISettingsPage {
     /**\r
      * Setting to ask for permission before restarting ADB.\r
      * Type: Boolean.\r
-     * Default: True.\r
+     * Default: False.\r
      */\r
     public static final String KEY_ASK_ADB_RESTART = "sdkman.ask.adb.restart";   //$NON-NLS-1$\r
     /**\r

diff --git a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SettingsController.java b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SettingsController.java
index fde9b28..449e6e3 100755 (executable)
--- a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SettingsController.java
+++ b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SettingsController.java
@@ -68,11 +68,7 @@ public class SettingsController {
      * @see ISettingsPage#KEY_ASK_ADB_RESTART\r
      */\r
     public boolean getAskBeforeAdbRestart() {\r
-        String value = mProperties.getProperty(ISettingsPage.KEY_ASK_ADB_RESTART);\r
-        if (value == null) {\r
-            return true;\r
-        }\r
-        return Boolean.parseBoolean(value);\r
+        return Boolean.parseBoolean(mProperties.getProperty(ISettingsPage.KEY_ASK_ADB_RESTART));\r
     }\r
 \r
     /**\r