fix(sandbox): Prevent hanging child processes (#402)

* Rename `SandboxCoordinator` -> `SandboxPool`
* Make sure newly created sandboxes are also disposed if the SandboxPool is already disposed

fixes #396

packages/stryker/src/Sandbox.ts

@@ -29,7 +29,7 @@ export default class Sandbox {
   private workingFolder: string;
   private testHooksFile = path.resolve('___testHooksForStryker.js');
 
-  constructor(private options: Config, private index: number, files: ReadonlyArray<File>, private testFramework: TestFramework | null, private coverageInstrumenter: CoverageInstrumenter | null) {
+  private constructor(private options: Config, private index: number, files: ReadonlyArray<File>, private testFramework: TestFramework | null, private coverageInstrumenter: CoverageInstrumenter | null) {
     this.workingFolder = TempFolder.instance().createRandomFolder('sandbox');
     log.debug('Creating a sandbox for files in %s', this.workingFolder);
     this.files = files.slice(); // Create a copy
@@ -46,11 +46,17 @@ export default class Sandbox {
     }
   }
 
-  public async initialize(): Promise<void> {
+  private async initialize(): Promise<void> {
     await this.fillSandbox();
     return this.initializeTestRunner();
   }
 
+  public static create(options: Config, index: number, files: ReadonlyArray<File>, testFramework: TestFramework | null, coverageInstrumenter: CoverageInstrumenter | null)
+    : Promise<Sandbox> {
+    const sandbox = new Sandbox(options, index, files, testFramework, coverageInstrumenter);
+    return sandbox.initialize().then(() => sandbox);
+  }
+
   public run(timeout: number): Promise<RunResult> {
     return this.testRunner.run({ timeout });
   }

packages/stryker/src/SandboxCoordinator.ts → packages/stryker/src/SandboxPool.ts

@@ -6,10 +6,11 @@ import { File } from 'stryker-api/core';
 import { TestFramework } from 'stryker-api/test_framework';
 import Sandbox from './Sandbox';
 
-export default class SandboxCoordinator {
+export default class SandboxPool {
 
-  private readonly log = getLogger(SandboxCoordinator.name);
+  private readonly log = getLogger(SandboxPool.name);
   private readonly sandboxes: Sandbox[] = [];
+  private isDisposed: boolean = false;
 
   constructor(private options: Config, private testFramework: TestFramework | null, private initialFiles: File[]) {
   }
@@ -29,19 +30,27 @@ export default class SandboxCoordinator {
       numConcurrentRunners = 1;
     }
     this.log.info(`Creating ${numConcurrentRunners} test runners (based on ${numConcurrentRunnersSource})`);
+
     const sandboxes = Observable.range(0, numConcurrentRunners)
-      .map(n => new Sandbox(this.options, n, this.initialFiles, this.testFramework, null))
-      .flatMap(sandbox => sandbox.initialize()
-        .then(() => sandbox))
-      .do(sandbox => this.registerSandbox(sandbox));
+      .flatMap(n => this.registerSandbox(Sandbox.create(this.options, n, this.initialFiles, this.testFramework, null)));
     return sandboxes;
   }
 
-  private registerSandbox(sandbox: Sandbox) {
-    this.sandboxes.push(sandbox);
+  private registerSandbox(promisedSandbox: Promise<Sandbox>): Promise<Sandbox> {
+    return promisedSandbox.then(sandbox => {
+      if (this.isDisposed) {
+        // This sandbox is too late for the party. Dispose it to prevent hanging child processes
+        // See issue #396
+        sandbox.dispose();
+      } else {
+        this.sandboxes.push(sandbox);
+      }
+      return sandbox;
+    });
   }
 
   public disposeAll() {
+    this.isDisposed = true;
     return Promise.all(this.sandboxes.map(sandbox => sandbox.dispose()));
   }
 }

packages/stryker/src/process/InitialTestExecutor.ts

@@ -57,8 +57,7 @@ export default class InitialTestExecutor {
       throw new Error(`Could not transpile input files: ${transpileResult.error}`);
     } else {
       this.logTranspileResult(transpileResult);
-      const sandbox = new Sandbox(this.options, 0, transpileResult.outputFiles, this.testFramework, this.coverageInstrumenter);
-      await sandbox.initialize();
+      const sandbox = await Sandbox.create(this.options, 0, transpileResult.outputFiles, this.testFramework, this.coverageInstrumenter);
       const runResult = await sandbox.run(INITIAL_RUN_TIMEOUT);
       await sandbox.dispose();
       return { runResult, transpiledFiles: transpileResult.outputFiles };

packages/stryker/src/process/MutationTestExecutor.ts

@@ -9,7 +9,7 @@ import TranspiledMutant from '../TranspiledMutant';
 import StrictReporter from '../reporters/StrictReporter';
 import TestableMutant from '../TestableMutant';
 import MutantTranspiler from '../transpiler/MutantTranspiler';
-import SandboxCoordinator from '../SandboxCoordinator';
+import SandboxPool from '../SandboxPool';
 
 export default class MutationTestExecutor {
 
@@ -18,7 +18,7 @@ export default class MutationTestExecutor {
   }
 
   async run(allMutants: TestableMutant[]): Promise<MutantResult[]> {
-    const sandboxPool = new SandboxCoordinator(this.config, this.testFramework, this.transpiledFiles);
+    const sandboxPool = new SandboxPool(this.config, this.testFramework, this.transpiledFiles);
     const mutantTranspiler = new MutantTranspiler(this.config);
     await mutantTranspiler.initialize(this.inputFiles);
     const result = await this.runInsideSandboxes(

packages/stryker/test/unit/SandboxCoordinatorSpec.ts → packages/stryker/test/unit/SandboxPoolSpec.ts

@@ -2,15 +2,14 @@ import { File } from 'stryker-api/core';
 import * as os from 'os';
 import { expect } from 'chai';
 import { Config } from 'stryker-api/config';
-import SandboxCoordinator from '../../src/SandboxCoordinator';
+import SandboxPool from '../../src/SandboxPool';
 import { TestFramework } from 'stryker-api/test_framework';
 import { Mock, mock, testFramework, textFile, config } from '../helpers/producers';
-import * as strykerSandbox from '../../src/Sandbox';
 import Sandbox from '../../src/Sandbox';
 import '../helpers/globals';
 
-describe('SandboxCoordinator', () => {
-  let sut: SandboxCoordinator;
+describe('SandboxPool', () => {
+  let sut: SandboxPool;
   let firstSandbox: Mock<Sandbox>;
   let secondSandbox: Mock<Sandbox>;
   let options: Config;
@@ -23,60 +22,62 @@ describe('SandboxCoordinator', () => {
     expectedTestFramework = testFramework();
     coverageInstrumenter = 'a coverage instrumenter';
     firstSandbox = mock(Sandbox);
-    firstSandbox.initialize.resolves();
     firstSandbox.dispose.resolves();
     secondSandbox = mock(Sandbox);
-    secondSandbox.initialize.resolves();
     secondSandbox.dispose.resolves();
-    const genericSandboxForAllSubsequentCallsToNewSandbox = mock(Sandbox);
-    genericSandboxForAllSubsequentCallsToNewSandbox.initialize.resolves();
+    const genericSandboxForAllSubsequentCallsToNewSandbox = mock<Sandbox>(Sandbox);
     genericSandboxForAllSubsequentCallsToNewSandbox.dispose.resolves();
-    global.sandbox.stub(strykerSandbox, 'default')
-      .returns(genericSandboxForAllSubsequentCallsToNewSandbox)
-      .onCall(0).returns(firstSandbox)
-      .onCall(1).returns(secondSandbox);
+    global.sandbox.stub(Sandbox, 'create')
+      .resolves(genericSandboxForAllSubsequentCallsToNewSandbox)
+      .onCall(0).resolves(firstSandbox)
+      .onCall(1).resolves(secondSandbox);
 
     expectedInputFiles = [textFile()];
-    sut = new SandboxCoordinator(options, expectedTestFramework, expectedInputFiles);
+    sut = new SandboxPool(options, expectedTestFramework, expectedInputFiles);
   });
 
   describe('streamSandboxes', () => {
     it('should use maxConcurrentTestRunners when set', async () => {
       options.maxConcurrentTestRunners = 1;
       await sut.streamSandboxes().toArray().toPromise();
-      expect(strykerSandbox.default).calledWithNew;
-      expect(strykerSandbox.default).to.have.callCount(1);
-      expect(strykerSandbox.default).calledWith(options, 0, expectedInputFiles, expectedTestFramework, null);
+      expect(Sandbox.create).to.have.callCount(1);
+      expect(Sandbox.create).calledWith(options, 0, expectedInputFiles, expectedTestFramework, null);
     });
 
     it('should use cpuCount when maxConcurrentTestRunners is set too high', async () => {
       global.sandbox.stub(os, 'cpus').returns([1, 2, 3]); // stub 3 cpus
       options.maxConcurrentTestRunners = 100;
       const actual = await sut.streamSandboxes().toArray().toPromise();
-      expect(strykerSandbox.default).calledWithNew;
       expect(actual).lengthOf(3);
-      expect(strykerSandbox.default).to.have.callCount(3);
-      expect(strykerSandbox.default).calledWith(options, 0, expectedInputFiles, expectedTestFramework, null);
+      expect(Sandbox.create).to.have.callCount(3);
+      expect(Sandbox.create).calledWith(options, 0, expectedInputFiles, expectedTestFramework, null);
     });
 
     it('should use the cpuCount when maxConcurrentTestRunners is <= 0', async () => {
       global.sandbox.stub(os, 'cpus').returns([1, 2, 3]); // stub 3 cpus
       options.maxConcurrentTestRunners = 0;
       const actual = await sut.streamSandboxes().toArray().toPromise();
-      expect(strykerSandbox.default).calledWithNew;
-      expect(strykerSandbox.default).to.have.callCount(3);
+      expect(Sandbox.create).to.have.callCount(3);
       expect(actual).lengthOf(3);
-      expect(strykerSandbox.default).calledWith(options, 0, expectedInputFiles, expectedTestFramework, null);
+      expect(Sandbox.create).calledWith(options, 0, expectedInputFiles, expectedTestFramework, null);
     });
 
     it('should use the cpuCount - 1 when a transpiler is configured', async () => {
       options.transpilers = ['a transpiler'];
       options.maxConcurrentTestRunners = 2;
       global.sandbox.stub(os, 'cpus').returns([1, 2]); // stub 2 cpus
       const actual = await sut.streamSandboxes().toArray().toPromise();
-      expect(strykerSandbox.default).to.have.callCount(1);
+      expect(Sandbox.create).to.have.callCount(1);
       expect(actual).lengthOf(1);
     });
+
+    // see https://github.com/stryker-mutator/stryker/issues/396
+    it('should dispose the newly created sandboxes if the sandbox pool is already disposed', async () => {
+      await sut.disposeAll();
+      const actualSandboxes = await sut.streamSandboxes().toArray().toPromise();
+      actualSandboxes.forEach(actual => expect(actual.dispose).called);
+      expect(actualSandboxes).to.have.length.greaterThan(0);
+    });
   });
   describe('dispose', () => {
     it('should have disposed all sandboxes', async () => {