From b6c884f011020b30c566d20e2b072381a2e63163 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 4 Jan 2022 02:56:33 -0800 Subject: [PATCH] Make sure flipper server initialization errors are propagated properly Summary: This diff makes sure that errors are propagated similarly in flipper desktop and browser version, and that they are shown in either case. Since in the browser version, the UI loads after the error happened, we'll store the error so that any client connecting in the future will read and report it. Also added a `--failFast` flag to flipper-server, so that the process exits immediately if misconfigured, which is convenient in CI use cases and such Reviewed By: nikoant Differential Revision: D33348922 fbshipit-source-id: 0f584104f881141fde38da3f0031748415343ea2 --- desktop/app/src/init.tsx | 1 - desktop/flipper-common/src/server-types.tsx | 6 +++- desktop/flipper-dump/src/index.tsx | 5 ++++ .../src/FlipperServerImpl.tsx | 12 ++++++-- .../src/comms/ServerController.tsx | 2 +- .../src/comms/ServerRSocket.tsx | 28 +++++++----------- .../src/comms/ServerWebSocket.tsx | 2 -- .../comms/__tests__/ServerWebSocket.node.tsx | 6 ++-- .../src/devices/android/adbClient.tsx | 9 ++++-- .../src/utils/CertificateProvider.tsx | 11 +++---- desktop/flipper-server/src/index.tsx | 26 +++++++++++++---- .../flipper-server/src/startFlipperServer.tsx | 5 +--- .../src/dispatcher/flipperServer.tsx | 29 +++++++++++++++++++ 13 files changed, 96 insertions(+), 46 deletions(-) diff --git a/desktop/app/src/init.tsx b/desktop/app/src/init.tsx index 7bd524c4c..bbb0948cb 100644 --- a/desktop/app/src/init.tsx +++ b/desktop/app/src/init.tsx @@ -16,7 +16,6 @@ import { } from 'flipper-plugin'; // eslint-disable-next-line no-restricted-imports,flipper/no-electron-remote-imports import {remote} from 'electron'; -import type {RenderHost} from 'flipper-ui-core'; import os from 'os'; import { FlipperServerImpl, diff --git a/desktop/flipper-common/src/server-types.tsx b/desktop/flipper-common/src/server-types.tsx index 03580989d..a4373c268 100644 --- a/desktop/flipper-common/src/server-types.tsx +++ b/desktop/flipper-common/src/server-types.tsx @@ -101,7 +101,7 @@ export type ClientResponseType = | {success?: never; error: ClientErrorType; length: number}; export type FlipperServerEvents = { - 'server-state': {state: FlipperServerState; error?: Error}; + 'server-state': {state: FlipperServerState; error?: string}; 'server-error': any; notification: { type: 'error'; @@ -152,6 +152,10 @@ export interface FSStatsLike { } export type FlipperServerCommands = { + 'get-server-state': () => Promise<{ + state: FlipperServerState; + error?: string; + }>; 'node-api-fs-access': (path: string, mode?: number) => Promise; 'node-api-fs-pathExists': (path: string, mode?: number) => Promise; 'node-api-fs-unlink': (path: string) => Promise; diff --git a/desktop/flipper-dump/src/index.tsx b/desktop/flipper-dump/src/index.tsx index 19ff700fb..f0ff60873 100644 --- a/desktop/flipper-dump/src/index.tsx +++ b/desktop/flipper-dump/src/index.tsx @@ -106,6 +106,11 @@ async function start(deviceTitle: string, appName: string, pluginId: string) { }); server.on('server-error', reject); + server.on('server-state', ({state, error}) => { + if (state === 'error') { + reject(error); + } + }); server.on('device-connected', (deviceInfo) => { logger.info( diff --git a/desktop/flipper-server-core/src/FlipperServerImpl.tsx b/desktop/flipper-server-core/src/FlipperServerImpl.tsx index 2021c60fd..8e51d17b2 100644 --- a/desktop/flipper-server-core/src/FlipperServerImpl.tsx +++ b/desktop/flipper-server-core/src/FlipperServerImpl.tsx @@ -70,6 +70,7 @@ export class FlipperServerImpl implements FlipperServer { readonly disposers: ((() => void) | void)[] = []; private readonly devices = new Map(); state: FlipperServerState = 'pending'; + stateError: string | undefined = undefined; android: AndroidDeviceManager; ios: IOSDeviceManager; keytarManager: KeytarManager; @@ -136,11 +137,14 @@ export class FlipperServerImpl implements FlipperServer { setServerState(state: FlipperServerState, error?: Error) { this.state = state; - this.emit('server-state', {state, error}); + this.stateError = '' + error; + this.emit('server-state', {state, error: this.stateError}); } /** - * Starts listening to parts and watching for devices + * Starts listening to parts and watching for devices. + * Connect never throws directly, but communicates + * through server-state events */ async connect() { if (this.state !== 'pending') { @@ -225,6 +229,10 @@ export class FlipperServerImpl implements FlipperServer { } private commandHandler: FlipperServerCommands = { + 'get-server-state': async () => ({ + state: this.state, + error: this.stateError, + }), 'node-api-exec': commandNodeApiExec, 'node-api-fs-access': access, 'node-api-fs-pathExists': async (path, mode) => { diff --git a/desktop/flipper-server-core/src/comms/ServerController.tsx b/desktop/flipper-server-core/src/comms/ServerController.tsx index 7745a0be3..d134dcc15 100644 --- a/desktop/flipper-server-core/src/comms/ServerController.tsx +++ b/desktop/flipper-server-core/src/comms/ServerController.tsx @@ -117,7 +117,7 @@ class ServerController extends EventEmitter implements ServerEventsListener { if (isTest()) { throw new Error('Spawing new server is not supported in test'); } - this.certificateProvider.init(); + await this.certificateProvider.init(); const {insecure, secure} = getServerPortsConfig().serverPorts; const options = await this.certificateProvider.loadSecureServerConfig(); diff --git a/desktop/flipper-server-core/src/comms/ServerRSocket.tsx b/desktop/flipper-server-core/src/comms/ServerRSocket.tsx index 98183b1ab..ce91636cb 100644 --- a/desktop/flipper-server-core/src/comms/ServerRSocket.tsx +++ b/desktop/flipper-server-core/src/comms/ServerRSocket.tsx @@ -56,23 +56,17 @@ class ServerRSocket extends ServerAdapter { onConnect(socket); }) : net.createServer(onConnect); - transportServer - .on('error', (err) => { - self.listener.onError(err); - console.error(`Error opening server on port ${port}`, 'server'); - reject(err); - }) - .on('listening', () => { - console.debug( - `${ - sslConfig ? 'Secure' : 'Certificate' - } server started on port ${port}`, - 'server', - ); - self.listener.onListening(port); - self.rawServer_ = rawServer; - resolve((transportServer.address() as AddressInfo).port); - }); + transportServer.on('error', reject).on('listening', () => { + console.debug( + `${ + sslConfig ? 'Secure' : 'Certificate' + } server started on port ${port}`, + 'server', + ); + self.listener.onListening(port); + self.rawServer_ = rawServer; + resolve((transportServer.address() as AddressInfo).port); + }); return transportServer; }; rawServer = new RSocketServer({ diff --git a/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx b/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx index f8171ef1f..da98f20d8 100644 --- a/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx +++ b/desktop/flipper-server-core/src/comms/ServerWebSocket.tsx @@ -62,8 +62,6 @@ class ServerWebSocket extends ServerAdapter { // We do not need to listen to http server's `error` because it is propagated to WS // https://github.com/websockets/ws/blob/a3a22e4ed39c1a3be8e727e9c630dd440edc61dd/lib/websocket-server.js#L109 const onConnectionError = (error: Error) => { - console.error(`[conn] Unable to start server at port ${port}`, error); - this.listener.onError(error); reject( new Error( `Unable to start server at port ${port} due to ${JSON.stringify( 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 15d80bf51..3eaa85e6c 100644 --- a/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx +++ b/desktop/flipper-server-core/src/comms/__tests__/ServerWebSocket.node.tsx @@ -92,10 +92,12 @@ describe('ServerWebSocket', () => { expect(mockSEListener.onError).toBeCalledTimes(0); const conflictingServer = new ServerWebSocket(mockSEListener); - await expect(conflictingServer.start(assignedPort)).rejects.toThrow(); + await expect(conflictingServer.start(assignedPort)).rejects.toThrow( + /EADDRINUSE/, + ); expect(mockSEListener.onListening).toBeCalledTimes(1); - expect(mockSEListener.onError).toBeCalledTimes(1); + expect(mockSEListener.onError).toBeCalledTimes(0); // no onError triggered, as start throws already }); test('calls listener onError if a connection attempt fails', async () => { diff --git a/desktop/flipper-server-core/src/devices/android/adbClient.tsx b/desktop/flipper-server-core/src/devices/android/adbClient.tsx index a8770501a..17feeb5e1 100644 --- a/desktop/flipper-server-core/src/devices/android/adbClient.tsx +++ b/desktop/flipper-server-core/src/devices/android/adbClient.tsx @@ -13,15 +13,18 @@ import adbConfig from './adbConfig'; import adbkit, {Client} from 'adbkit'; import path from 'path'; -let instance: Promise; +let instance: Client; type Config = { androidHome: string; }; -export function getAdbClient(config: Config): Promise { +export async function getAdbClient(config: Config): Promise { if (!instance) { - instance = reportPlatformFailures(createClient(config), 'createADBClient'); + instance = await reportPlatformFailures( + createClient(config), + 'createADBClient', + ); } return instance; } diff --git a/desktop/flipper-server-core/src/utils/CertificateProvider.tsx b/desktop/flipper-server-core/src/utils/CertificateProvider.tsx index 0a647beba..2bff8a6ff 100644 --- a/desktop/flipper-server-core/src/utils/CertificateProvider.tsx +++ b/desktop/flipper-server-core/src/utils/CertificateProvider.tsx @@ -115,13 +115,10 @@ export default class CertificateProvider { this._adb = await getAdbClient(this.config); } catch (_e) { // make sure initialization failure is already logged - const msg = - 'Failed to initialize ADB. Please disable Android support in settings, or configure a correct path'; - this.server.flipperServer.emit('notification', { - type: 'error', - title: 'Failed to initialise ADB', - description: msg, - }); + throw new Error( + 'Failed to initialize ADB. Please disable Android support in settings, or configure a correct path. ' + + _e, + ); } } } diff --git a/desktop/flipper-server/src/index.tsx b/desktop/flipper-server/src/index.tsx index c7e89d31c..58d7dd8c6 100644 --- a/desktop/flipper-server/src/index.tsx +++ b/desktop/flipper-server/src/index.tsx @@ -7,6 +7,7 @@ * @format */ +import process from 'process'; import chalk from 'chalk'; import path from 'path'; import {startFlipperServer} from './startFlipperServer'; @@ -37,6 +38,12 @@ const argv = yargs type: 'boolean', default: true, }, + failFast: { + describe: + 'Exit the process immediately if the server cannot start, for example due to an incorrect configuration.', + type: 'boolean', + default: false, + }, }) .version('DEV') .help() @@ -64,12 +71,19 @@ async function start() { staticDir, entry: 'index.web.dev.html', }); + const flipperServer = await startFlipperServer(rootDir, staticDir); + if (argv.failFast) { + flipperServer.on('server-state', ({state}) => { + if (state === 'error') { + process.exit(1); + } + }); + } + await flipperServer.connect(); - const [flipperServer] = await Promise.all([ - startFlipperServer(rootDir, staticDir), - argv.bundler ? startWebServerDev(app, server, socket, rootDir) : undefined, - ]); - + if (argv.bundler) { + await startWebServerDev(app, server, socket, rootDir); + } startSocketServer(flipperServer, socket); } @@ -85,6 +99,6 @@ start() } }) .catch((e) => { - console.error(chalk.red('Server error: '), e); + console.error(chalk.red('Server startup error: '), e); process.exit(1); }); diff --git a/desktop/flipper-server/src/startFlipperServer.tsx b/desktop/flipper-server/src/startFlipperServer.tsx index e616008ce..a439602e4 100644 --- a/desktop/flipper-server/src/startFlipperServer.tsx +++ b/desktop/flipper-server/src/startFlipperServer.tsx @@ -71,7 +71,7 @@ export async function startFlipperServer( const environmentInfo = await getEnvironmentInfo(appPath, isProduction); - const flipperServer = new FlipperServerImpl( + return new FlipperServerImpl( { environmentInfo, env: parseEnvironmentVariables(process.env), @@ -93,9 +93,6 @@ export async function startFlipperServer( logger, keytar, ); - - await flipperServer.connect(); - return flipperServer; } function createLogger(): Logger { diff --git a/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx b/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx index 8434aba50..c10db57e8 100644 --- a/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx +++ b/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx @@ -15,6 +15,7 @@ import { NoLongerConnectedToClientError, isTest, DeviceDescription, + FlipperServerState, } from 'flipper-common'; import Client from '../Client'; import {notification} from 'antd'; @@ -41,6 +42,8 @@ export function connectFlipperServerToStore( }); }); + server.on('server-state', handleServerStateChange); + server.on('server-error', (err) => { notification.error({ message: 'Failed to start connection server', @@ -113,6 +116,13 @@ export function connectFlipperServerToStore( 'Flipper server started and accepting device / client connections', ); + server + .exec('get-server-state') + .then(handleServerStateChange) + .catch((e) => { + console.error(`Failed to get initial server state`, e); + }); + // this flow is spawned delibarately from this main flow waitFor(store, (state) => state.plugins.initialized) .then(() => server.exec('device-list')) @@ -173,6 +183,25 @@ function startSideEffects(store: Store, server: FlipperServer) { }; } +function handleServerStateChange({ + state, + error, +}: { + state: FlipperServerState; + error?: string; +}) { + if (state === 'error') { + console.warn(`[conn] Flipper server state -> ${state}`, error); + notification.error({ + message: 'Failed to start flipper-server', + description: '' + error, + duration: null, + }); + } else { + console.info(`[conn] Flipper server state -> ${state}`); + } +} + function handleDeviceConnected( server: FlipperServer, store: Store,