From 087281ac013afe85c6c60d8d3df30930c0504566 Mon Sep 17 00:00:00 2001
From: Vitali Zaidman <vzaidman@meta.com>
Date: Mon, 17 Feb 2025 01:52:22 -0800
Subject: [PATCH] keep heartbeat between inspector proxy and debugger going
 even if not idle (#49441)

Summary:

Changelog:
[General][Internal] - keep heartbeat between inspector proxy and debugger going even if not idle

When heartbeat was only used to keep the connection alive and to detect debugger timeouts, it was enough to send a ping every time the connection was idle for 10 seconds.

Now, when we use the heartbeat as a way to track how good is the round trip time between the inspector proxy and the debugger, we would like to make this tracking more reliable by sending a ping 10 seconds after each pong, even if the connection is not idle.

It also simplifies the code and makes it more clear by removing the confusing `shouldSetTerminateTimeout` variable.

Differential Revision: D69665738
---
 .../src/inspector-proxy/InspectorProxy.js     | 74 +++++++++----------
 1 file changed, 33 insertions(+), 41 deletions(-)

diff --git a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js
index 536bae1ccb3a..8ee02753201f 100644
--- a/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js
+++ b/packages/dev-middleware/src/inspector-proxy/InspectorProxy.js
@@ -359,7 +359,6 @@ export default class InspectorProxy implements InspectorProxyQueries {
   //
   // https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.2
   #startHeartbeat(socket: WS, intervalMs: number, appId: string) {
-    let shouldSetTerminateTimeout = false;
     let terminateTimeout = null;
     let latestPingMs = Date.now();
 
@@ -370,58 +369,52 @@ export default class InspectorProxy implements InspectorProxyQueries {
         return;
       }
 
-      shouldSetTerminateTimeout = true;
       latestPingMs = Date.now();
+
       socket.ping(() => {
-        if (!shouldSetTerminateTimeout) {
-          // Sometimes, this `sent` callback fires later than
-          // the actual pong reply.
-          //
-          // If any message came in between ping `sending` and `sent`,
-          // then the connection exists; and we don't need to do anything.
-          return;
+        if (!terminateTimeout) {
+          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();
+            this.#logger?.error(
+              `Connection terminated with DevTools after not responding for ${MAX_PONG_LATENCY_MS / 1000} seconds.`,
+            );
+            this.#eventReporter?.logEvent({
+              type: 'debugger_timeout',
+              duration: MAX_PONG_LATENCY_MS,
+              appId,
+            });
+          }, MAX_PONG_LATENCY_MS).unref();
         }
-
-        shouldSetTerminateTimeout = false;
-        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();
-          this.#logger?.error(
-            `Connection terminated with DevTools after not responding for ${MAX_PONG_LATENCY_MS / 1000} seconds.`,
-          );
-          this.#eventReporter?.logEvent({
-            type: 'debugger_timeout',
-            duration: MAX_PONG_LATENCY_MS,
-            appId,
-          });
-        }, MAX_PONG_LATENCY_MS).unref();
       });
     }, intervalMs).unref();
 
-    const onAnyMessageFromDebugger = () => {
-      shouldSetTerminateTimeout = false;
-      terminateTimeout && clearTimeout(terminateTimeout);
-      pingTimeout.refresh();
-    };
-
     socket.on('pong', () => {
-      onAnyMessageFromDebugger();
+      const duration = Date.now() - latestPingMs;
+
+      debug(`debugger ping-pong for ${appId} took ${duration}ms.`);
+
       this.#eventReporter?.logEvent({
         type: 'debugger_heartbeat',
-        duration: Date.now() - latestPingMs,
+        duration,
         appId,
       });
+
+      terminateTimeout?.refresh();
+      pingTimeout.refresh();
     });
+
     socket.on('message', () => {
-      onAnyMessageFromDebugger();
+      terminateTimeout?.refresh();
     });
 
     socket.on('close', (code: number, reason: string) => {
@@ -430,7 +423,6 @@ export default class InspectorProxy implements InspectorProxyQueries {
         String(code),
         reason,
       );
-      shouldSetTerminateTimeout = false;
       terminateTimeout && clearTimeout(terminateTimeout);
       clearTimeout(pingTimeout);
     });