fix(Sandbox pool): remove race condition (#714)

Remove a race condition where the initalization of a sandbox takes longer than it takes to run stryker. Fixes #713
stryker-mutator · Apr 20, 2018 · a3606d8 · a3606d8
1 parent 54cbe58
commit a3606d8
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 23 deletions.
diff --git a/packages/stryker/src/SandboxPool.ts b/packages/stryker/src/SandboxPool.ts
@@ -9,8 +9,7 @@ import Sandbox from './Sandbox';
 export default class SandboxPool {
 
   private readonly log = getLogger(SandboxPool.name);
-  private readonly sandboxes: Sandbox[] = [];
-  private isDisposed: boolean = false;
+  private readonly sandboxes: Promise<Sandbox>[] = [];
 
   constructor(private options: Config, private testFramework: TestFramework | null, private initialFiles: ReadonlyArray<File>) {
   }
@@ -37,20 +36,11 @@ export default class SandboxPool {
   }
 
   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;
-    });
+    this.sandboxes.push(promisedSandbox);
+    return promisedSandbox;
   }
 
   public disposeAll() {
-    this.isDisposed = true;
-    return Promise.all(this.sandboxes.map(sandbox => sandbox.dispose()));
+    return Promise.all(this.sandboxes.map(promisedSandbox => promisedSandbox.then(sandbox => sandbox.dispose())));
   }
 }
diff --git a/packages/stryker/test/unit/SandboxPoolSpec.ts b/packages/stryker/test/unit/SandboxPoolSpec.ts
@@ -7,6 +7,7 @@ import { TestFramework } from 'stryker-api/test_framework';
 import { Mock, mock, testFramework, file, config } from '../helpers/producers';
 import Sandbox from '../../src/Sandbox';
 import '../helpers/globals';
+import Task from '../../src/utils/Task';
 
 describe('SandboxPool', () => {
   let sut: SandboxPool;
@@ -15,6 +16,7 @@ describe('SandboxPool', () => {
   let options: Config;
   let expectedTestFramework: TestFramework;
   let expectedInputFiles: File[];
+  let createStub: sinon.SinonStub;
 
   beforeEach(() => {
     options = config();
@@ -25,7 +27,7 @@ describe('SandboxPool', () => {
     secondSandbox.dispose.resolves();
     const genericSandboxForAllSubsequentCallsToNewSandbox = mock<Sandbox>(Sandbox);
     genericSandboxForAllSubsequentCallsToNewSandbox.dispose.resolves();
-    global.sandbox.stub(Sandbox, 'create')
+    createStub = global.sandbox.stub(Sandbox, 'create')
       .resolves(genericSandboxForAllSubsequentCallsToNewSandbox)
       .onCall(0).resolves(firstSandbox)
       .onCall(1).resolves(secondSandbox);
@@ -68,14 +70,6 @@ describe('SandboxPool', () => {
       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 () => {
@@ -90,6 +84,34 @@ describe('SandboxPool', () => {
       expect(firstSandbox.dispose).not.called;
       expect(secondSandbox.dispose).not.called;
     });
+
+    it('should not resolve when there are still sandboxes being created (issue #713)', async () => {
+      // Arrange
+      global.sandbox.stub(os, 'cpus').returns([1, 2, 3]); // stub 3 cpus
+      const task = new Task<Sandbox>();
+      createStub.onCall(2).returns(task.promise); // promise is not yet resolved
+      const registeredSandboxes: Sandbox[] = [];
+      let disposeAllResolved = false;
+      await sut.streamSandboxes().flatMap(async sandbox => {
+        if (registeredSandboxes.push(sandbox) === 2) {
+          // Act: The last sandbox will take a while to resolve (it is not yet created)
+          const disposeAllPromise = sut.disposeAll().then(_ => disposeAllResolved = true);
+          await tick();
+
+          // Assert: dispose should not have resolved yet, because last sandbox is not created yet
+          expect(disposeAllResolved).not.ok;
+          task.resolve(mock(Sandbox) as any); // Make sure it finally is resolved
+          await disposeAllPromise;
+        }
+      }).toArray().toPromise();
+
+    });
   });
 });
 
+function tick() {
+  return new Promise(res => {
+    setTimeout(res, 0);
+  });
+}
+