From d94d5cf6c634f0a2b72c367e885fc532ec9e0b71 Mon Sep 17 00:00:00 2001 From: Ben Reinhart Date: Wed, 23 Oct 2024 11:50:54 -0700 Subject: [PATCH] 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 --- packages/api/apps/processes.mts | 99 +++++++++++++++++++ packages/api/exec.mts | 22 +++-- packages/api/server/channels/app.mts | 76 +++++++++----- .../src/components/apps/use-package-json.tsx | 6 +- 4 files changed, 167 insertions(+), 36 deletions(-) create mode 100644 packages/api/apps/processes.mts diff --git a/packages/api/apps/processes.mts b/packages/api/apps/processes.mts new file mode 100644 index 00000000..3b17a94a --- /dev/null +++ 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 = 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[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[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 index 28878db9..ba87e58b 100644 --- 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 index b2cd6465..168c2fe2 100644 --- 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(); - 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,7 +112,7 @@ async function previewStart( }); }, onExit: (code) => { - processMetadata.delete(app.externalId); + deleteAppProcess(app.externalId, 'vite:server'); wss.broadcast(`app:${app.externalId}`, 'preview:status', { url: null, @@ -111,9 +120,20 @@ async function previewStart( 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,7 +167,7 @@ async function previewStop( async function dependenciesInstall( payload: DepsInstallPayloadType, context: AppContextType, - conn: ConnectionContextType, + wss: WebSocketServer, ) { const app = await loadApp(context.params.appId); @@ -155,28 +175,30 @@ async function dependenciesInstall( 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 index a3457e60..8d3f9aaa 100644 --- 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) => { 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}`, );