fix: Allocation for emulator reuse

Author: d4vidi

detox/src/devices/allocation/drivers/android/FreeDeviceFinder.js

@@ -1,3 +1,8 @@
+/**
+ * @typedef {import('../../../common/drivers/android/tools/DeviceHandle')} DeviceHandle
+ * @typedef {import('../../../common/drivers/android/tools/EmulatorHandle')} EmulatorHandle
+ */
+
 const log = require('../../../../utils/logger').child({ cat: 'device' });
 
 const DEVICE_LOOKUP = { event: 'DEVICE_LOOKUP' };
@@ -12,12 +17,17 @@ class FreeDeviceFinder {
     this.deviceRegistry = deviceRegistry;
   }
 
-  async findFreeDevice(deviceQuery) {
-    const { devices } = await this.adb.devices();
+  /**
+   * @param {DeviceHandle[]} candidates
+   * @param {string} deviceQuery
+   * @returns {Promise<import('../../../common/drivers/android/tools/EmulatorHandle') | null>}
+   */
+  async findFreeDevice(candidates, deviceQuery) {
     const takenDevices = this.deviceRegistry.getTakenDevicesSync();
-    for (const candidate of devices) {
+    for (const candidate of candidates) {
       if (await this._isDeviceFreeAndMatching(takenDevices, candidate, deviceQuery)) {
-        return candidate.adbName;
+        // @ts-ignore
+        return candidate;
       }
     }
     return null;

detox/src/devices/allocation/drivers/android/FreeDeviceFinder.test.js

@@ -4,8 +4,6 @@ const FreeDeviceFinder = require('./FreeDeviceFinder');
 const { deviceOffline, emulator5556, ip5557, localhost5555 } = require('./__mocks__/handles');
 
 describe('FreeDeviceFinder', () => {
-  const mockAdb = { devices: jest.fn() };
-
   /** @type {DeviceList} */
   let fakeDeviceList;
   let mockDeviceRegistry;
@@ -17,42 +15,35 @@ describe('FreeDeviceFinder', () => {
     mockDeviceRegistry = new DeviceRegistry();
     mockDeviceRegistry.getTakenDevicesSync.mockImplementation(() => fakeDeviceList);
 
+    const mockAdb = /** @type {any} */ ({});
     uut = new FreeDeviceFinder(mockAdb, mockDeviceRegistry);
   });
 
   it('should return the only device when it matches, is online and not already taken by other workers', async () => {
-    mockAdbDevices([emulator5556]);
-
-    const result = await uut.findFreeDevice(emulator5556.adbName);
-    expect(result).toEqual(emulator5556.adbName);
+    const result = await uut.findFreeDevice([emulator5556], emulator5556.adbName);
+    expect(result).toEqual(emulator5556);
   });
 
   it('should return null when there are no devices', async () => {
-    mockAdbDevices([]);
-
-    const result = await uut.findFreeDevice(emulator5556.adbName);
+    const result = await uut.findFreeDevice([], emulator5556.adbName);
     expect(result).toEqual(null);
   });
 
   it('should return null when device is already taken by other workers', async () => {
-    mockAdbDevices([emulator5556]);
     mockAllDevicesTaken();
 
-    expect(await uut.findFreeDevice(emulator5556.adbName)).toEqual(null);
+    expect(await uut.findFreeDevice([emulator5556], emulator5556.adbName)).toEqual(null);
   });
 
   it('should return null when device is offline', async () => {
-    mockAdbDevices([deviceOffline]);
-    expect(await uut.findFreeDevice(deviceOffline.adbName)).toEqual(null);
+    expect(await uut.findFreeDevice([deviceOffline], deviceOffline.adbName)).toEqual(null);
   });
 
   it('should return first device that matches a regular expression', async () => {
-    mockAdbDevices([emulator5556, localhost5555, ip5557]);
     const localhost = '^localhost:\\d+$';
-    expect(await uut.findFreeDevice(localhost)).toBe(localhost5555.adbName);
+    expect(await uut.findFreeDevice([emulator5556, localhost5555, ip5557], localhost)).toBe(localhost5555);
   });
 
-  const mockAdbDevices = (devices) => mockAdb.devices.mockResolvedValue({ devices });
   const mockAllDevicesTaken = () => {
     fakeDeviceList.add(emulator5556.adbName, { busy: true });
     fakeDeviceList.add(localhost5555.adbName, { busy: true });

detox/src/devices/allocation/drivers/android/attached/AttachedAndroidAllocDriver.js

@@ -27,7 +27,8 @@ class AttachedAndroidAllocDriver extends AndroidAllocDriver {
    */
   async allocate(deviceConfig) {
     const adbNamePattern = deviceConfig.device.adbName;
-    const adbName = await this._deviceRegistry.registerDevice(() => this._freeDeviceFinder.findFreeDevice(adbNamePattern));
+    const adbName = await this._deviceRegistry.registerDevice(async () =>
+      (await this._freeDeviceFinder.findFreeDevice(adbNamePattern)).adbName);
 
     return { id: adbName, adbName };
   }

detox/src/devices/allocation/drivers/android/emulator/EmulatorAllocDriver.js

@@ -1,11 +1,13 @@
 /**
  * @typedef {import('../../../../common/drivers/android/cookies').AndroidDeviceCookie} AndroidDeviceCookie
  * @typedef {import('../../../../common/drivers/android/cookies').EmulatorDeviceCookie} EmulatorDeviceCookie
+ * @typedef {import('../../../../common/drivers/android/tools/DeviceHandle')} DeviceHandle
  */
 
 const _ = require('lodash');
 
 const log = require('../../../../../utils/logger').child({ cat: 'device,device-allocation' });
+const { isPortTaken } = require('../../../../../utils/netUtils');
 const adbPortRegistry = require('../../../../common/drivers/android/AdbPortRegistry');
 const AndroidAllocDriver = require('../AndroidAllocDriver');
 
@@ -42,7 +44,6 @@ class EmulatorAllocDriver extends AndroidAllocDriver {
     this._freePortFinder = freePortFinder;
     this._shouldShutdown = detoxConfig.behavior.cleanup.shutdownDevice;
     this._fixAvdConfigIniSkinNameIfNeeded = _.memoize(this._fixAvdConfigIniSkinNameIfNeeded.bind(this));
-    this._nextAdbServerPort = adb.baseServerPort + 1;
   }
 
   async init() {
@@ -63,26 +64,34 @@ class EmulatorAllocDriver extends AndroidAllocDriver {
     let adbName;
 
     await this._deviceRegistry.registerDevice(async () => {
-      adbName = await this._freeDeviceFinder.findFreeDevice(avdName);
+      const candidates = await this._getAllDevices();
+      const device = await this._freeDeviceFinder.findFreeDevice(candidates, avdName);
 
-      if (adbName) {
-        adbServerPort = adbPortRegistry.getPort(adbName);
+      if (device) {
+        adbName = device.adbName;
+        adbServerPort = device.adbServerPort;
+        adbPortRegistry.register(adbName, adbServerPort);
       } else {
         const port = await this._freePortFinder.findFreePort();
 
         adbName = `emulator-${port}`;
-        adbServerPort = this._nextAdbServerPort++;
-
-        await this._emulatorLauncher.launch({
-          bootArgs: deviceConfig.bootArgs,
-          gpuMode: deviceConfig.gpuMode,
-          headless: deviceConfig.headless,
-          readonly: deviceConfig.readonly,
-          avdName,
-          adbName,
-          port,
-          adbServerPort,
-        });
+        adbServerPort = this._getFreeAdbServerPort(candidates);
+        adbPortRegistry.register(adbName, adbServerPort);
+
+        try {
+          await this._emulatorLauncher.launch({
+            bootArgs: deviceConfig.bootArgs,
+            gpuMode: deviceConfig.gpuMode,
+            headless: deviceConfig.headless,
+            readonly: deviceConfig.readonly,
+            avdName,
+            adbName,
+            port,
+            adbServerPort,
+          });
+        } catch (e) {
+          adbPortRegistry.unregister(adbName);
+        }
       }
 
       return adbName;
@@ -131,7 +140,7 @@ class EmulatorAllocDriver extends AndroidAllocDriver {
 
   async cleanup() {
     if (this._shouldShutdown) {
-      const { devices } = await this._adb.devices();
+      const devices = await this._getAllDevices();
       const actualEmulators = devices.map((device) => device.adbName);
       const sessionDevices = await this._deviceRegistry.readSessionDevices();
       const emulatorsToShutdown = _.intersection(sessionDevices.getIds(), actualEmulators);
@@ -159,6 +168,32 @@ class EmulatorAllocDriver extends AndroidAllocDriver {
     const binaryVersion = _.get(rawBinaryVersion, 'major');
     return await patchAvdSkinConfig(avdName, binaryVersion);
   }
+
+  /**
+   * @returns {Promise<DeviceHandle[]>}
+   * @private
+   */
+  async _getAllDevices() {
+    const adbServers = await this._getRunningAdbServers();
+    return (await this._adb.devices({}, adbServers)).devices;
+  }
+
+  /**
+   * @returns {Promise<number[]>}
+   * @private
+   */
+  async _getRunningAdbServers() {
+    const ports = [];
+    for (let port = this._adb.defaultServerPort + 1; await isPortTaken(port); port++) {
+      ports.push(port);
+    }
+    return ports;
+  }
+
+  _getFreeAdbServerPort(currentDevices) {
+    const maxPortDevice = _.maxBy(currentDevices, 'adbServerPort');
+    return _.get(maxPortDevice, 'adbServerPort', this._adb.defaultServerPort) + 1;
+  }
 }
 
 module.exports = EmulatorAllocDriver;

detox/src/devices/allocation/drivers/android/emulator/FreeEmulatorFinder.test.js

@@ -2,8 +2,6 @@ const DeviceList = require('../../../DeviceList');
 const { emulator5556, localhost5555, mockAvdName } = require('../__mocks__/handles');
 
 describe('FreeEmulatorFinder', () => {
-  const mockAdb = { devices: jest.fn() };
-
   /** @type {DeviceList} */
   let fakeDeviceList;
   /** @type {jest.Mocked<import('../../../DeviceRegistry')>} */
@@ -18,24 +16,20 @@ describe('FreeEmulatorFinder', () => {
     mockDeviceRegistry.getTakenDevicesSync.mockImplementation(() => fakeDeviceList);
 
     const FreeEmulatorFinder = require('./FreeEmulatorFinder');
+    const mockAdb = /** @type {any} */ ({});
     uut = new FreeEmulatorFinder(mockAdb, mockDeviceRegistry);
   });
 
   it('should return device when it is an emulator and avdName matches', async () => {
-    mockAdbDevices([emulator5556]);
-    const result = await uut.findFreeDevice(mockAvdName);
-    expect(result).toBe(emulator5556.adbName);
+    const result = await uut.findFreeDevice([emulator5556], mockAvdName);
+    expect(result).toBe(emulator5556);
   });
 
   it('should return null when avdName does not match', async () => {
-    mockAdbDevices([emulator5556]);
-    expect(await uut.findFreeDevice('wrongAvdName')).toBe(null);
+    expect(await uut.findFreeDevice([emulator5556], 'wrongAvdName')).toBe(null);
   });
 
   it('should return null when not an emulator', async () => {
-    mockAdbDevices([localhost5555]);
-    expect(await uut.findFreeDevice(mockAvdName)).toBe(null);
+    expect(await uut.findFreeDevice([localhost5555], mockAvdName)).toBe(null);
   });
-
-  const mockAdbDevices = (devices) => mockAdb.devices.mockResolvedValue({ devices });
 });

detox/src/devices/allocation/drivers/android/emulator/FreePortFinder.js

@@ -1,4 +1,4 @@
-const net = require('net');
+const { isPortTaken } = require('../../../../../utils/netUtils');
 
 class FreePortFinder {
   constructor({ min = 10000, max = 20000 } = {}) {
@@ -14,24 +14,10 @@ class FreePortFinder {
       const max = this._max;
       port = Math.random() * (max - min) + min;
       port = port & 0xFFFFFFFE; // Should always be even
-    } while (await this.isPortTaken(port));
+    } while (await isPortTaken(port));
 
     return port;
   }
-
-  async isPortTaken(port) {
-    return new Promise((resolve, reject) => {
-      const tester = net.createServer()
-        .once('error', /** @param {*} err */ (err) => {
-          /* istanbul ignore next */
-          return err && err.code === 'EADDRINUSE' ? resolve(true) : reject(err);
-        })
-        .once('listening', () => {
-          tester.once('close', () => resolve(false)).close();
-        })
-        .listen(port);
-    });
-  }
 }
 
 module.exports = FreePortFinder;

detox/src/devices/allocation/drivers/android/emulator/FreePortFinder.test.js

@@ -1,39 +1,21 @@
-const net = require('net');
-
-const FreePortFinder = require('./FreePortFinder');
-
 describe('FreePortFinder', () => {
   let finder;
-  let server;
+  let isPortTaken;
 
   beforeEach(() => {
-    finder = new FreePortFinder();
-  });
+    jest.mock('../../../../../utils/netUtils');
+    isPortTaken = require('../../../../../utils/netUtils').isPortTaken;
+    isPortTaken.mockResolvedValue(false);
 
-  afterEach(done => {
-    if (server) {
-      server.close(done);
-    } else {
-      done();
-    }
+    const FreePortFinder = require('./FreePortFinder');
+    finder = new FreePortFinder();
   });
 
   test('should find a free port', async () => {
     const port = await finder.findFreePort();
     expect(port).toBeGreaterThanOrEqual(10000);
     expect(port).toBeLessThanOrEqual(20000);
     expect(port % 2).toBe(0);
-    await expect(finder.isPortTaken(port)).resolves.toBe(false);
-  });
-
-  test('should identify a taken port', async () => {
-    server = net.createServer();
-    const portTaken = await new Promise(resolve => {
-      server.listen(0, () => { // 0 means random available port
-        resolve(server.address().port);
-      });
-    });
-
-    await expect(finder.isPortTaken(portTaken)).resolves.toBe(true);
+    expect(isPortTaken).toHaveBeenCalled();
   });
 });

detox/src/devices/allocation/drivers/android/emulator/launchEmulatorProcess.js

@@ -9,7 +9,6 @@ const fs = require('fs');
 const _ = require('lodash');
 
 const unitLogger = require('../../../../../utils/logger').child({ cat: 'device' });
-const adbPortRegistry = require('../../../../common/drivers/android/AdbPortRegistry');
 
 /**
  * @param { EmulatorExec } emulatorExec - Instance for executing emulator commands
@@ -48,10 +47,6 @@ function launchEmulatorProcess(emulatorExec, adb, emulatorLaunchCommand, adbServ
   });
   childProcessPromise.childProcess.unref();
 
-  if (adbServerPort) {
-    adbPortRegistry.register(emulatorLaunchCommand.adbName, adbServerPort);
-  }
-
   log = log.child({ child_pid: childProcessPromise.childProcess.pid });
 
   // Create a deferred promise that resolves when the device is ready

detox/src/devices/common/drivers/android/AdbPortRegistry.js

@@ -13,17 +13,17 @@ class AdbPortRegistry {
 
   /**
    * @param { string } adbName
-   * @returns { number | undefined }
    */
-  getPort(adbName) {
-    return this._registry.get(adbName);
+  unregister(adbName) {
+    this._registry.delete(adbName);
   }
 
   /**
-   * @returns { number[] }
+   * @param { string } adbName
+   * @returns { number | undefined }
    */
-  getAllPorts() {
-    return this._registry.values().toArray();
+  getPort(adbName) {
+    return this._registry.get(adbName);
   }
 }