fix timing of terminating an unresponsive debugger socket #44811

EdmondChuiHW · 2024-06-06T01:22:12Z

Summary:
Changelog:
[General][Fixed] - Debugger frontend socket-termination countdown now begins after the ping message is actually sent

The debugger is currently disconnected if a ping-pong message is missed.

This causes the debugger to be unusable if it happens to be lagging, e.g. when the initialisation is competing with the flood of log spam T191394188

There are a few ways to fix this as discused with motiz88 and robhogan:

Ensure the websocket has a chance to respond, e.g. in via web worker
Lengthen the time allowed for the pong resopnse

I've done some digging to find the root cause of the UI being blocked in CDT, However, profiling shows that most of the work is not simple to break up, i.e. the number of expensive re-layout calls. Diving into that rabbit hole could mean accidentally writing React.

Because we ping every 10 seconds, we could get un/lucky where CDT happens to be busy at that exact moment, making this a flaky symptom to fix, even if we lengthen the allowed time-to-respond.

V2

So upon further investigation, CDT websocket is actually responding to the pings in due time:

{F1679132204}

(CDT doesn't show the ping/pong API as frames, so a custom tick/tock message was used to visualise the timing)

Over here in dev-middleware, we currently start a timeout to terminate the socket after sending the ping:

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

Lines 306 to 307 in 2483c63

    
           socket.ping(); 
        
           terminateTimeout = setTimeout(() => {

If CDT doesn't respond in time, websocket would be terminated.

But we saw CDT respond immediately above, even during the log spam, so the delay must be coming from somewhere else.

The intuition is that during the log-spam, the middleware takes a perf hit too when it's processing the spam from the device and forwarding it to the CDT websocket.

We can confirm this by passing a "sent" callback via socket.ping(cb):

https://github.com/websockets/ws/blob/9bdb58070d64c33a9beeac7c732aac0f4e7e18b7/lib/websocket.js#L246-L254

This gives us the timing between calling socket.ping() and when the ping is actually sent.

Regular, stress-free operation without log-spam shows most pings are sent within the same millisecond:

{F1679223326}

With the pong response grace period at 5 seconds, there's plenty of time for CDT to pong back. That's why it has been working in most cases.

However, during the log-spam, we easily see this send-sent delay over 5 seconds. In extreme cases, almost 30 seconds would have passed before middleware sent a message to CDT, which then responded under 2 seconds:

{F1679163335}

This means while CDT is getting flooded and has observable lag in the UI, the smoking gun is actually the middleware.

Digging a little deeper, we know that incoming messages from the target goes into a Promise queue, including the console logs:

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

Lines 155 to 157 in 2483c63

    
           this.#deviceSocket.on('message', (message: string) => { 
        
             this.#messageFromDeviceQueue = this.#messageFromDeviceQueue 
        
               .then(async () => {

This means during the flood of logs from the target, the Promise queue keeps getting chained rapidly for each message.

Meanhile, the ws lib uses the underlying NodeJS Socket.write method for ping(…) and send(…):

https://github.com/websockets/ws/blob/9bdb58070d64c33a9beeac7c732aac0f4e7e18b7/lib/sender.js#L349

…which is guaranteed to fire the callback asynchronously:

nodejs/help#1504 (comment)

Promise queue is in the macro task queue, which gets priority before the micro task queue. So if the Promise queue is not cleared yet, the websocket queue will have a hard time getting executed in time – explaining the extreme send-sent durations during a log spam.

The fix is simple:

Start the terminate-socket-timer until the ping is actually sent
Treat any incoming message (along with pongs) as a terminate-socket-timer reset
1. This also applies if pong comes in between send and sent, which can happen sometimes due to the async nature of the callback:

{F1679288626}

V1

~~In this diff, a more forgiving mechanism is introduced, i.e. CDT is allowed to miss a ping-pong roundtrip 3 times before the websocket connection is terminated.~~

~~This allows a bit more breathing room for CDT's initialisation during log spam while maintaining the same ping-pong interval for VS Code to keep the auto SSH tunnel alive.~~

Differential Revision: D58220230

facebook-github-bot · 2024-06-06T01:22:21Z