From 224ec4d5d6eaa6b55893cc6f9659fbc21ad4b1cc Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 2 Mar 2021 03:57:02 -0800 Subject: [PATCH] Fix device cleanup Summary: Fixes https://github.com/facebook/flipper/issues/1989 We had some self healing side effect that would destroy devices when registering a new device with the same serial, if they weren't yet. Redux isn't too happy about that, causing the attached crash. Instead introduced a utility to destroy devices, and log an error if the device life cycle isn't respected by the device implementations, rather than crashing we will now just waste some memory. Changelog: Fix a crash when disconnecting metro devices Reviewed By: passy Differential Revision: D26749214 fbshipit-source-id: 4c185ac521d44c1337fac8a9145440123b8b784c --- desktop/app/src/__tests__/disconnect.node.tsx | 9 +++++- desktop/app/src/dispatcher/androidDevice.tsx | 6 ++-- desktop/app/src/dispatcher/iOSDevice.tsx | 3 ++ desktop/app/src/dispatcher/metroDevice.tsx | 15 ++-------- .../__tests__/sandydeviceplugins.node.tsx | 9 ++---- desktop/app/src/reducers/connections.tsx | 28 ++++++++++++++++--- desktop/app/src/server.tsx | 6 ++-- .../js-client-server-utils/serverUtils.tsx | 8 ++---- .../self-inspection/selfInspectionUtils.tsx | 7 ++--- 9 files changed, 49 insertions(+), 42 deletions(-) diff --git a/desktop/app/src/__tests__/disconnect.node.tsx b/desktop/app/src/__tests__/disconnect.node.tsx index 70a711d09..d9b6bff44 100644 --- a/desktop/app/src/__tests__/disconnect.node.tsx +++ b/desktop/app/src/__tests__/disconnect.node.tsx @@ -17,6 +17,7 @@ import { PluginClient, } from 'flipper-plugin'; import {registerNewClient} from '../dispatcher/server'; +import {destroyDevice} from '../reducers/connections'; test('Devices can disconnect', async () => { const deviceplugin = new _SandyPluginDefinition( @@ -89,7 +90,9 @@ test('New device with same serial removes & cleans the old one', async () => { }, }, ); - const {device, store} = await createMockFlipperWithPlugin(deviceplugin); + const {device, store, logger} = await createMockFlipperWithPlugin( + deviceplugin, + ); const instance = device.sandyPluginStates.get(deviceplugin.id)!; @@ -98,6 +101,10 @@ test('New device with same serial removes & cleans the old one', async () => { expect(instance.instanceApi.destroy).toBeCalledTimes(0); expect(store.getState().connections.devices).toEqual([device]); + // calling destroy explicitly defeats the point of this test a bit, + // but we now print an error rather than proactively destroying the device, + // see https://github.com/facebook/flipper/issues/1989 + destroyDevice(store, logger, device.serial); // submit a new device with same serial const device2 = new BaseDevice( device.serial, diff --git a/desktop/app/src/dispatcher/androidDevice.tsx b/desktop/app/src/dispatcher/androidDevice.tsx index 86b7bfae4..f70a0085f 100644 --- a/desktop/app/src/dispatcher/androidDevice.tsx +++ b/desktop/app/src/dispatcher/androidDevice.tsx @@ -20,6 +20,7 @@ import {promisify} from 'util'; import {ServerPorts} from '../reducers/application'; import {Client as ADBClient} from 'adbkit'; import {addErrorNotification} from '../reducers/notifications'; +import {destroyDevice} from '../reducers/connections'; function createDevice( adbClient: ADBClient, @@ -235,9 +236,8 @@ export default (store: Store, logger: Logger) => { ) .map((device) => device.serial); - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: new Set(reconnectedDevices), + reconnectedDevices.forEach((serial) => { + destroyDevice(store, logger, serial); }); androidDevice.loadDevicePlugins( diff --git a/desktop/app/src/dispatcher/iOSDevice.tsx b/desktop/app/src/dispatcher/iOSDevice.tsx index 90c455996..2df5ef737 100644 --- a/desktop/app/src/dispatcher/iOSDevice.tsx +++ b/desktop/app/src/dispatcher/iOSDevice.tsx @@ -21,6 +21,7 @@ import IOSDevice from '../devices/IOSDevice'; import {registerDeviceCallbackOnPlugins} from '../utils/onRegisterDevice'; import {addErrorNotification} from '../reducers/notifications'; import {getStaticPath} from '../utils/pathUtils'; +import {destroyDevice} from '../reducers/connections'; type iOSSimulatorDevice = { state: 'Booted' | 'Shutdown' | 'Shutting Down'; @@ -127,6 +128,8 @@ function processDevices( if (currentDeviceIDs.has(udid)) { currentDeviceIDs.delete(udid); } else { + // clean up offline device + destroyDevice(store, logger, udid); logger.track('usage', 'register-device', { os: 'iOS', type: type, diff --git a/desktop/app/src/dispatcher/metroDevice.tsx b/desktop/app/src/dispatcher/metroDevice.tsx index 10d39f219..8036a119b 100644 --- a/desktop/app/src/dispatcher/metroDevice.tsx +++ b/desktop/app/src/dispatcher/metroDevice.tsx @@ -13,6 +13,7 @@ import {registerDeviceCallbackOnPlugins} from '../utils/onRegisterDevice'; import MetroDevice from '../devices/MetroDevice'; import http from 'http'; import {addErrorNotification} from '../reducers/notifications'; +import {destroyDevice} from '../reducers/connections'; const METRO_PORT = 8081; const METRO_HOST = 'localhost'; @@ -75,18 +76,6 @@ export async function registerMetroDevice( ); } -async function unregisterDevices(store: Store, logger: Logger) { - logger.track('usage', 'unregister-device', { - os: 'Metro', - serial: METRO_URL, - }); - - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: new Set([METRO_URL]), - }); -} - export default (store: Store, logger: Logger) => { let timeoutHandle: NodeJS.Timeout; let ws: WebSocket | undefined; @@ -111,7 +100,7 @@ export default (store: Store, logger: Logger) => { unregistered = true; clearTimeout(guard); ws = undefined; - unregisterDevices(store, logger); + destroyDevice(store, logger, METRO_URL); scheduleNext(); } }; diff --git a/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx b/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx index 5470ff09a..acb5a41ad 100644 --- a/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx +++ b/desktop/app/src/reducers/__tests__/sandydeviceplugins.node.tsx @@ -9,7 +9,7 @@ import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin'; import {Store} from '../../'; -import {selectPlugin} from '../../reducers/connections'; +import {destroyDevice, selectPlugin} from '../../reducers/connections'; import { _SandyPluginDefinition, _SandyDevicePluginInstance, @@ -87,15 +87,12 @@ test('it should initialize device sandy plugins', async () => { }); test('it should cleanup if device is removed', async () => { - const {device, store} = await createMockFlipperWithPlugin(TestPlugin); + const {device, store, logger} = await createMockFlipperWithPlugin(TestPlugin); const pluginInstance = device.sandyPluginStates.get(TestPlugin.id)!; expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0); // close device - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: new Set([device.serial]), - }); + destroyDevice(store, logger, device.serial); expect( (pluginInstance.instanceApi as PluginApi).destroyStub, ).toHaveBeenCalledTimes(1); diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index ceadcedd4..ce46256ee 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -16,7 +16,7 @@ import type Client from '../Client'; import type {UninitializedClient} from '../UninitializedClient'; import {isEqual} from 'lodash'; import {performance} from 'perf_hooks'; -import type {Actions} from '.'; +import type {Actions, Store} from '.'; import {WelcomeScreenStaticView} from '../sandy-chrome/WelcomeScreen'; import {getPluginKey, isDevicePluginDefinition} from '../utils/pluginUtils'; import {deconstructClientId} from '../utils/clientUtils'; @@ -231,10 +231,9 @@ export default (state: State = INITAL_STATE, action: Actions): State => { (device) => device.serial === payload.serial, ); if (existing !== -1) { - console.debug( + console.warn( `Got a new device instance for already existing serial ${payload.serial}`, ); - state.devices[existing].destroy(); newDevices[existing] = payload; } else { newDevices.push(payload); @@ -255,7 +254,11 @@ export default (state: State = INITAL_STATE, action: Actions): State => { if (!deviceSerials.has(device.serial)) { return true; } else { - device.destroy(); + if (device.connected.get()) { + console.warn( + 'Tried to unregister a device before it was destroyed', + ); + } return false; } }); @@ -654,3 +657,20 @@ export function isPluginEnabled( const enabledAppPlugins = enabledPlugins[appInfo.app]; return enabledAppPlugins && enabledAppPlugins.indexOf(pluginId) > -1; } + +export function destroyDevice(store: Store, logger: Logger, serial: string) { + const device = store + .getState() + .connections.devices.find((device) => device.serial === serial); + if (device) { + device.destroy(); + logger.track('usage', 'unregister-device', { + os: device.os, + serial, + }); + store.dispatch({ + type: 'UNREGISTER_DEVICES', + payload: new Set([serial]), + }); + } +} diff --git a/desktop/app/src/server.tsx b/desktop/app/src/server.tsx index 716d4d08f..0ac3cf341 100644 --- a/desktop/app/src/server.tsx +++ b/desktop/app/src/server.tsx @@ -40,6 +40,7 @@ import {initSelfInpector} from './utils/self-inspection/selfInspectionUtils'; import ClientDevice from './devices/ClientDevice'; import BaseDevice from './devices/BaseDevice'; import {sideEffect} from './utils/sideEffect'; +import {destroyDevice} from './reducers/connections'; type ClientInfo = { connection: FlipperClientConnection | null | undefined; @@ -206,10 +207,7 @@ class Server extends EventEmitter { Object.values(clients).map((p) => p.then((c) => this.removeConnection(c.id)), ); - this.store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: new Set([deviceId]), - }); + destroyDevice(this.store, this.logger, deviceId); }; ws.on('message', (rawMessage: any) => { diff --git a/desktop/app/src/utils/js-client-server-utils/serverUtils.tsx b/desktop/app/src/utils/js-client-server-utils/serverUtils.tsx index 56e1b408d..ae3f97409 100644 --- a/desktop/app/src/utils/js-client-server-utils/serverUtils.tsx +++ b/desktop/app/src/utils/js-client-server-utils/serverUtils.tsx @@ -18,6 +18,7 @@ import {Payload, ConnectionStatus, ISubscriber} from 'rsocket-types'; import {Flowable, Single} from 'rsocket-flowable'; import Server from '../../server'; import {buildClientId} from '../clientUtils'; +import {destroyDevice} from '../../reducers/connections'; const connections: Map> = new Map(); @@ -83,12 +84,7 @@ export function initJsEmulatorIPC( if (payload.kind == 'ERROR' || payload.kind == 'CLOSED') { console.debug(`Device disconnected ${client.id}`, 'server'); flipperServer.removeConnection(client.id); - const toUnregister = new Set(); - toUnregister.add(jsDeviceId(windowId)); - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: toUnregister, - }); + destroyDevice(store, logger, jsDeviceId(windowId)); connections.delete(windowId); availablePlugins.delete(windowId); } diff --git a/desktop/app/src/utils/self-inspection/selfInspectionUtils.tsx b/desktop/app/src/utils/self-inspection/selfInspectionUtils.tsx index d27969728..e9957ee69 100644 --- a/desktop/app/src/utils/self-inspection/selfInspectionUtils.tsx +++ b/desktop/app/src/utils/self-inspection/selfInspectionUtils.tsx @@ -16,6 +16,7 @@ import Server from '../../server'; import {buildClientId} from '../clientUtils'; import {selfInspectionClient} from './selfInspectionClient'; import {flipperMessagesClientPlugin} from './plugins/FlipperMessagesClientPlugin'; +import {destroyDevice} from '../../reducers/connections'; export function initSelfInpector( store: Store, @@ -69,11 +70,7 @@ export function initSelfInpector( if (payload.kind == 'ERROR' || payload.kind == 'CLOSED') { console.debug(`Device disconnected ${client.id}`, 'server'); flipperServer.removeConnection(client.id); - const toUnregister = new Set(); - store.dispatch({ - type: 'UNREGISTER_DEVICES', - payload: toUnregister, - }); + destroyDevice(store, logger, client.id); } }, onSubscribe(subscription) {