From f85def32fb62bec82eb84de6e4786615edaa2c55 Mon Sep 17 00:00:00 2001 From: Andrey Goncharov Date: Thu, 10 Mar 2022 10:31:24 -0800 Subject: [PATCH] Migrate from socket.io Reviewed By: passy Differential Revision: D34787674 fbshipit-source-id: 63d7c166ea29d14c96f0646a045e3f6fa93472e2 --- desktop/flipper-common/src/index.tsx | 1 + desktop/flipper-common/src/transport.tsx | 49 +++++++ desktop/flipper-server/package.json | 4 +- .../flipper-server/src/startBaseServer.tsx | 89 ++++++++----- .../flipper-server/src/startSocketServer.tsx | 94 +++++++++---- .../flipper-server/src/startWebServerDev.tsx | 10 +- desktop/flipper-ui-browser/package.json | 7 +- .../src/flipperServerConnection.tsx | 104 +++++++++------ desktop/patches/metro+0.69.0.patch | 13 ++ desktop/static/index.web.dev.html | 21 +-- desktop/yarn.lock | 125 +++++------------- 11 files changed, 307 insertions(+), 210 deletions(-) create mode 100644 desktop/flipper-common/src/transport.tsx diff --git a/desktop/flipper-common/src/index.tsx b/desktop/flipper-common/src/index.tsx index 8cd13aca4..e5e1e1214 100644 --- a/desktop/flipper-common/src/index.tsx +++ b/desktop/flipper-common/src/index.tsx @@ -54,3 +54,4 @@ export * from './settings'; export * from './PluginDetails'; export * from './doctor'; export * from './ServerAddOn'; +export * from './transport'; diff --git a/desktop/flipper-common/src/transport.tsx b/desktop/flipper-common/src/transport.tsx new file mode 100644 index 000000000..ee86af0f0 --- /dev/null +++ b/desktop/flipper-common/src/transport.tsx @@ -0,0 +1,49 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +import {FlipperServerCommands, FlipperServerEvents} from './server-types'; + +type GenericWebSocketMessage = { + event: E; + payload: T; +}; + +export type ExecWebSocketMessage = GenericWebSocketMessage< + 'exec', + { + [K in keyof FlipperServerCommands]: { + id: number; + command: K; + args: Parameters; + }; + }[keyof FlipperServerCommands] +>; + +export type ExecResponseWebSocketMessage = GenericWebSocketMessage< + 'exec-response', + {id: number; data: unknown} +>; + +export type ExecResponseErrorWebSocketMessage = GenericWebSocketMessage< + 'exec-response-error', + {id: number; data: unknown} +>; + +export type ServerEventWebSocketMessage = GenericWebSocketMessage< + 'server-event', + { + [K in keyof FlipperServerEvents]: {event: K; data: FlipperServerEvents[K]}; + }[keyof FlipperServerEvents] +>; + +export type ClientWebSocketMessage = ExecWebSocketMessage; +export type ServerWebSocketMessage = + | ExecResponseWebSocketMessage + | ExecResponseErrorWebSocketMessage + | ServerEventWebSocketMessage; diff --git a/desktop/flipper-server/package.json b/desktop/flipper-server/package.json index e48a03eea..1b97ff25c 100644 --- a/desktop/flipper-server/package.json +++ b/desktop/flipper-server/package.json @@ -11,7 +11,8 @@ "dependenciesComment": "mac-ca is required dynamically for darwin, node-fetch is treated special in electron-requires, not sure why", "dependencies": { "mac-ca": "^1.0.6", - "node-fetch": "^2.6.7" + "node-fetch": "^2.6.7", + "ws": "^8.5.0" }, "devDependencies": { "@types/express": "^4.17.13", @@ -25,7 +26,6 @@ "metro": "^0.69.0", "open": "^8.3.0", "p-filter": "^2.1.0", - "socket.io": "^4.4.1", "yargs": "^17.0.1" }, "peerDependencies": {}, diff --git a/desktop/flipper-server/src/startBaseServer.tsx b/desktop/flipper-server/src/startBaseServer.tsx index fb1394551..05e4fea8c 100644 --- a/desktop/flipper-server/src/startBaseServer.tsx +++ b/desktop/flipper-server/src/startBaseServer.tsx @@ -11,8 +11,9 @@ import express, {Express} from 'express'; import http from 'http'; import path from 'path'; import fs from 'fs-extra'; -import socketio from 'socket.io'; +import {VerifyClientCallbackSync, WebSocketServer} from 'ws'; import {WEBSOCKET_MAX_MESSAGE_SIZE} from 'flipper-server-core'; +import {parse} from 'url'; type Config = { port: number; @@ -23,7 +24,7 @@ type Config = { export async function startBaseServer(config: Config): Promise<{ app: Express; server: http.Server; - socket: socketio.Server; + socket: WebSocketServer; }> { const {app, server} = await startAssetServer(config); const socket = addWebsocket(server, config); @@ -67,36 +68,60 @@ function addWebsocket(server: http.Server, config: Config) { const localhostIPV6NoBrackets = `::1:${config.port}`; const possibleHosts = [localhostIPV4, localhostIPV6, localhostIPV6NoBrackets]; + const possibleOrigins = possibleHosts.map((host) => `http://${host}`); - const io = new socketio.Server(server, { - maxHttpBufferSize: WEBSOCKET_MAX_MESSAGE_SIZE, - allowRequest(req, callback) { - const noOriginHeader = req.headers.origin === undefined; - if ( - noOriginHeader && - req.headers.host && - possibleHosts.includes(req.headers.host) - ) { - // no origin header? Either the request is not cross-origin, - // or the request is not originating from a browser, so should be OK to pass through - callback(null, true); - } else { - // for now we don't allow cross origin request, so that an arbitrary website cannot try to - // connect a socket to localhost:serverport, and try to use the all powerful Flipper APIs to read - // for example files. - // Potentially in the future we do want to allow this, e.g. if we want to connect to a local flipper-server - // directly from intern. But before that, we should either authenticate the request somehow, - // and discuss security impact and for example scope the files that can be read by Flipper. - console.warn( - `Refused sockect connection from cross domain request, origin: ${ - req.headers.origin - }, host: ${req.headers.host}. Expected: ${possibleHosts.join( - ' or ', - )}`, - ); - callback(null, false); - } - }, + const verifyClient: VerifyClientCallbackSync = ({origin, req}) => { + const noOriginHeader = origin === undefined; + if ( + (noOriginHeader || possibleOrigins.includes(origin)) && + req.headers.host && + possibleHosts.includes(req.headers.host) + ) { + // no origin header? The request is not originating from a browser, so should be OK to pass through + // If origin matches our own address, it means we are serving the page + return true; + } else { + // for now we don't allow cross origin request, so that an arbitrary website cannot try to + // connect a socket to localhost:serverport, and try to use the all powerful Flipper APIs to read + // for example files. + // Potentially in the future we do want to allow this, e.g. if we want to connect to a local flipper-server + // directly from intern. But before that, we should either authenticate the request somehow, + // and discuss security impact and for example scope the files that can be read by Flipper. + console.warn( + `Refused socket connection from cross domain request, origin: ${origin}, host: ${ + req.headers.host + }. Expected origins: ${possibleOrigins.join( + ' or ', + )}. Expected hosts: ${possibleHosts.join(' or ')}`, + ); + return false; + } + }; + + const wss = new WebSocketServer({ + noServer: true, + maxPayload: WEBSOCKET_MAX_MESSAGE_SIZE, + verifyClient, }); - return io; + + server.on('upgrade', function upgrade(request, socket, head) { + const {pathname} = parse(request.url); + + // Handled by Metro + if (pathname === '/hot') { + return; + } + + if (pathname === '/') { + wss.handleUpgrade(request, socket, head, function done(ws) { + wss.emit('connection', ws, request); + }); + return; + } + + console.error('addWebsocket.upgrade -> unknown pathname', pathname); + socket.destroy(); + }); + + return wss; } diff --git a/desktop/flipper-server/src/startSocketServer.tsx b/desktop/flipper-server/src/startSocketServer.tsx index cfc5fcdf7..5f2ad7734 100644 --- a/desktop/flipper-server/src/startSocketServer.tsx +++ b/desktop/flipper-server/src/startSocketServer.tsx @@ -8,53 +8,91 @@ */ import chalk from 'chalk'; +import { + ClientWebSocketMessage, + ExecResponseWebSocketMessage, + ExecResponseErrorWebSocketMessage, + ServerEventWebSocketMessage, +} from 'flipper-common'; import {FlipperServerImpl} from 'flipper-server-core'; -import socketio from 'socket.io'; +import {WebSocketServer} from 'ws'; export function startSocketServer( flipperServer: FlipperServerImpl, - socket: socketio.Server, + socket: WebSocketServer, ) { - socket.on('connection', (client) => { - console.log(chalk.green(`Client connected ${client.id}`)); + socket.on('connection', (client, req) => { + const clientAddress = `${req.socket.remoteAddress}:${req.socket.remotePort}`; + + console.log(chalk.green(`Client connected ${clientAddress}`)); let connected = true; - function onServerEvent(event: string, payoad: any) { - client.emit('event', event, payoad); + function onServerEvent(event: string, payload: any) { + const message = { + event: 'server-event', + payload: { + event, + data: payload, + }, + } as ServerEventWebSocketMessage; + client.send(JSON.stringify(message)); } flipperServer.onAny(onServerEvent); - client.on('exec', (id, command, args) => { - flipperServer - .exec(command, ...args) - .then((result: any) => { - if (connected) { - client.emit('exec-response', id, result); - } - }) - .catch((error: any) => { - if (connected) { - // TODO: Serialize error - // TODO: log if verbose console.warn('Failed to handle response', error); - client.emit( - 'exec-response-error', - id, - error.toString() + (error.stack ? `\n${error.stack}` : ''), - ); - } - }); + client.on('message', (data) => { + const {event, payload} = JSON.parse( + data.toString(), + ) as ClientWebSocketMessage; + + switch (event) { + case 'exec': { + const {id, command, args} = payload; + + flipperServer + .exec(command, ...args) + .then((result: any) => { + if (connected) { + const response: ExecResponseWebSocketMessage = { + event: 'exec-response', + payload: { + id, + data: result, + }, + }; + + client.send(JSON.stringify(response)); + } + }) + .catch((error: any) => { + if (connected) { + // TODO: Serialize error + // TODO: log if verbose console.warn('Failed to handle response', error); + const responseError: ExecResponseErrorWebSocketMessage = { + event: 'exec-response-error', + payload: { + id, + data: + error.toString() + + (error.stack ? `\n${error.stack}` : ''), + }, + }; + client.send(JSON.stringify(responseError)); + } + }); + } + } }); - client.on('disconnect', () => { - console.log(chalk.red(`Client disconnected ${client.id}`)); + client.on('close', () => { + console.log(chalk.red(`Client disconnected ${clientAddress}`)); connected = false; flipperServer.offAny(onServerEvent); }); client.on('error', (e) => { - console.error(chalk.red(`Socket error ${client.id}`), e); + console.error(chalk.red(`Socket error ${clientAddress}`), e); connected = false; flipperServer.offAny(onServerEvent); }); diff --git a/desktop/flipper-server/src/startWebServerDev.tsx b/desktop/flipper-server/src/startWebServerDev.tsx index 81e6d2e86..c98faa76c 100644 --- a/desktop/flipper-server/src/startWebServerDev.tsx +++ b/desktop/flipper-server/src/startWebServerDev.tsx @@ -12,7 +12,7 @@ import {Express} from 'express'; import http from 'http'; import path from 'path'; import fs from 'fs-extra'; -import socketio from 'socket.io'; +import {WebSocketServer} from 'ws'; import pFilter from 'p-filter'; import {homedir} from 'os'; @@ -51,7 +51,7 @@ export async function getPluginSourceFolders(): Promise { export async function startWebServerDev( app: Express, server: http.Server, - socket: socketio.Server, + socket: WebSocketServer, rootDir: string, ) { // prevent bundling! @@ -138,7 +138,11 @@ export async function startWebServerDev( connectMiddleware.attachHmrServer(server); app.use(function (err: any, _req: any, _res: any, next: any) { console.error(chalk.red('\n\nCompile error in client bundle\n'), err); - socket.local.emit('hasErrors', err.toString()); + socket.clients.forEach((client) => { + client.send( + JSON.stringify({event: 'hasErrors', payload: err.toString()}), + ); + }); next(); }); diff --git a/desktop/flipper-ui-browser/package.json b/desktop/flipper-ui-browser/package.json index dec9481c5..1024f02e1 100644 --- a/desktop/flipper-ui-browser/package.json +++ b/desktop/flipper-ui-browser/package.json @@ -9,7 +9,9 @@ "types": "lib/index.d.ts", "license": "MIT", "bugs": "https://github.com/facebook/flipper/issues", - "dependencies": {}, + "dependencies": { + "reconnecting-websocket": "^4.4.0" + }, "devDependencies": { "eventemitter3": "^4.0.7", "flipper-common": "0.0.0", @@ -17,8 +19,7 @@ "invariant": "^2.2.4", "metro-runtime": "^0.69.0", "pretty-format": "^27.5.0", - "react-refresh": "^0.10.0", - "socket.io-client": "^4.4.1" + "react-refresh": "^0.10.0" }, "peerDependencies": {}, "scripts": { diff --git a/desktop/flipper-ui-browser/src/flipperServerConnection.tsx b/desktop/flipper-ui-browser/src/flipperServerConnection.tsx index 5eeaf754f..00a9eacf4 100644 --- a/desktop/flipper-ui-browser/src/flipperServerConnection.tsx +++ b/desktop/flipper-ui-browser/src/flipperServerConnection.tsx @@ -8,8 +8,12 @@ */ import EventEmitter from 'eventemitter3'; -import {FlipperServer} from 'flipper-common'; -import {io, Socket} from 'socket.io-client'; +import { + ExecWebSocketMessage, + FlipperServer, + ServerWebSocketMessage, +} from 'flipper-common'; +import ReconnectingWebSocket from 'reconnecting-websocket'; const CONNECTION_TIMEOUT = 30 * 1000; const EXEC_TIMOUT = 30 * 1000; @@ -18,7 +22,7 @@ export function createFlipperServer(): Promise { // TODO: polish this all! window.flipperShowError?.('Connecting to server...'); return new Promise((resolve, reject) => { - const initialConnectionTimeout = setTimeout(() => { + let initialConnectionTimeout: number | undefined = setTimeout(() => { reject( new Error('Failed to connect to Flipper server in a timely manner'), ); @@ -26,7 +30,7 @@ export function createFlipperServer(): Promise { const eventEmitter = new EventEmitter(); // TODO: recycle the socket that is created in index.web.dev.html? - const socket: Socket = io(); + const socket = new ReconnectingWebSocket(`ws://${location.host}`); const pendingRequests: Map< number, { @@ -38,19 +42,20 @@ export function createFlipperServer(): Promise { let requestId = 0; let connected = false; - socket.on('connect', () => { + socket.addEventListener('open', () => { + if (initialConnectionTimeout) { + // only relevant for the first connect + resolve(flipperServer); + clearTimeout(initialConnectionTimeout); + initialConnectionTimeout = undefined; + } + window?.flipperHideError?.(); console.log('Socket to Flipper server connected'); connected = true; }); - socket.once('connect', () => { - // only relevant for the first connect - resolve(flipperServer); - clearTimeout(initialConnectionTimeout); - }); - - socket.on('disconnect', () => { + socket.addEventListener('close', () => { window?.flipperShowError?.('WebSocket connection lost'); console.warn('Socket to Flipper server disconnected'); connected = false; @@ -60,35 +65,50 @@ export function createFlipperServer(): Promise { pendingRequests.clear(); }); - socket.on('exec-response', (id: number, data: any) => { - console.debug('exec <<<', id, data); - const entry = pendingRequests.get(id); - if (!entry) { - console.warn(`Unknown request id `, id); - } else { - pendingRequests.delete(id); - clearTimeout(entry.timeout); - entry.resolve(data); - } - }); + socket.addEventListener('message', ({data}) => { + const {event, payload} = JSON.parse( + data.toString(), + ) as ServerWebSocketMessage; - socket.on('exec-response-error', (id: number, error: any) => { - // TODO: Deserialize error - console.debug('exec <<< [SERVER ERROR]', id, error); - const entry = pendingRequests.get(id); - if (!entry) { - console.warn(`Unknown request id `, id); - } else { - pendingRequests.delete(id); - clearTimeout(entry.timeout); - entry.reject(error); + switch (event) { + case 'exec-response': { + console.debug('exec <<<', payload); + const entry = pendingRequests.get(payload.id); + if (!entry) { + console.warn(`Unknown request id `, payload.id); + } else { + pendingRequests.delete(payload.id); + clearTimeout(entry.timeout); + entry.resolve(payload.data); + } + break; + } + case 'exec-response-error': { + // TODO: Deserialize error + console.debug('exec <<< [SERVER ERROR]', payload.id, payload.data); + const entry = pendingRequests.get(payload.id); + if (!entry) { + console.warn(`Unknown request id `, payload.id); + } else { + pendingRequests.delete(payload.id); + clearTimeout(entry.timeout); + entry.reject(payload.data); + } + break; + } + case 'server-event': { + eventEmitter.emit(payload.event, payload.data); + break; + } + default: { + console.warn( + 'createFlipperServer -> unknown message', + data.toString(), + ); + } } }); - socket.on('event', (eventType, data) => { - eventEmitter.emit(eventType, data); - }); - const flipperServer: FlipperServer = { async connect() {}, close() {}, @@ -107,7 +127,15 @@ export function createFlipperServer(): Promise { }, EXEC_TIMOUT), }); - socket.emit('exec', id, command, args); + const execMessage = { + event: 'exec', + payload: { + id, + command, + args, + }, + } as ExecWebSocketMessage; + socket.send(JSON.stringify(execMessage)); }); // socket. } else { diff --git a/desktop/patches/metro+0.69.0.patch b/desktop/patches/metro+0.69.0.patch index e38813f97..47702761a 100644 --- a/desktop/patches/metro+0.69.0.patch +++ b/desktop/patches/metro+0.69.0.patch @@ -1,3 +1,16 @@ +diff --git a/node_modules/metro/src/index.js b/node_modules/metro/src/index.js +index 1c3a58e..96c7376 100644 +--- a/node_modules/metro/src/index.js ++++ b/node_modules/metro/src/index.js +@@ -115,8 +115,6 @@ const createConnectMiddleware = async function (config, options) { + wss.handleUpgrade(request, socket, head, (ws) => { + wss.emit("connection", ws, request); + }); +- } else { +- socket.destroy(); + } + }); + }, diff --git a/node_modules/metro/src/lib/getPreludeCode.js b/node_modules/metro/src/lib/getPreludeCode.js index 56780d7..7da26d1 100644 --- a/node_modules/metro/src/lib/getPreludeCode.js diff --git a/desktop/static/index.web.dev.html b/desktop/static/index.web.dev.html index e67640874..551340760 100644 --- a/desktop/static/index.web.dev.html +++ b/desktop/static/index.web.dev.html @@ -58,24 +58,27 @@ - -