Make client -> device connection synchronous

Summary:
devices not always being readily available is causes a lot of complication in the api,
figured to resolve devices first before construction clients,
since clients not attached to a device are shown uncategorized anyway, making them practically un-interactable.
For more background info, see following chat.

{F344388883}

This diff will make it possible to only expose a synchronous api in Sandy

n.b. didn't update Navigation plugin, as that is done in a next diff

Reviewed By: jknoxville

Differential Revision: D24858332

fbshipit-source-id: 8339f831fbbc9c219add56a199364fde67adafc7

desktop/app/src/Client.tsx

@@ -21,7 +21,7 @@ import {setPluginState} from './reducers/pluginStates';
 import {Payload, ConnectionStatus} from 'rsocket-types';
 import {Flowable, Single} from 'rsocket-flowable';
 import {performance} from 'perf_hooks';
-import {reportPlatformFailures, reportPluginFailures} from './utils/metrics';
+import {reportPluginFailures} from './utils/metrics';
 import {notNull} from './utils/typeUtils';
 import {default as isProduction} from './utils/isProduction';
 import {registerPlugins} from './reducers/plugins';
@@ -33,7 +33,6 @@ import {
   defaultEnabledBackgroundPlugins,
 } from './utils/pluginUtils';
 import {processMessagesLater} from './utils/messageQueue';
-import {sideEffect} from './utils/sideEffect';
 import {emitBytesReceived} from './dispatcher/tracking';
 import {debounce} from 'lodash';
 import {batch} from 'react-redux';
@@ -134,11 +133,14 @@ export default class Client extends EventEmitter {
   connection: FlipperClientConnection<any, any> | null | undefined;
   store: Store;
   activePlugins: Set<string>;
+
+  /**
+   * @deprecated
+   * use plugin.deviceSync instead
+   */
   device: Promise<BaseDevice>;
-  _deviceResolve: (device: BaseDevice) => void = (_) => {};
+  deviceSync: BaseDevice;
-  _deviceResolved: BaseDevice | undefined;
   logger: Logger;
-  lastSeenDeviceList: Array<BaseDevice>;
   broadcastCallbacks: Map<string, Map<string, Set<Function>>>;
   messageBuffer: Record<
     string /*pluginKey*/,
@@ -168,8 +170,8 @@ export default class Client extends EventEmitter {
     conn: FlipperClientConnection<any, any> | null | undefined,
     logger: Logger,
     store: Store,
-    plugins?: Plugins | null | undefined,
+    plugins: Plugins | null | undefined,
-    device?: BaseDevice,
+    device: BaseDevice,
   ) {
     super();
     this.connected = true;
@@ -185,17 +187,9 @@ export default class Client extends EventEmitter {
     this.broadcastCallbacks = new Map();
     this.requestCallbacks = new Map();
     this.activePlugins = new Set();
-    this.lastSeenDeviceList = [];
 
-    this.device = device
+    this.device = Promise.resolve(device);
-      ? Promise.resolve(device)
+    this.deviceSync = device;
-      : new Promise((resolve, _reject) => {
-          this._deviceResolve = resolve;
-        });
-
-    if (device != null) {
-      this._deviceResolved = device;
-    }
 
     const client = this;
     if (conn) {
@@ -215,58 +209,6 @@ export default class Client extends EventEmitter {
     }
   }
 
-  /* All clients should have a corresponding Device in the store.
-     However, clients can connect before a device is registered, so wait a
-     while for the device to be registered if it isn't already. */
-  async setMatchingDevice(): Promise<void> {
-    return reportPlatformFailures(
-      new Promise<BaseDevice>((resolve, reject) => {
-        let unsubscribe: () => void = () => {};
-
-        const device = this.store
-          .getState()
-          .connections.devices.find(
-            (device) => device.serial === this.query.device_id,
-          );
-        if (device) {
-          this._deviceResolved = device;
-          resolve(device);
-          return;
-        }
-
-        const timeout = setTimeout(() => {
-          unsubscribe();
-          const error = `Timed out waiting for device for client ${this.id}`;
-          console.error(error);
-          reject(error);
-        }, 5000);
-        unsubscribe = sideEffect(
-          this.store,
-          {name: 'waitForDevice', throttleMs: 100},
-          (state) => state.connections.devices,
-          (newDeviceList) => {
-            if (newDeviceList === this.lastSeenDeviceList) {
-              return;
-            }
-            this.lastSeenDeviceList = newDeviceList;
-            const matchingDevice = newDeviceList.find(
-              (device) => device.serial === this.query.device_id,
-            );
-            if (matchingDevice) {
-              clearTimeout(timeout);
-              resolve(matchingDevice);
-              unsubscribe();
-            }
-          },
-        );
-      }),
-      'client-setMatchingDevice',
-    ).then((device) => {
-      this._deviceResolved = device;
-      this._deviceResolve(device);
-    });
-  }
-
   supportsPlugin(pluginId: string): boolean {
     return this.plugins.includes(pluginId);
   }
@@ -289,7 +231,6 @@ export default class Client extends EventEmitter {
   }
 
   async init() {
-    this.setMatchingDevice();
     await this.loadPlugins();
     // this starts all sandy enabled plugins
     this.plugins.forEach((pluginId) =>
@@ -434,20 +375,12 @@ export default class Client extends EventEmitter {
     this.emit('plugins-change');
   }
 
+  /**
+   * @deprecated
+   * use deviceSync.serial
+   */
   async deviceSerial(): Promise<string> {
-    try {
+    return this.deviceSync.serial;
-      const device = await this.device;
-      if (!device) {
-        console.error('Using "" for deviceId device is not ready');
-        return '';
-      }
-      return device.serial;
-    } catch (e) {
-      console.error(
-        'Using "" for deviceId because client has no matching device',
-      );
-      return '';
-    }
   }
 
   onMessage(msg: string) {
@@ -478,7 +411,7 @@ export default class Client extends EventEmitter {
       flipperMessagesClientPlugin.isConnected()
     ) {
       flipperMessagesClientPlugin.newMessage({
-        device: this._deviceResolved?.displayTitle(),
+        device: this.deviceSync?.displayTitle(),
         app: this.query.app,
         flipperInternalMethod: method,
         plugin: data.params?.api,
@@ -497,7 +430,7 @@ export default class Client extends EventEmitter {
           }: ${error.message} + \nDevice Stack Trace: ${error.stacktrace}`,
           'deviceError',
         );
-        this.device.then((device) => handleError(this.store, device, error));
+        handleError(this.store, this.deviceSync, error);
       } else if (method === 'refreshPlugins') {
         this.refreshPlugins();
       } else if (method === 'execute') {
@@ -576,7 +509,7 @@ export default class Client extends EventEmitter {
       reject(data.error);
       const {error} = data;
       if (error) {
-        this.device.then((device) => handleError(this.store, device, error));
+        handleError(this.store, this.deviceSync, error);
       }
     } else {
       // ???
@@ -670,7 +603,7 @@ export default class Client extends EventEmitter {
 
                   if (flipperMessagesClientPlugin.isConnected()) {
                     flipperMessagesClientPlugin.newMessage({
-                      device: this._deviceResolved?.displayTitle(),
+                      device: this.deviceSync?.displayTitle(),
                       app: this.query.app,
                       flipperInternalMethod: method,
                       payload: response,
@@ -692,7 +625,7 @@ export default class Client extends EventEmitter {
 
       if (flipperMessagesClientPlugin.isConnected()) {
         flipperMessagesClientPlugin.newMessage({
-          device: this._deviceResolved?.displayTitle(),
+          device: this.deviceSync?.displayTitle(),
           app: this.query.app,
           flipperInternalMethod: method,
           plugin: params?.api,
@@ -776,7 +709,7 @@ export default class Client extends EventEmitter {
 
     if (flipperMessagesClientPlugin.isConnected()) {
       flipperMessagesClientPlugin.newMessage({
-        device: this._deviceResolved?.displayTitle(),
+        device: this.deviceSync?.displayTitle(),
         app: this.query.app,
         flipperInternalMethod: method,
         payload: params,

desktop/app/src/chrome/__tests__/ExportDataPluginSheet.node.tsx

@@ -83,6 +83,7 @@ function getStore(selectedPlugins: Array<string>) {
     // @ts-ignore
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    selectedDevice,
   );
   mockStore.dispatch({
     type: 'NEW_CLIENT',

desktop/app/src/plugin.tsx

@@ -285,10 +285,17 @@ export class FlipperPlugin<
   client: PluginClient;
   realClient: Client;
 
+  /**
+   * @deprecated use .device instead
+   */
   getDevice(): Promise<BaseDevice> {
     return this.realClient.device;
   }
 
+  get device() {
+    return this.realClient.deviceSync;
+  }
+
   _teardown() {
     // automatically unsubscribe subscriptions
     const pluginId = this.constructor.id;

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

@@ -378,8 +378,8 @@ export function findBestDevice(
     return selected;
   }
   // if there is an active app, use device owning the app
-  if (client && client._deviceResolved) {
+  if (client) {
-    return client._deviceResolved;
+    return client.deviceSync;
   }
   // if no active app, use the preferred device
   if (userPreferredDevice) {

desktop/app/src/server.tsx

@@ -13,7 +13,7 @@ import {
 } from './utils/CertificateProvider';
 import {Logger} from './fb-interfaces/Logger';
 import {ClientQuery} from './Client';
-import {Store} from './reducers/index';
+import {Store, State} from './reducers/index';
 import CertificateProvider from './utils/CertificateProvider';
 import {RSocketServer} from 'rsocket-core';
 import RSocketTCPServer from 'rsocket-tcp-server';
@@ -38,6 +38,8 @@ import {IncomingMessage} from 'http';
 import ws from 'ws';
 import {initSelfInpector} from './utils/self-inspection/selfInspectionUtils';
 import ClientDevice from './devices/ClientDevice';
+import BaseDevice from './devices/BaseDevice';
+import {sideEffect} from './utils/sideEffect';
 
 type ClientInfo = {
   connection: FlipperClientConnection<any, any> | null | undefined;
@@ -529,7 +531,7 @@ class Server extends EventEmitter {
           );
         })
       : Promise.resolve(query.device_id)
-    ).then((csrId) => {
+    ).then(async (csrId) => {
       query.device_id = csrId;
       query.app = appNameWithUpdateHint(query);
 
@@ -541,7 +543,18 @@ class Server extends EventEmitter {
       });
       console.debug(`Device connected: ${id}`, 'server');
 
-      const client = new Client(id, query, conn, this.logger, this.store);
+      const device =
+        getDeviceBySerial(this.store.getState(), query.device_id) ??
+        (await findDeviceForConnection(this.store, query.app, query.device_id));
+      const client = new Client(
+        id,
+        query,
+        conn,
+        this.logger,
+        this.store,
+        undefined,
+        device,
+      );
 
       const info = {
         client,
@@ -627,4 +640,54 @@ class ConnectionTracker {
   }
 }
 
+function getDeviceBySerial(
+  state: State,
+  serial: string,
+): BaseDevice | undefined {
+  return state.connections.devices.find((device) => device.serial === serial);
+}
+
+async function findDeviceForConnection(
+  store: Store,
+  clientId: string,
+  serial: string,
+): Promise<BaseDevice> {
+  let lastSeenDeviceList: BaseDevice[] = [];
+  /* All clients should have a corresponding Device in the store.
+     However, clients can connect before a device is registered, so wait a
+     while for the device to be registered if it isn't already. */
+  return reportPlatformFailures(
+    new Promise<BaseDevice>((resolve, reject) => {
+      let unsubscribe: () => void = () => {};
+
+      const timeout = setTimeout(() => {
+        unsubscribe();
+        const error = `Timed out waiting for device ${serial} for client ${clientId}`;
+        console.error(error);
+        reject(error);
+      }, 5000);
+      unsubscribe = sideEffect(
+        store,
+        {name: 'waitForDevice', throttleMs: 100},
+        (state) => state.connections.devices,
+        (newDeviceList) => {
+          if (newDeviceList === lastSeenDeviceList) {
+            return;
+          }
+          lastSeenDeviceList = newDeviceList;
+          const matchingDevice = newDeviceList.find(
+            (device) => device.serial === serial,
+          );
+          if (matchingDevice) {
+            clearTimeout(timeout);
+            resolve(matchingDevice);
+            unsubscribe();
+          }
+        },
+      );
+    }),
+    'client-setMatchingDevice',
+  );
+}
+
 export default Server;

desktop/app/src/utils/__tests__/exportData.node.tsx

@@ -726,6 +726,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    device1,
   );
   const client2 = new Client(
     generateClientIdentifier(device1, 'app2'),
@@ -739,6 +740,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present
     logger,
     mockStore,
     ['TestDevicePlugin'],
+    device1,
   );
   const client3 = new Client(
     generateClientIdentifier(device1, 'app3'),
@@ -752,6 +754,7 @@ test('test determinePluginsToProcess for mutilple clients having plugins present
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    device1,
   );
   const plugins: PluginsState = {
     clientPlugins: new Map([
@@ -802,6 +805,7 @@ test('test determinePluginsToProcess for no selected plugin present in any clien
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    device1,
   );
   const client2 = new Client(
     generateClientIdentifier(device1, 'app2'),
@@ -815,6 +819,7 @@ test('test determinePluginsToProcess for no selected plugin present in any clien
     logger,
     mockStore,
     ['TestDevicePlugin'],
+    device1,
   );
   const plugins: PluginsState = {
     clientPlugins: new Map([
@@ -846,6 +851,7 @@ test('test determinePluginsToProcess for multiple clients on same device', async
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    device1,
   );
   const client2 = new Client(
     generateClientIdentifier(device1, 'app2'),
@@ -859,6 +865,7 @@ test('test determinePluginsToProcess for multiple clients on same device', async
     logger,
     mockStore,
     ['TestDevicePlugin'],
+    device1,
   );
   const plugins: PluginsState = {
     clientPlugins: new Map([['TestPlugin', TestPlugin]]),
@@ -897,6 +904,7 @@ test('test determinePluginsToProcess for multiple clients on different device',
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    device1,
   );
   const client2Device1 = new Client(
     generateClientIdentifier(device1, 'app2'),
@@ -910,6 +918,7 @@ test('test determinePluginsToProcess for multiple clients on different device',
     logger,
     mockStore,
     ['TestDevicePlugin'],
+    device1,
   );
   const client1Device2 = new Client(
     generateClientIdentifier(device2, 'app'),
@@ -923,6 +932,7 @@ test('test determinePluginsToProcess for multiple clients on different device',
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    device1,
   );
   const client2Device2 = new Client(
     generateClientIdentifier(device2, 'app2'),
@@ -936,6 +946,7 @@ test('test determinePluginsToProcess for multiple clients on different device',
     logger,
     mockStore,
     ['TestDevicePlugin'],
+    device1,
   );
   const plugins: PluginsState = {
     clientPlugins: new Map([['TestPlugin', TestPlugin]]),
@@ -999,6 +1010,7 @@ test('test determinePluginsToProcess to ignore archived clients', async () => {
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    archivedDevice,
   );
   const archivedClient = new Client(
     generateClientIdentifier(archivedDevice, 'app'),
@@ -1012,6 +1024,7 @@ test('test determinePluginsToProcess to ignore archived clients', async () => {
     logger,
     mockStore,
     ['TestPlugin', 'TestDevicePlugin'],
+    archivedDevice,
   );
   const plugins: PluginsState = {
     clientPlugins: new Map([['TestPlugin', TestPlugin]]),

desktop/app/src/utils/js-client-server-utils/serverUtils.tsx

@@ -44,9 +44,10 @@ export function initJsEmulatorIPC(
     (_event: IpcRendererEvent, message: any) => {
       const {windowId} = message;
       const {plugins, appName} = message.payload;
+      const device = new JSDevice(jsDeviceId(windowId), 'jsEmulator', windowId);
       store.dispatch({
         type: 'REGISTER_DEVICE',
-        payload: new JSDevice(jsDeviceId(windowId), 'jsEmulator', windowId),
+        payload: device,
       });
 
       const connection = new JSClientFlipperConnection(windowId);
@@ -69,6 +70,7 @@ export function initJsEmulatorIPC(
         logger,
         store,
         plugins,
+        device,
       );
 
       flipperConnections.set(clientId, {

desktop/app/src/utils/self-inspection/selfInspectionUtils.tsx

@@ -32,14 +32,15 @@ export function initSelfInpector(
 ) {
   const appName = 'Flipper';
   const device_id = 'FlipperSelfInspectionDevice';
+  const device = new FlipperSelfInspectionDevice(
+    device_id,
+    'emulator',
+    appName,
+    'JSWebApp',
+  );
   store.dispatch({
     type: 'REGISTER_DEVICE',
-    payload: new FlipperSelfInspectionDevice(
+    payload: device,
-      device_id,
-      'emulator',
-      appName,
-      'JSWebApp',
-    ),
   });
 
   selfInspectionClient.addPlugin(flipperMessagesClientPlugin);
@@ -59,6 +60,8 @@ export function initSelfInpector(
     selfInspectionClient,
     logger,
     store,
+    undefined,
+    device,
   );
 
   flipperConnections.set(clientId, {

desktop/plugins/layout/index.tsx

@@ -240,7 +240,7 @@ export default class LayoutPlugin extends FlipperPlugin<
     }
 
     if (this.props.isArchivedDevice) {
-      this.getDevice()
+      Promise.resolve(this.device)
         .then((d) => {
           const handle = (d as ArchivedDevice).getArchivedScreenshotHandle();
           if (!handle) {