More robust process handling for apps (#418)

* More robust process handling for apps * Always tell the client the server is stopped on error * Fix lint * Log process error if no onError callback is given
srcbookdev · Oct 23, 2024 · d94d5cf · d94d5cf
1 parent 42e467b
commit d94d5cf
Show file tree

Hide file tree

Showing 4 changed files with 167 additions and 36 deletions.
diff --git a/packages/api/apps/processes.mts b/packages/api/apps/processes.mts
@@ -0,0 +1,99 @@
+import { ChildProcess } from 'node:child_process';
+import { pathToApp } from './disk.mjs';
+import { npmInstall as execNpmInstall, vite as execVite } from '../exec.mjs';
+
+export type ProcessType = 'npm:install' | 'vite:server';
+
+export interface NpmInstallProcessType {
+  type: 'npm:install';
+  process: ChildProcess;
+}
+
+export interface ViteServerProcessType {
+  type: 'vite:server';
+  process: ChildProcess;
+  port: number | null;
+}
+
+export type AppProcessType = NpmInstallProcessType | ViteServerProcessType;
+
+class Processes {
+  private map: Map<string, AppProcessType> = new Map();
+
+  has(appId: string, type: ProcessType) {
+    return this.map.has(this.toKey(appId, type));
+  }
+
+  get(appId: string, type: ProcessType) {
+    return this.map.get(this.toKey(appId, type));
+  }
+
+  set(appId: string, process: AppProcessType) {
+    this.map.set(this.toKey(appId, process.type), process);
+  }
+
+  del(appId: string, type: ProcessType) {
+    return this.map.delete(this.toKey(appId, type));
+  }
+
+  private toKey(appId: string, type: ProcessType) {
+    return `${appId}:${type}`;
+  }
+}
+
+const processes = new Processes();
+
+export function getAppProcess(appId: string, type: 'npm:install'): NpmInstallProcessType;
+export function getAppProcess(appId: string, type: 'vite:server'): ViteServerProcessType;
+export function getAppProcess(appId: string, type: ProcessType): AppProcessType {
+  switch (type) {
+    case 'npm:install':
+      return processes.get(appId, type) as NpmInstallProcessType;
+    case 'vite:server':
+      return processes.get(appId, type) as ViteServerProcessType;
+  }
+}
+
+export function setAppProcess(appId: string, process: AppProcessType) {
+  processes.set(appId, process);
+}
+
+export function deleteAppProcess(appId: string, process: ProcessType) {
+  processes.del(appId, process);
+}
+
+/**
+ * Runs npm install for the given app.
+ *
+ * If there's already a process running npm install, it will return that process.
+ */
+export function npmInstall(
+  appId: string,
+  options: Omit<Parameters<typeof execNpmInstall>[0], 'cwd'>,
+) {
+  if (!processes.has(appId, 'npm:install')) {
+    processes.set(appId, {
+      type: 'npm:install',
+      process: execNpmInstall({ cwd: pathToApp(appId), ...options }),
+    });
+  }
+
+  return processes.get(appId, 'npm:install');
+}
+
+/**
+ * Runs a vite dev server for the given app.
+ *
+ * If there's already a process running the vite dev server, it will return that process.
+ */
+export function viteServer(appId: string, options: Omit<Parameters<typeof execVite>[0], 'cwd'>) {
+  if (!processes.has(appId, 'vite:server')) {
+    processes.set(appId, {
+      type: 'vite:server',
+      process: execVite({ cwd: pathToApp(appId), ...options }),
+      port: null,
+    });
+  }
+
+  return processes.get(appId, 'vite:server');
+}
diff --git a/packages/api/exec.mts b/packages/api/exec.mts
@@ -1,11 +1,16 @@
 import Path from 'node:path';
 import { spawn } from 'node:child_process';
 
+interface NodeError extends Error {
+  code?: string;
+}
+
 export type BaseExecRequestType = {
   cwd: string;
   stdout: (data: Buffer) => void;
   stderr: (data: Buffer) => void;
   onExit: (code: number | null, signal: NodeJS.Signals | null) => void;
+  onError?: (err: NodeError) => void;
 };
 
 export type NodeRequestType = BaseExecRequestType & {
@@ -30,25 +35,28 @@ type SpawnCallRequestType = {
   stdout: (data: Buffer) => void;
   stderr: (data: Buffer) => void;
   onExit: (code: number | null, signal: NodeJS.Signals | null) => void;
+  onError?: (err: NodeError) => void;
 };
 
 export function spawnCall(options: SpawnCallRequestType) {
-  const { cwd, env, command, args, stdout, stderr, onExit } = options;
+  const { cwd, env, command, args, stdout, stderr, onExit, onError } = options;
   const child = spawn(command, args, { cwd: cwd, env: env });
 
   child.stdout.on('data', stdout);
   child.stderr.on('data', stderr);
 
-  child.on('error', () => {
-    // Sometimes it's expected we abort the child process (e.g., user stops a running cell).
-    // Doing so crashes the parent process unless this callback callback is registered.
-    //
-    // TODO: Find a way to handle unexpected errors here.
+  child.on('error', (err) => {
+    if (onError) {
+      onError(err);
+    } else {
+      console.error(err);
+    }
   });
 
   child.on('exit', (code, signal) => {
-    onExit && onExit(code, signal);
+    onExit(code, signal);
   });
+
   return child;
 }
 

diff --git a/packages/api/server/channels/app.mts b/packages/api/server/channels/app.mts
@@ -1,7 +1,5 @@
 import path from 'node:path';
 import fs from 'node:fs/promises';
-import { ChildProcess } from 'node:child_process';
-
 import {
   PreviewStartPayloadSchema,
   PreviewStopPayloadSchema,
@@ -22,20 +20,19 @@ import WebSocketServer, {
 } from '../ws-client.mjs';
 import { loadApp } from '../../apps/app.mjs';
 import { fileUpdated, pathToApp } from '../../apps/disk.mjs';
-import { vite, npmInstall } from '../../exec.mjs';
 import { directoryExists } from '../../fs-utils.mjs';
+import {
+  getAppProcess,
+  setAppProcess,
+  deleteAppProcess,
+  npmInstall,
+  viteServer,
+} from '../../apps/processes.mjs';
 
 const VITE_PORT_REGEX = /Local:.*http:\/\/localhost:([0-9]{1,4})/;
 
 type AppContextType = MessageContextType<'appId'>;
 
-type ProcessMetadata = {
-  process: ChildProcess;
-  port: number | null;
-};
-
-const processMetadata = new Map<string, ProcessMetadata>();
-
 async function previewStart(
   _payload: PreviewStartPayloadType,
   context: AppContextType,
@@ -47,7 +44,7 @@ async function previewStart(
     return;
   }
 
-  const existingProcess = processMetadata.get(app.externalId);
+  const existingProcess = getAppProcess(app.externalId, 'vite:server');
 
   if (existingProcess) {
     wss.broadcast(`app:${app.externalId}`, 'preview:status', {
@@ -63,16 +60,28 @@ async function previewStart(
   });
 
   const onChangePort = (newPort: number) => {
-    processMetadata.set(app.externalId, { process, port: newPort });
+    const process = getAppProcess(app.externalId, 'vite:server');
+
+    // This is not expected to happen
+    if (!process) {
+      wss.broadcast(`app:${app.externalId}`, 'preview:status', {
+        url: null,
+        status: 'stopped',
+        code: null,
+      });
+      return;
+    }
+
+    setAppProcess(app.externalId, { ...process, port: newPort });
+
     wss.broadcast(`app:${app.externalId}`, 'preview:status', {
       url: `http://localhost:${newPort}/`,
       status: 'running',
     });
   };
 
-  const process = vite({
+  viteServer(app.externalId, {
     args: [],
-    cwd: pathToApp(app.externalId),
     stdout: (data) => {
       const encodedData = data.toString('utf8');
       console.log(encodedData);
@@ -103,17 +112,28 @@ async function previewStart(
       });
     },
     onExit: (code) => {
-      processMetadata.delete(app.externalId);
+      deleteAppProcess(app.externalId, 'vite:server');
 
       wss.broadcast(`app:${app.externalId}`, 'preview:status', {
         url: null,
         status: 'stopped',
         code: code,
       });
     },
-  });
+    onError: (_error) => {
+      // Errors happen when we try to run vite before node modules are installed.
+      // Make sure we clean up the app process and inform the client.
+      deleteAppProcess(app.externalId, 'vite:server');
 
-  processMetadata.set(app.externalId, { process, port: null });
+      // TODO: Use a different event to communicate to the client there was an error.
+      // If the error is ENOENT, for example, it means node_modules and/or vite is missing.
+      wss.broadcast(`app:${app.externalId}`, 'preview:status', {
+        url: null,
+        status: 'stopped',
+        code: null,
+      });
+    },
+  });
 }
 
 async function previewStop(
@@ -127,7 +147,7 @@ async function previewStop(
     return;
   }
 
-  const result = processMetadata.get(app.externalId);
+  const result = getAppProcess(app.externalId, 'vite:server');
 
   if (!result) {
     conn.reply(`app:${app.externalId}`, 'preview:status', {
@@ -147,36 +167,38 @@ async function previewStop(
 async function dependenciesInstall(
   payload: DepsInstallPayloadType,
   context: AppContextType,
-  conn: ConnectionContextType,
+  wss: WebSocketServer,
 ) {
   const app = await loadApp(context.params.appId);
 
   if (!app) {
     return;
   }
 
-  npmInstall({
+  npmInstall(app.externalId, {
     args: [],
-    cwd: pathToApp(app.externalId),
     packages: payload.packages ?? undefined,
     stdout: (data) => {
-      conn.reply(`app:${app.externalId}`, 'deps:install:log', {
+      wss.broadcast(`app:${app.externalId}`, 'deps:install:log', {
         log: { type: 'stdout', data: data.toString('utf8') },
       });
     },
     stderr: (data) => {
-      conn.reply(`app:${app.externalId}`, 'deps:install:log', {
+      wss.broadcast(`app:${app.externalId}`, 'deps:install:log', {
         log: { type: 'stderr', data: data.toString('utf8') },
       });
     },
     onExit: (code) => {
-      conn.reply(`app:${app.externalId}`, 'deps:install:status', {
+      // We must clean up this process so that we can run npm install again
+      deleteAppProcess(app.externalId, 'npm:install');
+
+      wss.broadcast(`app:${app.externalId}`, 'deps:install:status', {
         status: code === 0 ? 'complete' : 'failed',
         code,
       });
 
       if (code === 0) {
-        conn.reply(`app:${app.externalId}`, 'deps:status:response', {
+        wss.broadcast(`app:${app.externalId}`, 'deps:status:response', {
           nodeModulesExists: true,
         });
       }
@@ -239,7 +261,9 @@ export function register(wss: WebSocketServer) {
       previewStart(payload, context, wss),
     )
     .on('preview:stop', PreviewStopPayloadSchema, previewStop)
-    .on('deps:install', DepsInstallPayloadSchema, dependenciesInstall)
+    .on('deps:install', DepsInstallPayloadSchema, (payload, context) => {
+      dependenciesInstall(payload, context, wss);
+    })
     .on('deps:clear', DepsInstallPayloadSchema, clearNodeModules)
     .on('deps:status', DepsStatusPayloadSchema, dependenciesStatus)
     .on('file:updated', FileUpdatedPayloadSchema, onFileUpdated);

diff --git a/packages/web/src/components/apps/use-package-json.tsx b/packages/web/src/components/apps/use-package-json.tsx
@@ -56,8 +56,8 @@ export function PackageJsonProvider({ channel, children }: ProviderPropsType) {
     async (packages?: Array<string>) => {
       addLog(
         'info',
-        'npm',
-        `Running ${!packages ? 'npm install' : `npm install ${packages.join(' ')}`}`,
+        'srcbook',
+        `Running ${!packages ? 'npm install' : `npm install ${packages.join(' ')}`}...`,
       );
 
       // NOTE: caching of the log output is required here because socket events that call callback
@@ -78,7 +78,7 @@ export function PackageJsonProvider({ channel, children }: ProviderPropsType) {
 
           addLog(
             'info',
-            'npm',
+            'srcbook',
             `${!packages ? 'npm install' : `npm install ${packages.join(' ')}`} exited with status code ${code}`,
           );