From 80d3659e3f9fabf291f1e7241126453cd57a4aa5 Mon Sep 17 00:00:00 2001
From: Michel Weststrate <mweststrate@fb.com>
Date: Thu, 18 Aug 2022 06:58:59 -0700
Subject: [PATCH] Don't kill WS connection if individual message handling fails

Summary:
GraphQL plugin sends garbage JSON at the moment (see T129428800). Still figuring out if that is caused by the plugin on client side, or that the Flipper SDK should escape that properly.

In the mean time, closing the Flipper connection on not being able to handle an individual message seems a cause of unnecessary connection ping-pong and instability. Changing the strategy to instead merely log and drop the message.

Changelog: Stop applications from disconnecting if a single plugin message cannot be processed.

Reviewed By: LukeDefeo

Differential Revision: D38825940

fbshipit-source-id: 4d45c8eea457375ec677238ad09b5e02bc2eb5bf
---
 .../flipper-server-core/src/comms/ServerWebSocket.tsx  | 10 ++++++----
 .../src/comms/__tests__/ServerWebSocket.node.tsx       |  9 ++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx b/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx
index 49b4a0b55..6960191eb 100644
--- a/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx
+++ b/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx
@@ -170,15 +170,17 @@ class ServerWebSocket extends ServerAdapter {
     this.handleConnectionAttempt(ctx);
 
     ws.on('message', async (message: WebSocket.RawData) => {
+      const messageString = message.toString();
       try {
-        const messageString = message.toString();
         const parsedMessage = this.handleMessageDeserialization(messageString);
         // Successful deserialization is a proof that the message is a string
         this.handleMessage(ctx, parsedMessage, messageString);
       } catch (error) {
-        // See the reasoning in the error handler for a `connection` event
-        ws.emit('error', error);
-        ws.close(WSCloseCode.InternalError);
+        // If handling an individual message failes, we don't necessarily kill the connection,
+        // all other plugins might still be working correctly. So let's just report it.
+        // This avoids ping-ponging connections if an individual plugin sends garbage (e.g. T129428800)
+        // or throws an error when handling messages
+        console.error('Failed to handle message', messageString, error);
       }
     });
   }
diff --git a/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx b/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx
index 2a244bd75..8b2c57a6e 100644
--- a/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx
+++ b/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx
@@ -100,7 +100,7 @@ describe('ServerWebSocket', () => {
     expect(mockSEListener.onError).toBeCalledTimes(0); // no onError triggered, as start throws already
   });
 
-  test('calls listener onError if a message handling fails', async () => {
+  test('does NOT call listener onError if individual message handling fails', async () => {
     const mockSEListener = createMockSEListener();
 
     server = new ServerWebSocket(mockSEListener);
@@ -117,9 +117,8 @@ describe('ServerWebSocket', () => {
     // Sending invalid JSON to cause a parsing error
     wsClient.send(`{{{{`);
     // Server must close the connection on error
-    await new Promise((resolve) => {
-      wsClient!.onclose = resolve;
-    });
-    expect(mockSEListener.onError).toBeCalledTimes(1);
+
+    expect(mockSEListener.onError).toBeCalledTimes(0);
+    expect(mockSEListener.onListening).toBeCalledTimes(1);
   });
 });