Reuse Device instances when handing off connections based on device ID (facebook#45027)

Summary:
Pull Request resolved: facebook#45027

Changelog: [Internal]

Changes the device ID collision handling logic to reuse `Device` instances instead of creating new ones. This enables further refactoring of `Device` to improve session state isolation.

Reviewed By: hoxyq

Differential Revision: D58724884

fbshipit-source-id: bc11ce45ce8c80c58c32dcd1b07b28f1d1753a62

Author: motiz88

packages/dev-middleware/src/inspector-proxy/Device.js

@@ -29,6 +29,7 @@ import type {
 
 import DeviceEventReporter from './DeviceEventReporter';
 import * as fs from 'fs';
+import invariant from 'invariant';
 import fetch from 'node-fetch';
 import * as path from 'path';
 import WS from 'ws';
@@ -98,7 +99,7 @@ export default class Device {
   #deviceSocket: WS;
 
   // Stores the most recent listing of device's pages, keyed by the `id` field.
-  #pages: $ReadOnlyMap<string, Page>;
+  #pages: $ReadOnlyMap<string, Page> = new Map();
 
   // Stores information about currently connected debugger (if any).
   #debuggerConnection: ?DebuggerConnection = null;
@@ -135,11 +136,30 @@ export default class Device {
     projectRoot: string,
     eventReporter: ?EventReporter,
     createMessageMiddleware: ?CreateCustomMessageHandlerFn,
+  ) {
+    this.#dangerouslyConstruct(
+      id,
+      name,
+      app,
+      socket,
+      projectRoot,
+      eventReporter,
+      createMessageMiddleware,
+    );
+  }
+
+  #dangerouslyConstruct(
+    id: string,
+    name: string,
+    app: string,
+    socket: WS,
+    projectRoot: string,
+    eventReporter: ?EventReporter,
+    createMessageMiddleware: ?CreateCustomMessageHandlerFn,
   ) {
     this.#id = id;
     this.#name = name;
     this.#app = app;
-    this.#pages = new Map();
     this.#deviceSocket = socket;
     this.#projectRoot = projectRoot;
     this.#deviceEventReporter = eventReporter
@@ -192,16 +212,67 @@ export default class Device {
       PAGES_POLLING_INTERVAL,
     );
     this.#deviceSocket.on('close', () => {
-      this.#deviceEventReporter?.logDisconnection('device');
-      // Device disconnected - close debugger connection.
-      if (this.#debuggerConnection) {
-        this.#debuggerConnection.socket.close();
-        this.#debuggerConnection = null;
+      if (socket === this.#deviceSocket) {
+        this.#deviceEventReporter?.logDisconnection('device');
+        // Device disconnected - close debugger connection.
+        if (this.#debuggerConnection) {
+          this.#debuggerConnection.socket.close();
+          this.#debuggerConnection = null;
+        }
+        clearInterval(this.#pagesPollingIntervalId);
       }
-      clearInterval(this.#pagesPollingIntervalId);
     });
   }
 
+  /**
+   * Used to recreate the device connection if there is a device ID collision.
+   * 1. Checks if the same device is attempting to reconnect for the same app.
+   * 2. If not, close both the device and debugger socket.
+   * 3. If the debugger connection can be reused, close the device socket only.
+   *
+   * This hack attempts to allow users to reload the app, either as result of a
+   * crash, or manually reloading, without having to restart the debugger.
+   */
+  dangerouslyRecreateDevice(
+    id: string,
+    name: string,
+    app: string,
+    socket: WS,
+    projectRoot: string,
+    eventReporter: ?EventReporter,
+    createMessageMiddleware: ?CreateCustomMessageHandlerFn,
+  ) {
+    invariant(
+      id === this.#id,
+      'dangerouslyRecreateDevice() can only be used for the same device ID',
+    );
+    if (this.#app !== app || this.#name !== name) {
+      this.#deviceSocket.close();
+      this.#debuggerConnection?.socket.close();
+    }
+
+    const oldDebugger = this.#debuggerConnection;
+    this.#debuggerConnection = null;
+
+    if (oldDebugger) {
+      oldDebugger.socket.removeAllListeners();
+      this.#deviceSocket.close();
+      this.handleDebuggerConnection(oldDebugger.socket, oldDebugger.pageId, {
+        userAgent: oldDebugger.userAgent,
+      });
+    }
+
+    this.#dangerouslyConstruct(
+      id,
+      name,
+      app,
+      socket,
+      projectRoot,
+      eventReporter,
+      createMessageMiddleware,
+    );
+  }
+
   getName(): string {
     return this.#name;
   }
@@ -373,40 +444,6 @@ export default class Device {
     };
   }
 
-  /**
-   * Handles cleaning up a duplicate device connection, by client-side device ID.
-   * 1. Checks if the same device is attempting to reconnect for the same app.
-   * 2. If not, close both the device and debugger socket.
-   * 3. If the debugger connection can be reused, close the device socket only.
-   *
-   * This allows users to reload the app, either as result of a crash, or manually
-   * reloading, without having to restart the debugger.
-   */
-  handleDuplicateDeviceConnection(newDevice: Device) {
-    if (
-      this.#app !== newDevice.getApp() ||
-      this.#name !== newDevice.getName()
-    ) {
-      this.#deviceSocket.close();
-      this.#debuggerConnection?.socket.close();
-    }
-
-    const oldDebugger = this.#debuggerConnection;
-    this.#debuggerConnection = null;
-
-    if (oldDebugger) {
-      oldDebugger.socket.removeAllListeners();
-      this.#deviceSocket.close();
-      newDevice.handleDebuggerConnection(
-        oldDebugger.socket,
-        oldDebugger.pageId,
-        {
-          userAgent: oldDebugger.userAgent,
-        },
-      );
-    }
-  }
-
   /**
    * Returns `true` if a page supports the given target capability flag.
    */
@@ -927,4 +964,8 @@ export default class Device {
 
     return this.#pageHasCapability(page, 'prefersFuseboxFrontend');
   }
+
+  dangerouslyGetSocket(): WS {
+    return this.#deviceSocket;
+  }
 }

packages/dev-middleware/src/inspector-proxy/InspectorProxy.js

@@ -209,18 +209,28 @@ export default class InspectorProxy implements InspectorProxyQueries {
         const appName = query.app || 'Unknown';
 
         const oldDevice = this.#devices.get(deviceId);
-        const newDevice = new Device(
-          deviceId,
-          deviceName,
-          appName,
-          socket,
-          this.#projectRoot,
-          this.#eventReporter,
-          this.#customMessageHandler,
-        );
-
+        let newDevice;
         if (oldDevice) {
-          oldDevice.handleDuplicateDeviceConnection(newDevice);
+          oldDevice.dangerouslyRecreateDevice(
+            deviceId,
+            deviceName,
+            appName,
+            socket,
+            this.#projectRoot,
+            this.#eventReporter,
+            this.#customMessageHandler,
+          );
+          newDevice = oldDevice;
+        } else {
+          newDevice = new Device(
+            deviceId,
+            deviceName,
+            appName,
+            socket,
+            this.#projectRoot,
+            this.#eventReporter,
+            this.#customMessageHandler,
+          );
         }
 
         this.#devices.set(deviceId, newDevice);
@@ -230,7 +240,7 @@ export default class InspectorProxy implements InspectorProxyQueries {
         );
 
         socket.on('close', () => {
-          if (this.#devices.get(deviceId) === newDevice) {
+          if (this.#devices.get(deviceId)?.dangerouslyGetSocket() === socket) {
             this.#devices.delete(deviceId);
           }
           debug(`Device ${deviceName} disconnected.`);