feat(mutants): Prevent memory leak when transpiling mutants #1376
Conversation
Instead of transpiling all mutants at once, this PR makes it so that at most 100 mutants are transpiled at any one time. After that, it waits for the mutants to be tested before transpiling any more of them. Fixes #920
ghost
assigned 
| this.workingDirectory = TempFolder.instance().createRandomFolder('sandbox'); | ||
| this.log.debug('Creating a sandbox for files in %s', this.workingDirectory); | ||
| this.files = files.slice(); // Create a copy |
There was a problem hiding this comment.
Why do we no longer have to make a slice?
There was a problem hiding this comment.
It's no longer needed. We no longer mutate the original (it is a ReadonlyArray)
packages/stryker/src/SandboxPool.ts
Outdated
| const backlogItem = this.backlog.shift(); | ||
| if (backlogItem) { | ||
| if (this.isDisposed) { | ||
| backlogItem.task.resolve({ status: RunStatus.Timeout, tests: [] }); |
There was a problem hiding this comment.
So if the entire pool is disposed you resolve the last backlog item as a Timeout?
There was a problem hiding this comment.
All remaining backlog items are timedout. We need another way of dealing with this, the problem is I don't know exactly how. It is non-trivial functionality.
Note: this shouldn't happen. We only dispose the sandbox pool once all results are in. We'll be able to remove this code once we make typed-inject responsible to dispose our stuff.
| tap(this.reportAll) | ||
| ).toPromise(); | ||
|
|
||
| // TODO: Let typed inject dispose of sandbox pool |
There was a problem hiding this comment.
Yes it is. Once all disposable instances are created with typed-inject we can use it to dispose all our stuff. As for now, we cannot do that, so that's what this hack is for.
NOTE: this functionality didn't change. Disposing was always done at this point.
| if (recycleObserver) { | ||
| recycleObserver.complete(); | ||
| } | ||
| const early = this.earlyResult(transpiledMutant); |
There was a problem hiding this comment.
Could you rename this to const earlyResult = this.getEarlyResult(transpiledMutant);?
There was a problem hiding this comment.
I don't like the name getEarlyResult (Java has pretty much ruined getXxx for me 🙈) . I'll change it to retrieveEarlyResult.
| */ | ||
| public dispose() { | ||
| this.concurrencyTicket$.complete(); | ||
| // TODO: Let typed-inject dispose this one |
There was a problem hiding this comment.
Yes it is. Once all disposable instances are created with typed-inject we can use it to dispose all our stuff. As for now, we cannot do that, so that's what this hack is for.
|
I've commented on / fixed all remarks. Thanks for the review |
Instead of transpiling all mutants at once, this PR makes it so that at
most 100 mutants are transpiled at any one time.
After that, it waits for the mutants to be tested before transpiling any more of them.
Fixes #920