feat(android-setup): choose host port dynamically (#2933)

This PR uses the portfinder NPM package to have our service configurator dynamically choose a free host port to bind to, rather than unilaterally choosing the same port hardcoded into the service.
microsoft · Jun 23, 2020 · 23db22f · 23db22f
1 parent de5ef6a
commit 23db22f
Show file tree

Hide file tree

Showing 15 changed files with 388 additions and 140 deletions.
diff --git a/package.json b/package.json
@@ -151,6 +151,7 @@
         "lodash": "^4.17.15",
         "moment": "^2.27.0",
         "office-ui-fabric-react": "7.98.0",
+        "portfinder": "^1.0.26",
         "q": "1.5.1",
         "react": "^16.13.1",
         "react-copy-to-clipboard": "^5.0.2",

diff --git a/src/electron/flux/store/android-setup-store.ts b/src/electron/flux/store/android-setup-store.ts
@@ -28,6 +28,7 @@ export class AndroidSetupStore extends BaseStoreImpl<AndroidSetupStoreData> {
             stepTransition: this.stepTransition,
             setSelectedDevice: this.setSelectedDevice,
             setAvailableDevices: this.setAvailableDevices,
+            getScanPort: () => this.state.scanPort,
             setScanPort: this.setScanPort,
             setApplicationName: this.setApplicationName,
         });

diff --git a/src/electron/flux/types/android-setup-state-machine-types.ts b/src/electron/flux/types/android-setup-state-machine-types.ts
@@ -14,6 +14,7 @@ export type AndroidSetupStoreCallbacks = {
     stepTransition: AndroidSetupStepTransitionCallback;
     setSelectedDevice: (device: DeviceInfo) => void;
     setAvailableDevices: (devices: DeviceInfo[]) => void;
+    getScanPort: () => number | null;
     setScanPort: (scanPort?: number) => void;
     setApplicationName: (appName?: string) => void;
 };

diff --git a/src/electron/platform/android/android-service-configurator.ts b/src/electron/platform/android/android-service-configurator.ts
@@ -22,8 +22,8 @@ export interface AndroidServiceConfigurator {
     getPermissionInfo(deviceId: string): Promise<PermissionInfo>;
     installService(deviceId: string): Promise<void>;
     uninstallService(deviceId: string): Promise<void>;
-    setTcpForwarding(deviceId: string): Promise<number>;
-    removeTcpForwarding(deviceId: string): Promise<void>;
+    setupTcpForwarding(deviceId: string): Promise<number>;
+    removeTcpForwarding(deviceId: string, hostPort: number): Promise<void>;
 }
 
 export interface AndroidServiceConfiguratorFactory {

diff --git a/src/electron/platform/android/appium-service-configurator-factory.ts b/src/electron/platform/android/appium-service-configurator-factory.ts
@@ -11,12 +11,16 @@ import {
     AppiumAdbCreateParameters,
     AppiumAdbCreator,
 } from 'electron/platform/android/appium-adb-creator';
-import { AppiumServiceConfigurator } from 'electron/platform/android/appium-service-configurator';
+import {
+    AppiumServiceConfigurator,
+    PortFinder,
+} from 'electron/platform/android/appium-service-configurator';
 
 export class AppiumServiceConfiguratorFactory implements AndroidServiceConfiguratorFactory {
     public constructor(
         private readonly adbCreator: AppiumAdbCreator,
         private readonly apkLocator: AndroidServiceApkLocator,
+        private readonly portFinder: PortFinder,
     ) {}
 
     public getServiceConfigurator = async (
@@ -30,6 +34,6 @@ export class AppiumServiceConfiguratorFactory implements AndroidServiceConfigura
 
         const adb: ADB = await this.adbCreator.createADB(parameters);
 
-        return new AppiumServiceConfigurator(adb, this.apkLocator);
+        return new AppiumServiceConfigurator(adb, this.apkLocator, this.portFinder);
     };
 }
diff --git a/src/electron/platform/android/appium-service-configurator.ts b/src/electron/platform/android/appium-service-configurator.ts
@@ -9,17 +9,24 @@ import {
     PackageInfo,
     PermissionInfo,
 } from 'electron/platform/android/android-service-configurator';
+import { PortFinderOptions } from 'portfinder';
 import { DictionaryStringTo } from 'types/common-types';
 
 type AdbDevice = {
     udid: string;
 };
 
+export type PortFinder = (options?: PortFinderOptions) => Promise<number>;
+
 const servicePackageName: string = 'com.microsoft.accessibilityinsightsforandroidservice';
+export const servicePortNumber: number = 62442; // hardcoded in service APK
 
-export const defaultAdbPortNumber: number = 62442;
 export class AppiumServiceConfigurator implements AndroidServiceConfigurator {
-    constructor(private readonly adb: ADB, private readonly apkLocator: AndroidServiceApkLocator) {}
+    constructor(
+        private readonly adb: ADB,
+        private readonly apkLocator: AndroidServiceApkLocator,
+        private readonly portFinder: PortFinder,
+    ) {}
 
     public getConnectedDevices = async (): Promise<Array<DeviceInfo>> => {
         const detectedDevices: DictionaryStringTo<DeviceInfo> = {};
@@ -82,14 +89,19 @@ export class AppiumServiceConfigurator implements AndroidServiceConfigurator {
         await this.adb.uninstallApk(servicePackageName);
     };
 
-    public setTcpForwarding = async (deviceId: string): Promise<number> => {
+    public setupTcpForwarding = async (deviceId: string): Promise<number> => {
+        const hostPort = await this.portFinder({
+            port: servicePortNumber,
+            stopPort: servicePortNumber + 100,
+        });
+
         this.adb.setDeviceId(deviceId);
-        await this.adb.forwardPort(defaultAdbPortNumber, defaultAdbPortNumber);
-        return defaultAdbPortNumber;
+        await this.adb.forwardPort(hostPort, servicePortNumber);
+        return hostPort;
     };
 
-    public removeTcpForwarding = async (deviceId: string): Promise<void> => {
+    public removeTcpForwarding = async (deviceId: string, hostPort: number): Promise<void> => {
         this.adb.setDeviceId(deviceId);
-        await this.adb.removePortForward(defaultAdbPortNumber);
+        await this.adb.removePortForward(hostPort);
     };
 }
diff --git a/src/electron/platform/android/setup/android-setup-deps.ts b/src/electron/platform/android/setup/android-setup-deps.ts
@@ -12,7 +12,8 @@ export type AndroidSetupDeps = {
     hasExpectedServiceVersion: () => Promise<boolean>;
     installService: () => Promise<boolean>;
     hasExpectedPermissions: () => Promise<boolean>;
-    setTcpForwarding: () => Promise<number>;
+    setupTcpForwarding: () => Promise<number>;
+    removeTcpForwarding: (hostPort: number) => Promise<void>;
     getApplicationName: () => Promise<string>;
     logger: Logger;
 };
diff --git a/src/electron/platform/android/setup/live-android-setup-deps.ts b/src/electron/platform/android/setup/live-android-setup-deps.ts
@@ -93,8 +93,12 @@ export class LiveAndroidSetupDeps implements AndroidSetupDeps {
         return false;
     };
 
-    public setTcpForwarding = async (): Promise<number> => {
-        return await this.serviceConfig.setTcpForwarding(this.selectedDeviceId);
+    public setupTcpForwarding = async (): Promise<number> => {
+        return await this.serviceConfig.setupTcpForwarding(this.selectedDeviceId);
+    };
+
+    public removeTcpForwarding = async (hostPort: number): Promise<void> => {
+        return await this.serviceConfig.removeTcpForwarding(this.selectedDeviceId, hostPort);
     };
 
     public getApplicationName = async (): Promise<string> => {

diff --git a/src/electron/platform/android/setup/steps/configuring-port-forwarding.ts b/src/electron/platform/android/setup/steps/configuring-port-forwarding.ts
@@ -7,9 +7,15 @@ export const configuringPortForwarding: AndroidSetupStepConfig = deps => ({
     actions: {},
     onEnter: async () => {
         try {
-            deps.setScanPort(null);
-            deps.setApplicationName(null);
-            const hostPort = await deps.setTcpForwarding();
+            const existingPort = deps.getScanPort();
+            if (existingPort != null) {
+                deps.logger.log(`removing old tcp:${existingPort} forwarding`);
+                await deps.removeTcpForwarding(existingPort);
+                deps.setScanPort(null);
+                deps.setApplicationName(null);
+            }
+
+            const hostPort = await deps.setupTcpForwarding();
             deps.logger.log(`configured forwarding to tcp:${hostPort}`);
             const appName = await deps.getApplicationName();
 

diff --git a/src/electron/views/renderer-initializer.ts b/src/electron/views/renderer-initializer.ts
@@ -94,6 +94,7 @@ import { PlainTextFormatter } from 'issue-filing/common/markup/plain-text-format
 import { IssueFilingServiceProviderForUnifiedImpl } from 'issue-filing/issue-filing-service-provider-for-unified-impl';
 import { UnifiedResultToIssueFilingDataConverter } from 'issue-filing/unified-result-to-issue-filing-data';
 import { loadTheme, setFocusVisibility } from 'office-ui-fabric-react';
+import { getPortPromise } from 'portfinder';
 import * as ReactDOM from 'react-dom';
 import { ReportExportServiceProviderImpl } from 'report-export/report-export-service-provider-impl';
 import { getDefaultAddListenerForCollapsibleSection } from 'reports/components/report-sections/collapsible-script-provider';
@@ -204,7 +205,11 @@ getPersistedData(indexedDBInstance, indexedDBDataKeysToFetch).then(
             androidSetupActions,
             createAndroidSetupStateMachineFactory(
                 new LiveAndroidSetupDeps(
-                    new AppiumServiceConfiguratorFactory(new LiveAppiumAdbCreator(), apkLocator),
+                    new AppiumServiceConfiguratorFactory(
+                        new LiveAppiumAdbCreator(),
+                        apkLocator,
+                        getPortPromise,
+                    ),
                     userConfigurationStore,
                     apkLocator,
                     userConfigMessageCreator,

diff --git a/src/tests/unit/tests/electron/platform/android/appium-service-configurator-factory.test.ts b/src/tests/unit/tests/electron/platform/android/appium-service-configurator-factory.test.ts
@@ -3,17 +3,22 @@
 
 import { AndroidServiceApkLocator } from 'electron/platform/android/android-service-apk-locator';
 import { AppiumAdbCreator } from 'electron/platform/android/appium-adb-creator';
-import { AppiumServiceConfigurator } from 'electron/platform/android/appium-service-configurator';
+import {
+    AppiumServiceConfigurator,
+    PortFinder,
+} from 'electron/platform/android/appium-service-configurator';
 import { AppiumServiceConfiguratorFactory } from 'electron/platform/android/appium-service-configurator-factory';
 import { IMock, Mock, MockBehavior, Times } from 'typemoq';
 
 describe('AppiumServiceConfiguratorFactory tests', () => {
     let adbCreatorMock: IMock<AppiumAdbCreator>;
     let apkLocatorMock: IMock<AndroidServiceApkLocator>;
+    let portFinderMock: IMock<PortFinder>;
 
     beforeEach(() => {
         adbCreatorMock = Mock.ofType<AppiumAdbCreator>(undefined, MockBehavior.Strict);
         apkLocatorMock = Mock.ofType<AndroidServiceApkLocator>(undefined, MockBehavior.Strict);
+        portFinderMock = Mock.ofType<PortFinder>(undefined, MockBehavior.Strict);
     });
 
     it('getServiceConfigurator creates without parameters if no sdkRoot is provided', async () => {
@@ -24,14 +29,14 @@ describe('AppiumServiceConfiguratorFactory tests', () => {
         const factory = new AppiumServiceConfiguratorFactory(
             adbCreatorMock.object,
             apkLocatorMock.object,
+            portFinderMock.object,
         );
 
         expect(await factory.getServiceConfigurator(null)).toBeInstanceOf(
             AppiumServiceConfigurator,
         );
 
-        adbCreatorMock.verifyAll();
-        apkLocatorMock.verifyAll();
+        verifyAllMocks();
     });
 
     it('getServiceConfigurator creates with sdkRoot if it is provided', async () => {
@@ -43,14 +48,14 @@ describe('AppiumServiceConfiguratorFactory tests', () => {
         const factory = new AppiumServiceConfiguratorFactory(
             adbCreatorMock.object,
             apkLocatorMock.object,
+            portFinderMock.object,
         );
 
         expect(await factory.getServiceConfigurator(expectedSdkRoot)).toBeInstanceOf(
             AppiumServiceConfigurator,
         );
 
-        adbCreatorMock.verifyAll();
-        apkLocatorMock.verifyAll();
+        verifyAllMocks();
     });
 
     it('getServiceConfigurator propagates error to caller', async () => {
@@ -62,11 +67,17 @@ describe('AppiumServiceConfiguratorFactory tests', () => {
         const factory = new AppiumServiceConfiguratorFactory(
             adbCreatorMock.object,
             apkLocatorMock.object,
+            portFinderMock.object,
         );
 
         await expect(factory.getServiceConfigurator(null)).rejects.toThrowError(expectedMessage);
 
+        verifyAllMocks();
+    });
+
+    function verifyAllMocks(): void {
         adbCreatorMock.verifyAll();
         apkLocatorMock.verifyAll();
-    });
+        portFinderMock.verifyAll();
+    }
 });