jens/flipper
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't kill WS connection if individual message handling fails

Browse Source 
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
This commit is contained in:
Michel Weststrate
2022-08-18 06:58:59 -07:00
committed by Facebook GitHub Bot
parent c863af6942
commit 80d3659e3f
2 changed files with 10 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
desktop/flipper-server-core/src/comms/ServerWebSocket.tsx
View File

@@ -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);
}
});
}

9
desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx
View File

@@ -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);
});
});