From 4596b452fc2ad6b7b0459cf9b82aa5c4d927d7f6 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 15 Apr 2024 08:14:42 -0700 Subject: [PATCH 1/2] Inspector proxy: Add ping/pong keepalive/heartbeat to debugger connection (#44086) Summary: When a debugger frontend is connected to inspector-proxy via another proxy or tunnel that times out on idle (such as [VS Code's remote tunnel](https://github.com/microsoft/vscode/blob/main/src/vs/platform/tunnel/node/tunnelService.ts)), the connection between proxy and debugger may be dropped. In addition, when the connection is dropped without a closing handshake, the proxy does *not* detect the disconnection - no disconnect is logged to the reporter and no notifications are sent to any connected devices. This adds a mechanism using the WebSocket-standard `ping` and `pong` frames to: 1. Keep the connection alive 2. Detect when the debugger has gone away Note that as all WebSocket clients already **must** reply to a ping with a pong, this is non-breaking for compliant implementations: https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.2 Changelog: [General][Added] Inspector proxy: Add ping/pong keepalive to debugger connections. Reviewed By: hoxyq Differential Revision: D56069185 --- .../src/inspector-proxy/InspectorProxy.js | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js index 9b9916dc220d..6df32d2ea5ac 100644 --- a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js +++ b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js @@ -19,9 +19,14 @@ import type { PageDescription, } from './types'; import type {IncomingMessage, ServerResponse} from 'http'; +// $FlowFixMe[cannot-resolve-module] libdef missing in RN OSS +import type {Timeout} from 'timers'; import Device from './Device'; import nullthrows from 'nullthrows'; +// Import these from node:timers to get the correct Flow types. +// $FlowFixMe[cannot-resolve-module] libdef missing in RN OSS +import {clearTimeout, setTimeout} from 'timers'; import url from 'url'; import WS from 'ws'; @@ -32,6 +37,8 @@ const WS_DEBUGGER_URL = '/inspector/debug'; const PAGES_LIST_JSON_URL = '/json'; const PAGES_LIST_JSON_URL_2 = '/json/list'; const PAGES_LIST_JSON_VERSION_URL = '/json/version'; +const MAX_PONG_LATENCY_MS = 5000; +const DEBUGGER_HEARTBEAT_INTERVAL_MS = 10000; const INTERNAL_ERROR_CODE = 1011; @@ -264,6 +271,8 @@ export default class InspectorProxy implements InspectorProxyQueries { throw new Error('Unknown device with ID ' + deviceId); } + this.#startHeartbeat(socket, DEBUGGER_HEARTBEAT_INTERVAL_MS); + device.handleDebuggerConnection(socket, pageId, { userAgent: req.headers['user-agent'] ?? query.userAgent ?? null, }); @@ -279,4 +288,45 @@ export default class InspectorProxy implements InspectorProxyQueries { }); return wss; } + + // Starts pinging the socket at the given interval. Compliant clients will + // respond with pong frame. This serves both to detect when the client + // has gone away without sending a close frame, and as a keepalive in cases + // where proxies may drop idle connections (e.g., VS Code tunnels). + // + // https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.2 + #startHeartbeat(socket: WS, intervalMs: number) { + let terminateTimeout = null; + + const pingTimeout: Timeout = setTimeout(() => { + if (socket.readyState !== WS.OPEN) { + // May be connecting or closing, try again later. + pingTimeout.refresh(); + return; + } + socket.ping(); + terminateTimeout = setTimeout(() => { + if (socket.readyState !== WS.OPEN) { + return; + } + // We don't use close() here because that initiates a closing handshake, + // which will not complete if the other end has gone away - 'close' + // would not be emitted. + // + // terminate() emits 'close' immediately, allowing us to handle it and + // inform any clients. + socket.terminate(); + }, MAX_PONG_LATENCY_MS).unref(); + }, intervalMs).unref(); + + socket.on('pong', () => { + terminateTimeout && clearTimeout(terminateTimeout); + pingTimeout.refresh(); + }); + + socket.on('close', () => { + terminateTimeout && clearTimeout(terminateTimeout); + clearTimeout(pingTimeout); + }); + } } From 2c70268debec531346bd31133c93cdda8d96e5a0 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 15 Apr 2024 08:14:42 -0700 Subject: [PATCH 2/2] Inspector proxy: Rewrite 127.0.0.1 to localhost in source map URLs to support IPv4->IPv6 tunnels Summary: In a setup where a device retrieves a bundle from `http://127.0.0.1:8081`, but this is tunnelled to a remote host without an IPv6 stack (eg, FB dev servers), the host running the inspector-proxy will fail to fetch source or source maps from 127.0.0.1 despite typically being on the same host (indeed, process) as Metro. This causes a surprising inconsistency where using a bundler URL of `localhost` from the device results in source maps being inlined into `Debugger.scriptParsed`, but using a bundler URL of `127.0.0.1` causes inspector-proxy to fall back to preserving URLs, which are typically fetched lazily by CDT later. This should be unnecessary once we've implemented CDP `Network.loadNetworkResource` and removed `Debugger.scriptParsed` rewriting, but for now it brings IPv6 tunnelled servers in line with local servers. Changelog: [General][Changed] Inspector proxy: Rewrite 127.0.0.1 to localhost in source map URLs for better IPv4->IPv6 tunnelling support. Differential Revision: D56138742 --- .../InspectorProxyCdpRewritingHacks-test.js | 2 +- .../src/inspector-proxy/Device.js | 35 ++++++++++++------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js index b2d83c053d32..2f5eab91061b 100644 --- a/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js +++ b/packages/dev-middleware/src/__tests__/InspectorProxyCdpRewritingHacks-test.js @@ -192,7 +192,7 @@ describe.each(['HTTP', 'HTTPS'])( } }); - describe.each(['10.0.2.2', '10.0.3.2'])( + describe.each(['10.0.2.2', '10.0.3.2', '127.0.0.1'])( '%s aliasing to and from localhost', sourceHost => { test('in source map fetching during Debugger.scriptParsed', async () => { diff --git a/packages/dev-middleware/src/inspector-proxy/Device.js b/packages/dev-middleware/src/inspector-proxy/Device.js index b9481dac9106..1ecfd66f6535 100644 --- a/packages/dev-middleware/src/inspector-proxy/Device.js +++ b/packages/dev-middleware/src/inspector-proxy/Device.js @@ -37,8 +37,21 @@ const debug = require('debug')('Metro:InspectorProxy'); const PAGES_POLLING_INTERVAL = 1000; -// Android's stock emulator and other emulators such as genymotion use a standard localhost alias. -const EMULATOR_LOCALHOST_ADDRESSES: Array = ['10.0.2.2', '10.0.3.2']; +// Replace hosts appearing in the `url` and `sourceMapURL` fields of +// `Debugger.scriptParsed`, and back again in messages from the debugger, +// to account for device/debugger/proxy running on different networks. +const REWRITE_HOSTS_TO_LOCALHOST: Array = [ + // A device may retrieve a bundle through 127.0.0.1 via a (SSH) tunnel, but + // the (remote) Metro server may be on a host without an IPv4 loopback, so + // 127.0.0.1 may not be addressible locally for (e.g., for source map + // fetching). Replacing with the more general 'localhost' should always be + // safe while also more compatible with IPv6-only setups. + '127.0.0.1', + // Android's stock emulator and other emulators such as genymotion use a + // standard localhost alias. + '10.0.2.2', + '10.0.3.2', +]; // Prefix for script URLs that are alphanumeric IDs. See comment in #processMessageFromDeviceLegacy method for // more details. @@ -621,15 +634,14 @@ export default class Device { ) { const params = payload.params; if ('sourceMapURL' in params) { - for (let i = 0; i < EMULATOR_LOCALHOST_ADDRESSES.length; ++i) { - const address = EMULATOR_LOCALHOST_ADDRESSES[i]; - if (params.sourceMapURL.includes(address)) { + for (const hostToRewrite of REWRITE_HOSTS_TO_LOCALHOST) { + if (params.sourceMapURL.includes(hostToRewrite)) { // $FlowFixMe[cannot-write] payload.params.sourceMapURL = params.sourceMapURL.replace( - address, + hostToRewrite, 'localhost', ); - debuggerInfo.originalSourceURLAddress = address; + debuggerInfo.originalSourceURLAddress = hostToRewrite; } } @@ -654,12 +666,11 @@ export default class Device { } } if ('url' in params) { - for (let i = 0; i < EMULATOR_LOCALHOST_ADDRESSES.length; ++i) { - const address = EMULATOR_LOCALHOST_ADDRESSES[i]; - if (params.url.indexOf(address) >= 0) { + for (const hostToRewrite of REWRITE_HOSTS_TO_LOCALHOST) { + if (params.url.includes(hostToRewrite)) { // $FlowFixMe[cannot-write] - payload.params.url = params.url.replace(address, 'localhost'); - debuggerInfo.originalSourceURLAddress = address; + payload.params.url = params.url.replace(hostToRewrite, 'localhost'); + debuggerInfo.originalSourceURLAddress = hostToRewrite; } }