Bug 1469054 - Make sure the ADB server is ready to listen from clients when we call adb.start(). r?jdescottes draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Thu, 09 Aug 2018 15:43:39 +0900
changeset 827791 5711a5c7f5b30e2b0d0ee2ed6077b2032039a5e0
parent 827787 e8e8383858fd0af4f69d86355c4a0563d721451d
child 827792 f10bec1379b96d7487aee9572d1df1bcdcfa67d1
push id118584
push userhikezoe@mozilla.com
push dateThu, 09 Aug 2018 06:45:13 +0000
reviewersjdescottes
bugs1469054
milestone63.0a1
Bug 1469054 - Make sure the ADB server is ready to listen from clients when we call adb.start(). r?jdescottes There is a race condition that when we adb.start() finishes, i.e. the ADB server is launched, the ADB server is not ready to listen from clients yet. To avoid this race condition in adb.start() function, we do wait until the server gets ready. MozReview-Commit-ID: EfSLA9uvhI9
devtools/shared/adb/adb.js
--- a/devtools/shared/adb/adb.js
+++ b/devtools/shared/adb/adb.js
@@ -44,16 +44,48 @@ const ADB = {
   get adbFilePromise() {
     if (this._adbFilePromise) {
       return this._adbFilePromise;
     }
     this._adbFilePromise = getFileForBinary();
     return this._adbFilePromise;
   },
 
+  async _runProcess(process, params) {
+    return new Promise((resolve, reject) => {
+      process.runAsync(params, params.length, {
+        observe(subject, topic, data) {
+          switch (topic) {
+            case "process-finished":
+              resolve();
+              break;
+            case "process-failed":
+              reject();
+              break;
+          }
+        }
+      }, false);
+    });
+  },
+
+  // Waits until a predicate returns true or re-tries the predicate calls
+  // |retry| times, we wait for 100ms between each calls.
+  async _waitUntil(predicate, retry = 20) {
+    let count = 0;
+    while (count++ < retry) {
+      if (await predicate()) {
+        return true;
+      }
+      // Wait for 100 milliseconds.
+      await new Promise(resolve => setTimeout(resolve, 100));
+    }
+    // Timed out after trying too many times.
+    return false;
+  },
+
   // We startup by launching adb in server mode, and setting
   // the tcp socket preference to |true|
   async start() {
     return new Promise(async (resolve, reject) => {
       const onSuccessfulStart = () => {
         Services.obs.notifyObservers(null, "adb-ready");
         this.ready = true;
         resolve();
@@ -78,57 +110,51 @@ const ADB = {
       try {
         // startHidden is 55+
         process.startHidden = true;
         // noShell attribute is 58+
         process.noShell = true;
       } catch (e) {
       }
       const params = ["start-server"];
-      const self = this;
-      process.runAsync(params, params.length, {
-        observe(subject, topic, data) {
-          switch (topic) {
-            case "process-finished":
-              onSuccessfulStart();
-              break;
-            case "process-failed":
-              self.ready = false;
-              reject();
-              break;
-          }
-        }
-      }, false);
+      let isStarted = false;
+      try {
+        await this._runProcess(process, params);
+        isStarted = await this._waitUntil(check);
+      } catch (e) {
+      }
+
+      if (isStarted) {
+        onSuccessfulStart();
+      } else {
+        this.ready = false;
+        reject();
+      }
     });
   },
 
   /**
    * Stop the ADB server, but only if we started it.  If it was started before
    * us, we return immediately.
    *
    * @param boolean sync
    *        In case, we do need to kill the server, this param is passed through
    *        to kill to determine whether it's a sync operation.
    */
   async stop(sync) {
     if (!this.didRunInitially) {
       return; // We didn't start the server, nothing to do
     }
     await this.kill(sync);
-    // Make sure the ADB server stops listening.
-    //
-    // kill() above doesn't mean that the ADB server stops, it just means that
-    // 'adb kill-server' command finished, so that it's possible that the ADB
-    // server is still alive there.
-    while (true) {
-      const isAdbRunning = await check();
-      if (!isAdbRunning) {
-        break;
-      }
-    }
+    // Make sure the ADB server stops listening because kill() above doesn't
+    // mean that the ADB server stops, it means that 'adb kill-server' command
+    // just finished, so that it's possible that the ADB server is still alive.
+    await this._waitUntil(async () => {
+      return !await check();
+    });
   },
 
   /**
    * Kill the ADB server.  We do this by running ADB again, passing it
    * the "kill-server" argument.
    *
    * @param {Boolean} sync
    *        Whether or not to kill the server synchronously.  In general,