From 4ae7d9c42b4bbf0485081e9713a76fdb80aabb19 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 17 Aug 2021 04:43:18 -0700 Subject: [PATCH] Decouple Metro device handling from Flipper core Summary: Decoupled metro 'device' from Redux store. Extracting some commonalities with Android device management up into FlipperServer Reviewed By: timur-valiev Differential Revision: D30309256 fbshipit-source-id: 1a9ac01e3f21d2d08761554d3644a7ae8d00a93e --- desktop/app/src/dispatcher/flipperServer.tsx | 23 +++- .../appinspect/__tests__/PluginList.spec.tsx | 6 +- desktop/app/src/server/FlipperServer.tsx | 123 ++++++++++++++++-- .../devices/android/androidDeviceManager.tsx | 12 +- .../devices/desktop/desktopDeviceManager.tsx | 2 +- .../devices/metro/metroDeviceManager.tsx | 44 ++----- 6 files changed, 152 insertions(+), 58 deletions(-) diff --git a/desktop/app/src/dispatcher/flipperServer.tsx b/desktop/app/src/dispatcher/flipperServer.tsx index e6b13aa66..de08b8eaa 100644 --- a/desktop/app/src/dispatcher/flipperServer.tsx +++ b/desktop/app/src/dispatcher/flipperServer.tsx @@ -17,7 +17,7 @@ import {notification} from 'antd'; export default async (store: Store, logger: Logger) => { const {enableAndroid, androidHome} = store.getState().settingsState; - const server = await startFlipperServer( + const server = startFlipperServer( { enableAndroid, androidHome, @@ -40,7 +40,7 @@ export default async (store: Store, logger: Logger) => { }); }); - server.on('server-start-error', (err) => { + server.on('server-error', (err) => { notification.error({ message: 'Failed to start connection server', description: @@ -58,7 +58,7 @@ export default async (store: Store, logger: Logger) => { {'' + err} ) : ( - <>Failed to start connection server: ${err.message} + <>Failed to start Flipper server: ${err.message} ), duration: null, }); @@ -87,7 +87,7 @@ export default async (store: Store, logger: Logger) => { os: device.os, serial: device.serial, }); - // N.B.: note that we don't remove the device, we keep it in offline mode! + // N.B.: note that we don't remove the device, we keep it in offline }); server.on('client-connected', (payload) => @@ -100,6 +100,21 @@ export default async (store: Store, logger: Logger) => { }); } + server + .waitForServerStarted() + .then(() => { + console.log( + 'Flipper server started and accepting device / client connections', + ); + }) + .catch((e) => { + console.error('Failed to start Flipper server', e); + notification.error({ + message: 'Failed to start Flipper server', + description: 'error: ' + e, + }); + }); + return () => { server.close(); }; diff --git a/desktop/app/src/sandy-chrome/appinspect/__tests__/PluginList.spec.tsx b/desktop/app/src/sandy-chrome/appinspect/__tests__/PluginList.spec.tsx index 7bb58f18d..973a77edc 100644 --- a/desktop/app/src/sandy-chrome/appinspect/__tests__/PluginList.spec.tsx +++ b/desktop/app/src/sandy-chrome/appinspect/__tests__/PluginList.spec.tsx @@ -17,7 +17,6 @@ import BaseDevice from '../../../server/devices/BaseDevice'; import {_SandyPluginDefinition} from 'flipper-plugin'; import {TestUtils} from 'flipper-plugin'; import {selectPlugin} from '../../../reducers/connections'; -import {registerMetroDevice} from '../../../server/devices/metro/metroDeviceManager'; import { addGatekeepedPlugins, registerMarketplacePlugins, @@ -78,7 +77,10 @@ describe('basic getActiveDevice with metro present', () => { }; testDevice = flipper.device; // flipper.store.dispatch(registerPlugins([LogsPlugin])) - await registerMetroDevice(undefined, flipper.store, flipper.logger); + flipper.store.dispatch({ + type: 'REGISTER_DEVICE', + payload: new MetroDevice('http://localhost:8081', undefined), + }); metro = getMetroDevice(flipper.store.getState())!; metro.supportsPlugin = (p) => { return p.id !== 'unsupportedDevicePlugin'; diff --git a/desktop/app/src/server/FlipperServer.tsx b/desktop/app/src/server/FlipperServer.tsx index 142cfd3b4..8509e23b8 100644 --- a/desktop/app/src/server/FlipperServer.tsx +++ b/desktop/app/src/server/FlipperServer.tsx @@ -30,7 +30,8 @@ import desktopDevice from './devices/desktop/desktopDeviceManager'; import BaseDevice from './devices/BaseDevice'; type FlipperServerEvents = { - 'server-start-error': any; + 'server-state': {state: ServerState; error?: Error}; + 'server-error': any; notification: { type: 'error'; title: string; @@ -47,17 +48,18 @@ export interface FlipperServerConfig { serverPorts: ServerPorts; } -export async function startFlipperServer( +export function startFlipperServer( config: FlipperServerConfig, store: Store, logger: Logger, -): Promise { +): FlipperServer { const server = new FlipperServer(config, store, logger); - - await server.start(); + server.start(); return server; } +type ServerState = 'pending' | 'starting' | 'started' | 'error' | 'closed'; + /** * FlipperServer takes care of all incoming device & client connections. * It will set up managers per device type, and create the incoming @@ -70,7 +72,8 @@ export class FlipperServer { private readonly events = new EventEmitter(); readonly server: ServerController; readonly disposers: ((() => void) | void)[] = []; - + private readonly devices = new Map(); + state: ServerState = 'pending'; android: AndroidDeviceManager; // TODO: remove store argument @@ -84,8 +87,50 @@ export class FlipperServer { this.android = new AndroidDeviceManager(this); } + setServerState(state: ServerState, error?: Error) { + this.state = state; + this.emit('server-state', {state, error}); + } + + async waitForServerStarted() { + return new Promise((resolve, reject) => { + switch (this.state) { + case 'closed': + return reject(new Error('Server was closed already')); + case 'error': + return reject(new Error('Server has errored already')); + case 'started': + return resolve(); + default: { + const listener = ({ + state, + error, + }: { + state: ServerState; + error: Error; + }) => { + switch (state) { + case 'error': + return reject(error); + case 'started': + return resolve(); + case 'closed': + return reject(new Error('Server closed')); + } + this.events.off('server-state', listener); + }; + this.events.on('server-state', listener); + } + } + }); + } + /** @private */ async start() { + if (this.state !== 'pending') { + throw new Error('Server already started'); + } + this.setServerState('starting'); const server = this.server; server.addListener('new-client', (client: Client) => { @@ -93,7 +138,7 @@ export class FlipperServer { }); server.addListener('error', (err) => { - this.emit('server-start-error', err); + this.emit('server-error', err); }); server.addListener('start-client-setup', (client: UninitializedClient) => { @@ -170,15 +215,21 @@ export class FlipperServer { }, ); - await server.init(); - await this.startDeviceListeners(); + try { + await server.init(); + await this.startDeviceListeners(); + this.setServerState('started'); + } catch (e) { + console.error('Failed to start FlipperServer', e); + this.setServerState('error', e); + } } async startDeviceListeners() { this.disposers.push( await this.android.watchAndroidDevices(), iOSDevice(this.store, this.logger), - metroDevice(this.store, this.logger), + metroDevice(this), desktopDevice(this), ); } @@ -200,8 +251,60 @@ export class FlipperServer { this.events.emit(event, payload); } + registerDevice(device: BaseDevice) { + // destroy existing device + const existing = this.devices.get(device.serial); + if (existing) { + // assert different kind of devices aren't accidentally reusing the same serial + if (Object.getPrototypeOf(existing) !== Object.getPrototypeOf(device)) { + throw new Error( + `Tried to register a new device type for existing serial '${ + device.serial + }': Trying to replace existing '${ + Object.getPrototypeOf(existing).constructor.name + }' with a new '${Object.getPrototypeOf(device).constructor.name}`, + ); + } + // devices should be recycled, unless they have lost connection + if (existing.connected.get()) { + throw new Error( + `Tried to replace still connected device '${device.serial}' with a new instance`, + ); + } + existing.destroy(); + } + // register new device + this.devices.set(device.serial, device); + this.emit('device-connected', device); + } + + unregisterDevice(serial: string) { + const device = this.devices.get(serial); + if (!device) { + return; + } + device.disconnect(); // we'll only destroy upon replacement + this.emit('device-disconnected', device); + } + + getDevice(serial: string): BaseDevice { + const device = this.devices.get(serial); + if (!device) { + throw new Error('No device with serial: ' + serial); + } + return device; + } + + getDeviceSerials(): string[] { + return Array.from(this.devices.keys()); + } + public async close() { this.server.close(); + for (const device of this.devices.values()) { + device.destroy(); + } this.disposers.forEach((f) => f?.()); + this.setServerState('closed'); } } diff --git a/desktop/app/src/server/devices/android/androidDeviceManager.tsx b/desktop/app/src/server/devices/android/androidDeviceManager.tsx index 82f628ca5..397189514 100644 --- a/desktop/app/src/server/devices/android/androidDeviceManager.tsx +++ b/desktop/app/src/server/devices/android/androidDeviceManager.tsx @@ -230,20 +230,12 @@ export class AndroidDeviceManager { return; } - // remove offline devices with same serial as the connected. - this.devices.get(androidDevice.serial)?.destroy(); - // register new device - this.devices.set(androidDevice.serial, androidDevice); - this.flipperServer.emit('device-connected', androidDevice); + this.flipperServer.registerDevice(androidDevice); } unregisterDevices(serials: Array) { serials.forEach((serial) => { - const device = this.devices.get(serial); - if (device?.connected?.get()) { - device.disconnect(); - this.flipperServer.emit('device-disconnected', device); - } + this.flipperServer.unregisterDevice(serial); }); } } diff --git a/desktop/app/src/server/devices/desktop/desktopDeviceManager.tsx b/desktop/app/src/server/devices/desktop/desktopDeviceManager.tsx index 8b56b086f..85e832e9a 100644 --- a/desktop/app/src/server/devices/desktop/desktopDeviceManager.tsx +++ b/desktop/app/src/server/devices/desktop/desktopDeviceManager.tsx @@ -20,5 +20,5 @@ export default (flipperServer: FlipperServer) => { } else { return; } - flipperServer.emit('device-connected', device); + flipperServer.registerDevice(device); }; diff --git a/desktop/app/src/server/devices/metro/metroDeviceManager.tsx b/desktop/app/src/server/devices/metro/metroDeviceManager.tsx index 98a5e8756..f268289e6 100644 --- a/desktop/app/src/server/devices/metro/metroDeviceManager.tsx +++ b/desktop/app/src/server/devices/metro/metroDeviceManager.tsx @@ -7,13 +7,10 @@ * @format */ -import {Store} from '../../../reducers/index'; -import {Logger} from '../../../fb-interfaces/Logger'; import MetroDevice from './MetroDevice'; import http from 'http'; -import {addErrorNotification} from '../../../reducers/notifications'; -import {destroyDevice} from '../../../reducers/connections'; import {parseEnvironmentVariableAsNumber} from '../../utils/environmentVariables'; +import {FlipperServer} from '../../FlipperServer'; const METRO_HOST = 'localhost'; const METRO_PORT = parseEnvironmentVariableAsNumber('METRO_SERVER_PORT', 8081); @@ -47,29 +44,15 @@ async function isMetroRunning(): Promise { }); } -export async function registerMetroDevice( +async function registerMetroDevice( ws: WebSocket | undefined, - store: Store, - logger: Logger, + flipperServer: FlipperServer, ) { const metroDevice = new MetroDevice(METRO_URL, ws); - logger.track('usage', 'register-device', { - os: 'Metro', - name: metroDevice.title, - }); - - metroDevice.loadDevicePlugins( - store.getState().plugins.devicePlugins, - store.getState().connections.enabledDevicePlugins, - ); - store.dispatch({ - type: 'REGISTER_DEVICE', - payload: metroDevice, - serial: METRO_URL, - }); + flipperServer.registerDevice(metroDevice); } -export default (store: Store, logger: Logger) => { +export default (flipperServer: FlipperServer) => { let timeoutHandle: NodeJS.Timeout; let ws: WebSocket | undefined; let unregistered = false; @@ -86,7 +69,7 @@ export default (store: Store, logger: Logger) => { _ws.onopen = () => { clearTimeout(guard); ws = _ws; - registerMetroDevice(ws, store, logger); + registerMetroDevice(ws, flipperServer); }; _ws.onclose = _ws.onerror = function (event?: any) { @@ -100,20 +83,19 @@ export default (store: Store, logger: Logger) => { unregistered = true; clearTimeout(guard); ws = undefined; - destroyDevice(store, logger, METRO_URL); + flipperServer.unregisterDevice(METRO_URL); scheduleNext(); } }; const guard = setTimeout(() => { // Metro is running, but didn't respond to /events endpoint - store.dispatch( - addErrorNotification( - 'Failed to connect to Metro', - `Flipper did find a running Metro instance, but couldn't connect to the logs. Probably your React Native version is too old to support Flipper. Cause: Failed to get a connection to ${METRO_LOGS_ENDPOINT} in a timely fashion`, - ), - ); - registerMetroDevice(undefined, store, logger); + flipperServer.emit('notification', { + type: 'error', + title: 'Failed to connect to Metro', + description: `Flipper did find a running Metro instance, but couldn't connect to the logs. Probably your React Native version is too old to support Flipper. Cause: Failed to get a connection to ${METRO_LOGS_ENDPOINT} in a timely fashion`, + }); + registerMetroDevice(undefined, flipperServer); // Note: no scheduleNext, we won't retry until restart }, 5000); } catch (e) {