diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index 27a95a816..17fb48b0b 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -35,7 +35,7 @@ import {processMessagesLater} from './utils/messageQueue'; import {emitBytesReceived} from './dispatcher/tracking'; import {debounce} from 'lodash'; import {batch} from 'react-redux'; -import {_SandyPluginInstance} from 'flipper-plugin'; +import {createState, _SandyPluginInstance} from 'flipper-plugin'; import {flipperMessagesClientPlugin} from './utils/self-inspection/plugins/FlipperMessagesClientPlugin'; import {getFlipperLibImplementation} from './utils/flipperLibImplementation'; import {freeze} from 'immer'; @@ -123,7 +123,7 @@ export interface FlipperClientConnection { } export default class Client extends EventEmitter { - connected: boolean; + connected = createState(false); id: string; query: ClientQuery; sdkVersion: number; @@ -175,7 +175,7 @@ export default class Client extends EventEmitter { device: BaseDevice, ) { super(); - this.connected = true; + this.connected.set(true); this.plugins = plugins ? plugins : []; this.backgroundPlugins = []; this.connection = conn; @@ -197,7 +197,7 @@ export default class Client extends EventEmitter { conn.connectionStatus().subscribe({ onNext(payload) { if (payload.kind == 'ERROR' || payload.kind == 'CLOSED') { - client.connected = false; + client.connected.set(false); } }, onSubscribe(subscription) { @@ -324,8 +324,19 @@ export default class Client extends EventEmitter { } } - close() { + // connection lost, but Client might live on + disconnect() { + this.sandyPluginStates.forEach((instance) => { + instance.disconnect(); + }); this.emit('close'); + this.connected.set(false); + this.connection = undefined; + } + + // clean up this client + destroy() { + this.disconnect(); this.plugins.forEach((pluginId) => this.stopPluginIfNeeded(pluginId, true)); } diff --git a/desktop/app/src/__tests__/disconnect.node.tsx b/desktop/app/src/__tests__/disconnect.node.tsx index b3587beab..eac6a7e24 100644 --- a/desktop/app/src/__tests__/disconnect.node.tsx +++ b/desktop/app/src/__tests__/disconnect.node.tsx @@ -14,7 +14,9 @@ import { _SandyPluginDefinition, createState, DevicePluginClient, + PluginClient, } from 'flipper-plugin'; +import {registerNewClient} from '../dispatcher/server'; test('Devices can disconnect', async () => { const deviceplugin = new _SandyPluginDefinition( @@ -43,12 +45,12 @@ test('Devices can disconnect', async () => { expect(device.isArchived).toBe(false); - device.markDisconnected(); + device.disconnect(); expect(device.isArchived).toBe(true); const instance = device.sandyPluginStates.get(deviceplugin.id)!; expect(instance).toBeTruthy(); - expect(instance.instanceApi.counter.get(1)).toBe(1); // state preserved + expect(instance.instanceApi.counter.get()).toBe(1); // state preserved expect(instance.instanceApi.destroy).toBeCalledTimes(0); device.destroy(); @@ -106,3 +108,113 @@ test('New device with same serial removes & cleans the old one', async () => { expect(store.getState().connections.devices.length).toBe(1); expect(store.getState().connections.devices[0]).toBe(device2); }); + +test('clients can disconnect but preserve state', async () => { + const plugin = new _SandyPluginDefinition( + TestUtils.createMockPluginDetails(), + { + plugin(client: PluginClient) { + const connect = jest.fn(); + const disconnect = jest.fn(); + const destroy = jest.fn(); + client.onConnect(connect); + client.onDestroy(destroy); + client.onDisconnect(disconnect); + const counter = createState(0); + return { + connect, + disconnect, + counter, + destroy, + }; + }, + Component() { + return null; + }, + }, + ); + const {client} = await createMockFlipperWithPlugin(plugin, { + asBackgroundPlugin: true, + }); + + let instance = client.sandyPluginStates.get(plugin.id)!; + instance.instanceApi.counter.set(1); + expect(instance.instanceApi.destroy).toBeCalledTimes(0); + expect(instance.instanceApi.connect).toBeCalledTimes(1); + expect(instance.instanceApi.disconnect).toBeCalledTimes(0); + expect(client.connected.get()).toBe(true); + + client.disconnect(); + + expect(client.connected.get()).toBe(false); + instance = client.sandyPluginStates.get(plugin.id)!; + expect(instance).toBeTruthy(); + expect(instance.instanceApi.counter.get()).toBe(1); // state preserved + expect(instance.instanceApi.destroy).toBeCalledTimes(0); + expect(instance.instanceApi.connect).toBeCalledTimes(1); + expect(instance.instanceApi.disconnect).toBeCalledTimes(1); + + client.destroy(); + expect(instance.instanceApi.destroy).toBeCalledTimes(1); + expect(instance.instanceApi.connect).toBeCalledTimes(1); + expect(instance.instanceApi.disconnect).toBeCalledTimes(1); + + expect(client.sandyPluginStates.get(plugin.id)).toBeUndefined(); +}); + +test('new clients replace old ones', async () => { + const plugin = new _SandyPluginDefinition( + TestUtils.createMockPluginDetails(), + { + plugin(client: PluginClient) { + const connect = jest.fn(); + const disconnect = jest.fn(); + const destroy = jest.fn(); + client.onConnect(connect); + client.onDestroy(destroy); + client.onDisconnect(disconnect); + const counter = createState(0); + return { + connect, + disconnect, + counter, + destroy, + }; + }, + Component() { + return null; + }, + }, + ); + const { + client, + store, + device, + createClient, + } = await createMockFlipperWithPlugin(plugin, { + asBackgroundPlugin: true, + }); + + const instance = client.sandyPluginStates.get(plugin.id)!; + instance.instanceApi.counter.set(1); + expect(instance.instanceApi.destroy).toBeCalledTimes(0); + expect(instance.instanceApi.connect).toBeCalledTimes(1); + expect(instance.instanceApi.disconnect).toBeCalledTimes(0); + + const client2 = await createClient(device, 'AnotherApp', client.query, true); + registerNewClient(store, client2); + + expect(client2.connected.get()).toBe(true); + const instance2 = client2.sandyPluginStates.get(plugin.id)!; + expect(instance2).toBeTruthy(); + expect(instance2.instanceApi.counter.get()).toBe(0); + expect(instance2.instanceApi.destroy).toBeCalledTimes(0); + expect(instance2.instanceApi.connect).toBeCalledTimes(1); + expect(instance2.instanceApi.disconnect).toBeCalledTimes(0); + + expect(client.connected.get()).toBe(false); + expect(instance.instanceApi.counter.get()).toBe(1); + expect(instance.instanceApi.destroy).toBeCalledTimes(1); + expect(instance.instanceApi.connect).toBeCalledTimes(1); + expect(instance.instanceApi.disconnect).toBeCalledTimes(1); +}); diff --git a/desktop/app/src/devices/AndroidDevice.tsx b/desktop/app/src/devices/AndroidDevice.tsx index 227eb913d..42012d2f1 100644 --- a/desktop/app/src/devices/AndroidDevice.tsx +++ b/desktop/app/src/devices/AndroidDevice.tsx @@ -10,7 +10,6 @@ import BaseDevice from './BaseDevice'; import adb, {Client as ADBClient} from 'adbkit'; import {Priority} from 'adbkit-logcat'; -import ArchivedDevice from './ArchivedDevice'; import {createWriteStream} from 'fs'; import type {LogLevel, DeviceType} from 'flipper-plugin'; import which from 'which'; diff --git a/desktop/app/src/devices/BaseDevice.tsx b/desktop/app/src/devices/BaseDevice.tsx index 4695c462f..accfe0ce4 100644 --- a/desktop/app/src/devices/BaseDevice.tsx +++ b/desktop/app/src/devices/BaseDevice.tsx @@ -93,7 +93,7 @@ export default class BaseDevice { } displayTitle(): string { - return this.isArchived ? `${this.title} (Offline)` : this.title; + return this.title; } async exportState( @@ -221,12 +221,12 @@ export default class BaseDevice { this.devicePlugins.splice(this.devicePlugins.indexOf(pluginId), 1); } - markDisconnected() { + disconnect() { this.archivedState.set(true); } destroy() { - this.markDisconnected(); + this.disconnect(); this.sandyPluginStates.forEach((instance) => { instance.destroy(); }); diff --git a/desktop/app/src/dispatcher/androidDevice.tsx b/desktop/app/src/dispatcher/androidDevice.tsx index 834b0f4d9..434bcf8fc 100644 --- a/desktop/app/src/dispatcher/androidDevice.tsx +++ b/desktop/app/src/dispatcher/androidDevice.tsx @@ -266,7 +266,7 @@ export default (store: Store, logger: Logger) => { const device = store .getState() .connections.devices.find((device) => device.serial === id); - device?.markDisconnected(); + device?.disconnect(); }); } diff --git a/desktop/app/src/dispatcher/iOSDevice.tsx b/desktop/app/src/dispatcher/iOSDevice.tsx index 972d337ac..1fd4cb197 100644 --- a/desktop/app/src/dispatcher/iOSDevice.tsx +++ b/desktop/app/src/dispatcher/iOSDevice.tsx @@ -152,7 +152,7 @@ function processDevices( const device = store .getState() .connections.devices.find((device) => device.serial === id); - device?.markDisconnected(); + device?.disconnect(); }); } diff --git a/desktop/app/src/dispatcher/metroDevice.tsx b/desktop/app/src/dispatcher/metroDevice.tsx index c77d5644a..afc2f8f11 100644 --- a/desktop/app/src/dispatcher/metroDevice.tsx +++ b/desktop/app/src/dispatcher/metroDevice.tsx @@ -11,7 +11,6 @@ import {Store} from '../reducers/index'; import {Logger} from '../fb-interfaces/Logger'; import {registerDeviceCallbackOnPlugins} from '../utils/onRegisterDevice'; import MetroDevice from '../devices/MetroDevice'; -import ArchivedDevice from '../devices/ArchivedDevice'; import http from 'http'; import {addErrorNotification} from '../reducers/notifications'; diff --git a/desktop/app/src/dispatcher/server.tsx b/desktop/app/src/dispatcher/server.tsx index ee4aee572..d7c9ff7b4 100644 --- a/desktop/app/src/dispatcher/server.tsx +++ b/desktop/app/src/dispatcher/server.tsx @@ -15,31 +15,13 @@ import Client from '../Client'; import {UninitializedClient} from '../UninitializedClient'; import {addErrorNotification} from '../reducers/notifications'; import {CertificateExchangeMedium} from '../utils/CertificateProvider'; + export default (store: Store, logger: Logger) => { const server = new Server(logger, store); server.init(); server.addListener('new-client', (client: Client) => { - store.dispatch({ - type: 'NEW_CLIENT', - payload: client, - }); - }); - - server.addListener('removed-client', (id: string) => { - store.dispatch({ - type: 'CLIENT_REMOVED', - payload: id, - }); - store.dispatch({ - type: 'CLEAR_PLUGIN_STATE', - payload: { - clientId: id, - devicePlugins: new Set([ - ...store.getState().plugins.devicePlugins.keys(), - ]), - }, - }); + registerNewClient(store, client); }); server.addListener('error', (err) => { @@ -112,3 +94,29 @@ export default (store: Store, logger: Logger) => { } return server.close; }; + +export function registerNewClient(store: Store, client: Client) { + const existingClient = store + .getState() + .connections.clients.find((c) => c.id === client.id); + + if (existingClient) { + existingClient.destroy(); + store.dispatch({ + type: 'CLEAR_CLIENT_PLUGINS_STATE', + payload: { + clientId: client.id, + devicePlugins: new Set(), + }, + }); + store.dispatch({ + type: 'CLIENT_REMOVED', + payload: client.id, + }); + } + + store.dispatch({ + type: 'NEW_CLIENT', + payload: client, + }); +} diff --git a/desktop/app/src/reducers/__tests__/pluginStates.node.tsx b/desktop/app/src/reducers/__tests__/pluginStates.node.tsx index 2b52853b0..cf9a4004c 100644 --- a/desktop/app/src/reducers/__tests__/pluginStates.node.tsx +++ b/desktop/app/src/reducers/__tests__/pluginStates.node.tsx @@ -17,12 +17,12 @@ test('reduce setPluginState', () => { expect(result).toEqual({myPlugin: {a: 1}}); }); -test('CLEAR_PLUGIN_STATE removes plugin state', () => { +test('CLEAR_CLIENT_PLUGINS_STATE removes plugin state', () => { const clientId = 'app1#device1'; const pluginKey = 'app1#device1#plugin1'; const action: Action = { - type: 'CLEAR_PLUGIN_STATE', + type: 'CLEAR_CLIENT_PLUGINS_STATE', payload: {clientId: clientId, devicePlugins: new Set()}, }; const result = reducer( diff --git a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx index a9cfc17c5..4c1620469 100644 --- a/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx +++ b/desktop/app/src/reducers/__tests__/sandyplugins.node.tsx @@ -116,14 +116,33 @@ test('it should cleanup a plugin if disabled', async () => { expect(pluginInstance.destroyStub).toHaveBeenCalledTimes(1); }); -test('it should cleanup if client is removed', async () => { +test('it should NOT cleanup if client is removed', async () => { const {client} = await createMockFlipperWithPlugin(TestPlugin); const pluginInstance = client.sandyPluginStates.get(TestPlugin.id)!; expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0); - // close client - client.close(); + pluginInstance.connect(); + expect(client.connected.get()).toBe(true); + client.disconnect(); + expect(client.connected.get()).toBe(false); + expect(client.sandyPluginStates.has(TestPlugin.id)).toBeTruthy(); + expect( + (pluginInstance.instanceApi as PluginApi).disconnectStub, + ).toHaveBeenCalledTimes(1); + + expect( + (pluginInstance.instanceApi as PluginApi).destroyStub, + ).toHaveBeenCalledTimes(0); +}); + +test('it should cleanup if client is destroyed', async () => { + const {client} = await createMockFlipperWithPlugin(TestPlugin); + const pluginInstance = client.sandyPluginStates.get(TestPlugin.id)!; + expect(pluginInstance.instanceApi.destroyStub).toHaveBeenCalledTimes(0); + + client.destroy(); expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy(); + expect( (pluginInstance.instanceApi as PluginApi).destroyStub, ).toHaveBeenCalledTimes(1); @@ -188,11 +207,7 @@ test('it should not initialize a sandy plugin if not enabled', async () => { test('it trigger hooks for background plugins', async () => { const {client} = await createMockFlipperWithPlugin(TestPlugin, { - onSend(method) { - if (method === 'getBackgroundPlugins') { - return {plugins: [TestPlugin.id]}; - } - }, + asBackgroundPlugin: true, }); const pluginInstance: PluginApi = client.sandyPluginStates.get(TestPlugin.id)! .instanceApi; @@ -201,8 +216,7 @@ test('it trigger hooks for background plugins', async () => { expect(pluginInstance.connectStub).toHaveBeenCalledTimes(1); expect(pluginInstance.disconnectStub).toHaveBeenCalledTimes(0); - // close client - client.close(); + client.destroy(); expect(client.sandyPluginStates.has(TestPlugin.id)).toBeFalsy(); expect(pluginInstance.destroyStub).toHaveBeenCalledTimes(1); expect(pluginInstance.connectStub).toHaveBeenCalledTimes(1); @@ -289,7 +303,7 @@ test('it should initialize "Navigation" plugin if not enabled', async () => { expect(instance.destroyStub).toHaveBeenCalledTimes(0); // closing does stop the plugin! - client.close(); + client.destroy(); expect(instance.destroyStub).toHaveBeenCalledTimes(1); expect(client.sandyPluginStates.get(Plugin2.id)).toBeUndefined(); }); diff --git a/desktop/app/src/reducers/connections.tsx b/desktop/app/src/reducers/connections.tsx index 88f180d5f..beb96e740 100644 --- a/desktop/app/src/reducers/connections.tsx +++ b/desktop/app/src/reducers/connections.tsx @@ -283,9 +283,20 @@ export default (state: State = INITAL_STATE, action: Actions): State => { case 'NEW_CLIENT': { const {payload} = action; + const newClients = state.clients.filter((client) => { + if (client.id === payload.id) { + console.error( + `Received a new connection for client ${client.id}, but the old connection was not cleaned up`, + ); + return false; + } + return true; + }); + newClients.push(payload); + return updateSelection({ ...state, - clients: state.clients.concat(payload), + clients: newClients, uninitializedClients: state.uninitializedClients.filter((c) => { return ( c.deviceId !== payload.query.device_id || @@ -306,11 +317,13 @@ export default (state: State = INITAL_STATE, action: Actions): State => { case 'CLIENT_REMOVED': { const {payload} = action; + + const newClients = state.clients.filter( + (client) => client.id !== payload, + ); return updateSelection({ ...state, - clients: state.clients.filter( - (client: Client) => client.id !== payload, - ), + clients: newClients, }); } diff --git a/desktop/app/src/reducers/pluginMessageQueue.tsx b/desktop/app/src/reducers/pluginMessageQueue.tsx index 06f71a2d1..82662cb2b 100644 --- a/desktop/app/src/reducers/pluginMessageQueue.tsx +++ b/desktop/app/src/reducers/pluginMessageQueue.tsx @@ -38,7 +38,7 @@ export type Action = }; } | { - type: 'CLEAR_PLUGIN_STATE'; + type: 'CLEAR_CLIENT_PLUGINS_STATE'; payload: {clientId: string; devicePlugins: Set}; }; @@ -76,7 +76,7 @@ export default function reducer( }); } - case 'CLEAR_PLUGIN_STATE': { + case 'CLEAR_CLIENT_PLUGINS_STATE': { const {payload} = action; return Object.keys(state).reduce((newState: State, pluginKey) => { // Only add the pluginState, if its from a plugin other than the one that diff --git a/desktop/app/src/reducers/pluginStates.tsx b/desktop/app/src/reducers/pluginStates.tsx index 2a0e3a92a..5033d1b0d 100644 --- a/desktop/app/src/reducers/pluginStates.tsx +++ b/desktop/app/src/reducers/pluginStates.tsx @@ -27,7 +27,7 @@ export type Action = }; } | { - type: 'CLEAR_PLUGIN_STATE'; + type: 'CLEAR_CLIENT_PLUGINS_STATE'; payload: {clientId: string; devicePlugins: Set}; }; @@ -47,7 +47,7 @@ export default function reducer( }; } return {...state}; - } else if (action.type === 'CLEAR_PLUGIN_STATE') { + } else if (action.type === 'CLEAR_CLIENT_PLUGINS_STATE') { const {payload} = action; return Object.keys(state).reduce((newState: State, pluginKey) => { // Only add the pluginState, if its from a plugin other than the one that diff --git a/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx b/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx index e46cc2903..72b7d5223 100644 --- a/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx +++ b/desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx @@ -65,7 +65,7 @@ export function AppInspect() { {isArchived ? ( ) : ( diff --git a/desktop/app/src/server.tsx b/desktop/app/src/server.tsx index 802a37198..716d4d08f 100644 --- a/desktop/app/src/server.tsx +++ b/desktop/app/src/server.tsx @@ -601,7 +601,7 @@ class Server extends EventEmitter { removeConnection(id: string) { const info = this.connections.get(id); if (info) { - info.client.close(); + info.client.disconnect(); this.connections.delete(id); this.emit('clients-change'); this.emit('removed-client', id); diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 1fcc74fc6..00591a7e0 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -45,7 +45,12 @@ export type MockFlipperResult = { pluginKey: string; sendMessage(method: string, params: any, client?: Client): void; createDevice(serial: string): BaseDevice; - createClient(device: BaseDevice, name: string): Promise; + createClient( + device: BaseDevice, + name: string, + query?: ClientQuery, + skipRegister?: boolean, + ): Promise; logger: Logger; togglePlugin(plugin?: string): void; }; @@ -58,6 +63,7 @@ type MockOptions = Partial<{ onSend?: (pluginId: string, method: string, params?: object) => any; additionalPlugins?: PluginDefinition[]; dontEnableAdditionalPlugins?: true; + asBackgroundPlugin?: true; }>; export async function createMockFlipperWithPlugin( @@ -89,8 +95,10 @@ export async function createMockFlipperWithPlugin( async function createClient( device: BaseDevice, name: string, + query?: ClientQuery, + skipRegister?: boolean, ): Promise { - const query: ClientQuery = { + query = query ?? { app: name, os: 'Android', device: device.title, @@ -119,12 +127,6 @@ export async function createMockFlipperWithPlugin( device, ); - // yikes - client.device = { - then() { - return device; - }, - } as any; client.rawCall = async ( method: string, _fromPlugin: boolean, @@ -141,7 +143,7 @@ export async function createMockFlipperWithPlugin( plugins: [...store.getState().plugins.clientPlugins.keys()], }; case 'getBackgroundPlugins': - return {plugins: []}; + return {plugins: options?.asBackgroundPlugin ? [pluginClazz.id] : []}; default: throw new Error( `Test client doesn't support rawCall method '${method}'`, @@ -181,10 +183,12 @@ export async function createMockFlipperWithPlugin( await client.init(); // As convenience, by default we select the new client, star the plugin, and select it - store.dispatch({ - type: 'NEW_CLIENT', - payload: client, - }); + if (!skipRegister) { + store.dispatch({ + type: 'NEW_CLIENT', + payload: client, + }); + } return client; } diff --git a/desktop/app/src/utils/__tests__/messageQueue.node.tsx b/desktop/app/src/utils/__tests__/messageQueue.node.tsx index fe69a0a7b..a61eb65ea 100644 --- a/desktop/app/src/utils/__tests__/messageQueue.node.tsx +++ b/desktop/app/src/utils/__tests__/messageQueue.node.tsx @@ -432,7 +432,7 @@ test('queue - make sure resetting plugin state clears the message queue', async expect(store.getState().pluginMessageQueue[pluginKey].length).toBe(2); store.dispatch({ - type: 'CLEAR_PLUGIN_STATE', + type: 'CLEAR_CLIENT_PLUGINS_STATE', payload: {clientId: client.id, devicePlugins: new Set()}, }); diff --git a/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx b/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx index 19e23e1af..8604ac9e4 100644 --- a/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx +++ b/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx @@ -505,7 +505,7 @@ test('queue - make sure resetting plugin state clears the message queue', async expect(store.getState().pluginMessageQueue[pluginKey].length).toBe(2); store.dispatch({ - type: 'CLEAR_PLUGIN_STATE', + type: 'CLEAR_CLIENT_PLUGINS_STATE', payload: {clientId: client.id, devicePlugins: new Set()}, });