fix(node-runtime-worker-thread): Better clean-up for the child process spawner method

Author: gribnoysup

packages/node-runtime-worker-thread/src/child-process-proxy.ts

@@ -1,3 +1,6 @@
+/* istanbul ignore file */
+/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */
+
 /**
  * This proxy is needed as a workaround for the old electron verison "bug" where
  * due to the electron runtime being a chromium, not just node (even with

packages/node-runtime-worker-thread/src/index.spec.ts

@@ -1,12 +1,10 @@
 import chai, { expect } from 'chai';
 import sinon from 'sinon';
 import sinonChai from 'sinon-chai';
-import chaiAsPromised from 'chai-as-promised';
 import { startTestServer } from '../../../testing/integration-testing-hooks';
 import { WorkerRuntime } from '../dist/index';
 
 chai.use(sinonChai);
-chai.use(chaiAsPromised);
 
 describe('WorkerRuntime', () => {
   describe('evaluate', () => {

packages/node-runtime-worker-thread/src/index.ts

@@ -1,9 +1,11 @@
-import { once } from 'events';
+/* istanbul ignore file */
+/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */
+
 import { ChildProcess } from 'child_process';
 import { MongoClientOptions } from '@mongosh/service-provider-core';
 import { Runtime } from '@mongosh/browser-runtime-core';
 import { EvaluationListener } from '@mongosh/shell-evaluator';
-import spawnChildFromSource from './spawn-child-from-source';
+import spawnChildFromSource, { kill } from './spawn-child-from-source';
 import { Caller, createCaller, exposeAll, WithClose } from './rpc';
 import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime';
 import childProcessProxySrc from 'inline-entry-loader!./child-process-proxy';
@@ -104,13 +106,7 @@ class WorkerRuntime implements Runtime {
 
   async terminate() {
     await this.initWorkerPromise;
-    this.childProcess.kill('SIGTERM');
-    if (
-      this.childProcess.exitCode === null &&
-      this.childProcess.signalCode === null
-    ) {
-      await once(this.childProcess, 'exit');
-    }
+    await kill(this.childProcess);
   }
 }
 

packages/node-runtime-worker-thread/src/spawn-child-from-source.spec.ts

@@ -1,47 +1,69 @@
-import chai, { expect } from 'chai';
-import chaiAsPromised from 'chai-as-promised';
-import childProcess from 'child_process';
+import { expect } from 'chai';
+import childProcess, { ChildProcess } from 'child_process';
 import { once } from 'events';
 
-import spawnChildFromSource from './spawn-child-from-source';
-
-chai.use(chaiAsPromised);
+import spawnChildFromSource, { kill } from './spawn-child-from-source';
 
 describe('spawnChildFromSource', () => {
+  let spawned: ChildProcess;
+
+  afterEach(async() => {
+    if (spawned) {
+      await kill(spawned, 'SIGKILL');
+      spawned = null;
+    }
+  });
+
   it('should resolve with a child process', async() => {
-    const spawned = await spawnChildFromSource('');
+    spawned = await spawnChildFromSource('');
     expect(spawned).to.be.instanceof((childProcess as any).ChildProcess);
-    spawned.kill('SIGTERM');
   });
 
   it('should spawn a process with an ipc channel open', async() => {
-    const spawned = await spawnChildFromSource(
+    spawned = await spawnChildFromSource(
       'process.on("message", (data) => process.send(data))'
     );
     spawned.send('Hi!');
     const [message] = await once(spawned, 'message');
     expect(message).to.equal('Hi!');
-    spawned.kill('SIGTERM');
   });
 
-  it('should fail if process exited before successfully starting', () => {
-    return expect(
-      spawnChildFromSource(
+  it('should fail if process exited before successfully starting', async() => {
+    let err: Error;
+
+    try {
+      spawned = await spawnChildFromSource(
         'throw new Error("Whoops!")',
         {},
         undefined,
         'ignore',
         'ignore'
-      )
-    ).to.eventually.be.rejectedWith(
+      );
+    } catch (e) {
+      err = e;
+    }
+
+    expect(err).to.be.instanceof(Error);
+    expect(err.message).to.match(
       /Child process exited with error before starting/
     );
   });
 
-  it('should fail if a timeout exceeded before the process is "ready"', () => {
-    return expect(
-      spawnChildFromSource('while(true){}', {}, 200)
-    ).to.eventually.be.rejectedWith(
+  it('should fail if a timeout exceeded before the process is "ready"', async() => {
+    let err: Error;
+
+    try {
+      spawned = await spawnChildFromSource(
+        'let i = 0; while(++i < 10000000000){};',
+        {},
+        10
+      );
+    } catch (e) {
+      err = e;
+    }
+
+    expect(err).to.be.instanceof(Error);
+    expect(err.message).to.match(
       /Timed out while waiting for child process to start/
     );
   });

packages/node-runtime-worker-thread/src/spawn-child-from-source.ts

@@ -6,15 +6,26 @@ import {
   StdioNull,
   StdioPipe
 } from 'child_process';
+import { once } from 'events';
+
+export async function kill(
+  childProcess: ChildProcess,
+  code: NodeJS.Signals | number = 'SIGTERM'
+) {
+  childProcess.kill(code);
+  if (childProcess.exitCode === null && childProcess.signalCode === null) {
+    await once(childProcess, 'exit');
+  }
+}
 
 export default function spawnChildFromSource(
   src: string,
   spawnOptions: SpawnOptionsWithoutStdio = {},
   timeoutMs?: number,
   _stdout: StdioNull | StdioPipe = 'inherit',
-  _stderr: StdioNull | StdioPipe = 'inherit',
+  _stderr: StdioNull | StdioPipe = 'inherit'
 ): Promise<ChildProcess> {
-  return new Promise((resolve, reject) => {
+  return new Promise(async(resolve, reject) => {
     const readyToken = Date.now().toString(32);
 
     const childProcess = spawn(process.execPath, {
@@ -23,6 +34,8 @@ export default function spawnChildFromSource(
     });
 
     if (!childProcess.stdin) {
+      await kill(childProcess);
+
       return reject(
         new Error("Can't write src to the spawned process, missing stdin")
       );
@@ -31,38 +44,56 @@ export default function spawnChildFromSource(
     // eslint-disable-next-line prefer-const
     let timeoutId: NodeJS.Timeout | null;
 
-    const onExit = (exitCode: number | null) => {
+    function cleanupListeners() {
+      if (timeoutId) {
+        clearTimeout(timeoutId);
+      }
+      if (childProcess.stdin) {
+        childProcess.stdin.off('error', onWriteError);
+      }
+      childProcess.off('message', onMessage);
+      childProcess.off('exit', onExit);
+    }
+
+    async function onExit(exitCode: number | null) {
       if (exitCode && exitCode > 0) {
+        cleanupListeners();
         reject(new Error('Child process exited with error before starting'));
       }
-    };
+    }
 
-    const onMessage = (data: Serializable) => {
+    async function onWriteError(error: Error) {
+      cleanupListeners();
+      await kill(childProcess);
+      reject(error);
+    }
+
+    async function onTimeout() {
+      cleanupListeners();
+      await kill(childProcess);
+      reject(
+        new Error('Timed out while waiting for child process to start')
+      );
+    }
+
+    function onMessage(data: Serializable) {
       if (data === readyToken) {
-        if (timeoutId) {
-          clearTimeout(timeoutId);
-        }
-        childProcess.off('exit', onExit);
+        cleanupListeners();
         resolve(childProcess);
       }
-    };
+    }
 
     childProcess.on('message', onMessage);
-
     childProcess.on('exit', onExit);
+    childProcess.stdin.on('error', onWriteError);
 
     childProcess.stdin.write(src);
     childProcess.stdin.write(`;process.send(${JSON.stringify(readyToken)})`);
     childProcess.stdin.end();
 
     timeoutId =
       timeoutMs !== undefined
-        ? setTimeout(() => {
-          reject(
-            new Error('Timed out while waiting for child process to start')
-          );
-          childProcess.kill('SIGTERM');
-        }, timeoutMs)
+        ? setTimeout(onTimeout, timeoutMs)
         : null;
   });
 }

packages/node-runtime-worker-thread/src/worker-runtime.ts

@@ -1,3 +1,6 @@
+/* istanbul ignore file */
+/* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */
+
 import { parentPort, isMainThread } from 'worker_threads';
 import { Runtime } from '@mongosh/browser-runtime-core';
 import { ElectronRuntime } from '@mongosh/browser-runtime-electron';