Preserve client state after disconnect

Summary:
This diff introduces support for keeping clients around after they have disconnected. This is a pretty important debugging improvement, that will allow inspecting a device / app after it crashed for example.

With this diff, the current client is just kept around until it connects again, instead of throwing clients immediately away if they disconnect. After this change, ArchivedClients will only be created by imports / exports, and no longer by disconnects. Initially I played with improving the creation of archived devices, by migrating all plugin state over from the original client to the archive, but I discovered that is very prone, as it would be a lot of pointer redistribution (plugins would point to a different client / device etc). While in contrast, disconnected clients is already an existing concept in Flipper, so reusing that keeps all the changes relatively simple.

Note that we could potentially still reuse old clients around after reconnected, but it would become much harder to reason about how plugins would behave if they missed updates for a while, so throwing away the device / clients and starting with a fresh slate sounds safer. So I figured that chance to be too risky for now, but would probably be good follow up work.

Issues with import / export, UX, and making calls to to a disconnected client will be addressed in follow up diffs

Changelog: Clients will retain their state after being disconnected, until they reconnect again

Reviewed By: nikoant

Differential Revision: D26224677

fbshipit-source-id: feb9d241df2304341c2847fe7fd751ac54c045f6

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<D, M> {
 }
 
 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));
   }
 

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);
+});

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';

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();
     });

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();
     });
   }
 

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();
   });
 }
 

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';
 

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,
+  });
+}

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(

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();
 });

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,
       });
     }
 

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<string>};
     };
 
@@ -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

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<string>};
     };
 
@@ -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

desktop/app/src/sandy-chrome/appinspect/AppInspect.tsx

@@ -65,7 +65,7 @@ export function AppInspect() {
             <AppSelector />
             {isArchived ? (
               <Alert
-                message="This device is a snapshot and cannot be interacted with."
+                message="This device is a offline and cannot be interacted with."
                 type="info"
               />
             ) : (

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);

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<Client>;
+  createClient(
+    device: BaseDevice,
+    name: string,
+    query?: ClientQuery,
+    skipRegister?: boolean,
+  ): Promise<Client>;
   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<Client> {
-    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
+    if (!skipRegister) {
       store.dispatch({
         type: 'NEW_CLIENT',
         payload: client,
       });
+    }
     return client;
   }
 

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()},
   });
 

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()},
   });