From 6695b6e3b87e825cd283a2cf07d7eac08038342c Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Tue, 18 Jun 2024 09:39:35 -0700 Subject: [PATCH] Reuse Device instances when handing off connections based on device ID (#45027) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- .../src/inspector-proxy/Device.js | 125 ++++++++++++------ .../src/inspector-proxy/InspectorProxy.js | 34 +++-- 2 files changed, 105 insertions(+), 54 deletions(-) diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index d43654c5a50..fb1367dc26c 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/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; + #pages: $ReadOnlyMap = 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; + } } diff --git a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js index 717cc8c2987..d33750b3ecd 100644 --- a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js +++ b/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.`);