fix(utils): handle error events from child processes

src/utils/Child.ts

@@ -22,6 +22,11 @@ export interface ChildResult {
 
 export type ChildSpawner = typeof spawn;
 
+const CHILD_ENCODING = 'utf-8';
+const CHILD_EVENT = 'child process emitted error event';
+const CHILD_STATUS = 'child process exited with error status';
+const CHILD_OUTPUT = 'child process emitted error output';
+
 export function waitForChild(child: ChildProcessWithoutNullStreams): Promise<ChildResult> {
   return new Promise((res, rej) => {
     const stderr: Array<Buffer> = [];
@@ -35,28 +40,29 @@ export function waitForChild(child: ChildProcessWithoutNullStreams): Promise<Chi
       stdout.push(chunk);
     });
 
+    child.on('error', (err: Error | number) => {
+      if (err instanceof Error) {
+        rej(new ChildProcessError(CHILD_EVENT, err));
+      }
+    });
+
     child.on('close', (status: number) => {
-      const errors = encode(stderr, 'utf-8');
+      const errors = encode(stderr, CHILD_ENCODING);
       if (status > 0) {
-        rej(new ChildProcessError(
-          `child process exited with error status: ${status}`,
-          new BaseError(errors)
-        ));
+        const msg = `${CHILD_STATUS}: ${status}`;
+        rej(new ChildProcessError(msg, new BaseError(errors)));
         return;
       }
 
       if (errors.length > 0) {
-        rej(new ChildProcessError(
-          'child process exited with error output',
-          new BaseError(errors)
-        ));
+        rej(new ChildProcessError(CHILD_OUTPUT, new BaseError(errors)));
         return;
       }
 
       res({
         status,
         stderr: errors,
-        stdout: encode(stdout, 'utf-8'),
+        stdout: encode(stdout, CHILD_ENCODING),
       });
     });
   });

test/filter/TestShellFilter.ts

@@ -1,18 +1,20 @@
 import { expect } from 'chai';
 import { ChildProcessByStdio, ChildProcessWithoutNullStreams } from 'child_process';
 import { ineeda } from 'ineeda';
-import { stub } from 'sinon';
+import { BaseError } from 'noicejs';
+import { match, stub } from 'sinon';
 import { Readable, Writable } from 'stream';
 
 import { INJECT_CLOCK, INJECT_LOGGER } from '../../src/BaseService';
 import { Message } from '../../src/entity/Message';
+import { ChildProcessError } from '../../src/error/ChildProcessError';
 import { FilterBehavior } from '../../src/filter';
 import { ShellFilter, ShellFilterData } from '../../src/filter/ShellFilter';
+import { waitForChild } from '../../src/utils/Child';
 import { Clock } from '../../src/utils/Clock';
 import { describeLeaks, itLeaks } from '../helpers/async';
 import { createService, createServiceContainer } from '../helpers/container';
 import { getTestLogger } from '../helpers/logger';
-import { waitForChild } from '../../src/utils/Child';
 
 const TEST_CONFIG: ShellFilterData = {
   child: {
@@ -38,7 +40,7 @@ function createChild(status: number) {
     on: stub(),
   });
   const child = ineeda<ChildProcessWithoutNullStreams>({
-    on: stub().withArgs('close').yields(status),
+    on: stub().withArgs('close', match.func).yields(status),
     stderr: stdout,
     stdin,
     stdout,
@@ -114,13 +116,13 @@ describeLeaks('shell filter', async () => {
     });
 
     const stderr = ineeda<Readable>({
-      on: stub().withArgs('data').yields(Buffer.from('this is an error')),
+      on: stub().withArgs('data', match.func).yields(Buffer.from('this is an error')),
     });
     const stdout = ineeda<Readable>({
       on: stub(),
     });
     const child = ineeda<ChildProcessWithoutNullStreams>({
-      on: stub().withArgs('close').yields(0),
+      on: stub().withArgs('close', match.func).yields(0),
       stderr,
       stdin,
       stdout,
@@ -149,7 +151,7 @@ describeLeaks('shell filter', async () => {
     const { stdout } = createChild(0);
 
     const child = ineeda<ChildProcessByStdio<null, Readable, Readable>>({
-      on: stub().withArgs('close').yields(0),
+      on: stub().withArgs('close', match.func).yields(0),
       stderr: stdout,
       /* getter/setter pair shouldn't be required for proper sinon mock */
       get stdin() {
@@ -176,7 +178,6 @@ describeLeaks('shell filter', async () => {
     });
 
     const result = await filter.check(ineeda.instanceof(Message));
-    /* should have unsuccessfully called exec */
     expect(exec).to.have.callCount(1);
     expect(result).to.equal(FilterBehavior.Allow);
   });
@@ -186,16 +187,46 @@ describeLeaks('shell filter', async () => {
 
     const TEST_OUTPUT = 'hello world';
     const child = ineeda<ChildProcessWithoutNullStreams>({
-      on: stub().withArgs('close').yields(0),
+      on: stub().withArgs('close', match.func).yields(0),
       stderr,
       stdin,
       stdout: ineeda<Readable>({
-        on: stub().withArgs('data').yields(Buffer.from(TEST_OUTPUT)),
+        on: stub().withArgs('data', match.func).yields(Buffer.from(TEST_OUTPUT)),
       }),
     });
 
-    /* service in test */
     const result = await waitForChild(child);
     expect(result.stdout).to.equal(TEST_OUTPUT);
   });
+
+  itLeaks('should handle error events from child', async () => {
+    const { stderr, stdin, stdout } = createChild(0);
+
+    const child = ineeda<ChildProcessWithoutNullStreams>({
+      on: stub()
+        .withArgs('close', match.func).yields(0)
+        .withArgs('error', match.func).yields(new BaseError('child process broke')),
+      stderr,
+      stdin,
+      stdout,
+    });
+
+    return expect(waitForChild(child)).to.eventually.be.rejectedWith(ChildProcessError);
+  });
+
+  itLeaks('should ignore exit status passed as error event', async () => {
+    const { stderr, stdin, stdout } = createChild(0);
+
+    const child = ineeda<ChildProcessWithoutNullStreams>({
+      on: stub()
+        .withArgs('close', match.func).yields(0)
+        .withArgs('error', match.func).yields(0),
+      stderr,
+      stdin,
+      stdout,
+    });
+
+    const result = await waitForChild(child);
+    expect(result.status).to.equal(0);
+  });
 });