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

@@ -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,

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<void>;
   'node-api-fs-pathExists': (path: string, mode?: number) => Promise<boolean>;
   'node-api-fs-unlink': (path: string) => Promise<void>;

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(

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<string, ServerDevice>();
   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) => {

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();

desktop/flipper-server-core/src/comms/ServerRSocket.tsx

@@ -56,13 +56,7 @@ 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', () => {
+        transportServer.on('error', reject).on('listening', () => {
           console.debug(
             `${
               sslConfig ? 'Secure' : 'Certificate'

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(

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 () => {

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<Client>;
+let instance: Client;
 
 type Config = {
   androidHome: string;
 };
 
-export function getAdbClient(config: Config): Promise<Client> {
+export async function getAdbClient(config: Config): Promise<Client> {
   if (!instance) {
-    instance = reportPlatformFailures(createClient(config), 'createADBClient');
+    instance = await reportPlatformFailures(
+      createClient(config),
+      'createADBClient',
+    );
   }
   return instance;
 }

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,
+        );
       }
     }
   }

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

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 {

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,