From 1159a9b80da567ea7915d7424426aecc5d4cba0a Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Fri, 25 Sep 2020 17:32:50 +0200 Subject: [PATCH 1/6] fix(core): fix "too many open files" Fixes "too many open files" issue by limitting the number of concurrent file IO to 256 --- packages/core/src/input/InputFileResolver.ts | 21 +++++-- packages/core/src/sandbox/sandbox.ts | 14 ++++- packages/core/src/utils/fileUtils.ts | 2 + .../test/unit/input/InputFileResolver.spec.ts | 57 ++++++++++++++----- .../core/test/unit/sandbox/sandbox.spec.ts | 30 +++++++++- 5 files changed, 98 insertions(+), 26 deletions(-) diff --git a/packages/core/src/input/InputFileResolver.ts b/packages/core/src/input/InputFileResolver.ts index f377479c62..1e519ba15e 100644 --- a/packages/core/src/input/InputFileResolver.ts +++ b/packages/core/src/input/InputFileResolver.ts @@ -1,7 +1,9 @@ -import * as path from 'path'; -import { promises as fs } from 'fs'; import { StringDecoder } from 'string_decoder'; +import * as path from 'path'; +import fs = require('fs'); +import { from } from 'rxjs'; +import { filter, mergeMap, toArray } from 'rxjs/operators'; import { File, StrykerOptions } from '@stryker-mutator/api/core'; import { Logger } from '@stryker-mutator/api/logging'; import { commonTokens, tokens } from '@stryker-mutator/api/plugin'; @@ -10,7 +12,7 @@ import { childProcessAsPromised, isErrnoException, normalizeWhitespaces, Stryker import { coreTokens } from '../di'; import StrictReporter from '../reporters/StrictReporter'; -import { glob } from '../utils/fileUtils'; +import { glob, MAX_CONCURRENT_FILE_IO } from '../utils/fileUtils'; import { defaultOptions } from '../config/OptionsValidator'; import InputFileCollection from './InputFileCollection'; @@ -157,13 +159,20 @@ export default class InputFileResolver { } private async readFiles(fileNames: string[]): Promise { - const files = await Promise.all(fileNames.map((fileName) => this.readFile(fileName))); - return files.filter(notEmpty); + const files = from(fileNames) + .pipe( + mergeMap((fileName) => this.readFile(fileName), MAX_CONCURRENT_FILE_IO), + filter(notEmpty), + toArray() + ) + .toPromise(); + return files; } private async readFile(fileName: string): Promise { try { - const content = await fs.readFile(fileName); + const p = fs.promises.readFile(fileName); + const content = await p; const file = new File(fileName, content); this.reportSourceFilesRead(file); return file; diff --git a/packages/core/src/sandbox/sandbox.ts b/packages/core/src/sandbox/sandbox.ts index 7c13e4086e..8c873625bc 100644 --- a/packages/core/src/sandbox/sandbox.ts +++ b/packages/core/src/sandbox/sandbox.ts @@ -9,8 +9,12 @@ import * as mkdirp from 'mkdirp'; import { Logger, LoggerFactoryMethod } from '@stryker-mutator/api/logging'; import { tokens, commonTokens } from '@stryker-mutator/api/plugin'; +import { from } from 'rxjs'; + +import { mergeMap, toArray } from 'rxjs/operators'; + import { TemporaryDirectory } from '../utils/TemporaryDirectory'; -import { findNodeModules, symlinkJunction, writeFile } from '../utils/fileUtils'; +import { findNodeModules, MAX_CONCURRENT_FILE_IO, symlinkJunction, writeFile } from '../utils/fileUtils'; import { coreTokens } from '../di'; interface SandboxFactory { @@ -75,8 +79,12 @@ export class Sandbox { } private fillSandbox(): Promise { - const copyPromises = this.files.map((file) => this.fillFile(file)); - return Promise.all(copyPromises); + return from(this.files) + .pipe( + mergeMap((file) => this.fillFile(file), MAX_CONCURRENT_FILE_IO), + toArray() + ) + .toPromise(); } private async runBuildCommand() { diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index fe9788cccf..c25416b479 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -7,6 +7,8 @@ import * as nodeGlob from 'glob'; import * as mkdirp from 'mkdirp'; import * as rimraf from 'rimraf'; +export const MAX_CONCURRENT_FILE_IO = 256; + export function glob(expression: string): Promise { return new Promise((resolve, reject) => { nodeGlob(expression, { nodir: true }, (error, matches) => { diff --git a/packages/core/test/unit/input/InputFileResolver.spec.ts b/packages/core/test/unit/input/InputFileResolver.spec.ts index e911f06ac1..d2a1ba193e 100644 --- a/packages/core/test/unit/input/InputFileResolver.spec.ts +++ b/packages/core/test/unit/input/InputFileResolver.spec.ts @@ -1,12 +1,12 @@ import * as os from 'os'; import * as path from 'path'; -import * as fs from 'fs'; +import fs = require('fs'); import { File } from '@stryker-mutator/api/core'; import { SourceFile } from '@stryker-mutator/api/report'; -import { testInjector, factory, assertions } from '@stryker-mutator/test-helpers'; +import { testInjector, factory, assertions, tick } from '@stryker-mutator/test-helpers'; import { createIsDirError, fileNotFoundError } from '@stryker-mutator/test-helpers/src/factory'; -import { childProcessAsPromised, errorToString } from '@stryker-mutator/util'; +import { childProcessAsPromised, errorToString, Task } from '@stryker-mutator/util'; import { expect } from 'chai'; import * as sinon from 'sinon'; @@ -29,8 +29,8 @@ describe(InputFileResolver.name, () => { beforeEach(() => { reporterMock = mock(BroadcastReporter); globStub = sinon.stub(fileUtils, 'glob'); - readFileStub = sinon - .stub(fs.promises, 'readFile') + readFileStub = sinon.stub(fs.promises, 'readFile'); + readFileStub .withArgs(sinon.match.string) .resolves(Buffer.from('')) // fallback .withArgs(sinon.match('file1')) @@ -43,6 +43,7 @@ describe(InputFileResolver.name, () => { .resolves(Buffer.from('mutate 1 content')) .withArgs(sinon.match('mute2')) .resolves(Buffer.from('mutate 2 content')); + globStub.withArgs('mute*').resolves(['/mute1.js', '/mute2.js']); globStub.withArgs('mute1').resolves(['/mute1.js']); globStub.withArgs('mute2').resolves(['/mute2.js']); @@ -152,6 +153,43 @@ describe(InputFileResolver.name, () => { ]); }); + it('should reject when a globbing expression results in a reject', () => { + testInjector.options.files = ['fileError', 'fileError']; + testInjector.options.mutate = ['file1']; + sut = createSut(); + const expectedError = new Error('ERROR: something went wrong'); + globStub.withArgs('fileError').rejects(expectedError); + return expect(sut.resolve()).rejectedWith(expectedError); + }); + + it('should not open too many file handles', async () => { + // Arrange + const maxFileIO = 256; + sut = createSut(); + const fileHandles: Array<{ fileName: string; task: Task }> = []; + for (let i = 0; i < maxFileIO + 1; i++) { + const fileName = `file_${i}.js`; + const readFileTask = new Task(); + fileHandles.push({ fileName, task: readFileTask }); + readFileStub.withArgs(sinon.match(fileName)).returns(readFileTask.promise); + } + childProcessExecStub.resolves({ + stdout: Buffer.from(fileHandles.map(({ fileName }) => fileName).join(os.EOL)), + }); + + // Act + const onGoingWork = sut.resolve(); + await tick(); + expect(readFileStub).callCount(maxFileIO); + fileHandles[0].task.resolve(Buffer.from('content')); + await tick(); + + // Assert + expect(readFileStub).callCount(maxFileIO + 1); + fileHandles.forEach(({ task }) => task.resolve(Buffer.from('content'))); + await onGoingWork; + }); + describe('with mutate file expressions', () => { it('should result in the expected mutate files', async () => { testInjector.options.mutate = ['mute*']; @@ -256,15 +294,6 @@ describe(InputFileResolver.name, () => { }); }); - it('should reject when a globbing expression results in a reject', () => { - testInjector.options.files = ['fileError', 'fileError']; - testInjector.options.mutate = ['file1']; - sut = createSut(); - const expectedError = new Error('ERROR: something went wrong'); - globStub.withArgs('fileError').rejects(expectedError); - return expect(sut.resolve()).rejectedWith(expectedError); - }); - describe('when excluding files with "!"', () => { it('should exclude the files that were previously included', async () => { testInjector.options.files = ['file2', 'file1', '!file2']; diff --git a/packages/core/test/unit/sandbox/sandbox.spec.ts b/packages/core/test/unit/sandbox/sandbox.spec.ts index 84bbda1cda..c9b055d2fd 100644 --- a/packages/core/test/unit/sandbox/sandbox.spec.ts +++ b/packages/core/test/unit/sandbox/sandbox.spec.ts @@ -6,10 +6,10 @@ import { expect } from 'chai'; import sinon = require('sinon'); import * as mkdirp from 'mkdirp'; -import { testInjector } from '@stryker-mutator/test-helpers'; +import { testInjector, tick } from '@stryker-mutator/test-helpers'; import { File } from '@stryker-mutator/api/core'; import { fileAlreadyExistsError } from '@stryker-mutator/test-helpers/src/factory'; -import { normalizeWhitespaces } from '@stryker-mutator/util'; +import { normalizeWhitespaces, Task } from '@stryker-mutator/util'; import { Sandbox } from '../../../src/sandbox/sandbox'; import { coreTokens } from '../../../src/di'; @@ -80,7 +80,31 @@ describe(Sandbox.name, () => { it('should be able to copy a local file', async () => { files.push(new File('localFile.txt', 'foobar')); await createSut(); - expect(fileUtils.writeFile).calledWith(path.join(SANDBOX_WORKING_DIR, 'localFile.txt'), Buffer.from('foobar')); + expect(writeFileStub).calledWith(path.join(SANDBOX_WORKING_DIR, 'localFile.txt'), Buffer.from('foobar')); + }); + + it('should not open too many file handles', async () => { + const maxFileIO = 256; + const fileHandles: Array<{ fileName: string; task: Task }> = []; + for (let i = 0; i < maxFileIO + 1; i++) { + const fileName = `file_${i}.js`; + const task = new Task(); + fileHandles.push({ fileName, task }); + writeFileStub.withArgs(sinon.match(fileName)).returns(task.promise); + files.push(new File(fileName, '')); + } + + // Act + const onGoingWork = createSut(); + await tick(); + expect(writeFileStub).callCount(maxFileIO); + fileHandles[0].task.resolve(); + await tick(); + + // Assert + expect(writeFileStub).callCount(maxFileIO + 1); + fileHandles.forEach(({ task }) => task.resolve()); + await onGoingWork; }); it('should symlink node modules in sandbox directory if exists', async () => { From f459b7c84acd3111c7dc43f9350185a00162528d Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Fri, 25 Sep 2020 17:38:42 +0200 Subject: [PATCH 2/6] One liner await --- packages/core/src/input/InputFileResolver.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core/src/input/InputFileResolver.ts b/packages/core/src/input/InputFileResolver.ts index 1e519ba15e..340e616afc 100644 --- a/packages/core/src/input/InputFileResolver.ts +++ b/packages/core/src/input/InputFileResolver.ts @@ -171,8 +171,7 @@ export default class InputFileResolver { private async readFile(fileName: string): Promise { try { - const p = fs.promises.readFile(fileName); - const content = await p; + const content = await fs.promises.readFile(fileName); const file = new File(fileName, content); this.reportSourceFilesRead(file); return file; From abf817b122ddc5337306c894655a6c6dd056d79d Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Fri, 25 Sep 2020 17:50:39 +0200 Subject: [PATCH 3/6] fix unit test --- packages/core/src/sandbox/sandbox.ts | 8 +++----- packages/core/test/unit/sandbox/sandbox.spec.ts | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/core/src/sandbox/sandbox.ts b/packages/core/src/sandbox/sandbox.ts index 8c873625bc..7a44338c42 100644 --- a/packages/core/src/sandbox/sandbox.ts +++ b/packages/core/src/sandbox/sandbox.ts @@ -8,10 +8,8 @@ import { normalizeWhitespaces, I } from '@stryker-mutator/util'; import * as mkdirp from 'mkdirp'; import { Logger, LoggerFactoryMethod } from '@stryker-mutator/api/logging'; import { tokens, commonTokens } from '@stryker-mutator/api/plugin'; - -import { from } from 'rxjs'; - import { mergeMap, toArray } from 'rxjs/operators'; +import { from } from 'rxjs'; import { TemporaryDirectory } from '../utils/TemporaryDirectory'; import { findNodeModules, MAX_CONCURRENT_FILE_IO, symlinkJunction, writeFile } from '../utils/fileUtils'; @@ -119,12 +117,12 @@ export class Sandbox { } } - private fillFile(file: File): Promise { + private async fillFile(file: File): Promise { const relativePath = path.relative(process.cwd(), file.name); const folderName = path.join(this.workingDirectory, path.dirname(relativePath)); mkdirp.sync(folderName); const targetFileName = path.join(folderName, path.basename(relativePath)); this.fileMap.set(file.name, targetFileName); - return writeFile(targetFileName, file.content); + await writeFile(targetFileName, file.content); } } diff --git a/packages/core/test/unit/sandbox/sandbox.spec.ts b/packages/core/test/unit/sandbox/sandbox.spec.ts index c9b055d2fd..04aed65de5 100644 --- a/packages/core/test/unit/sandbox/sandbox.spec.ts +++ b/packages/core/test/unit/sandbox/sandbox.spec.ts @@ -5,7 +5,6 @@ import npmRunPath = require('npm-run-path'); import { expect } from 'chai'; import sinon = require('sinon'); import * as mkdirp from 'mkdirp'; - import { testInjector, tick } from '@stryker-mutator/test-helpers'; import { File } from '@stryker-mutator/api/core'; import { fileAlreadyExistsError } from '@stryker-mutator/test-helpers/src/factory'; From d00f16f518ef460914438605dad452d4de842ea6 Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Fri, 25 Sep 2020 18:03:44 +0200 Subject: [PATCH 4/6] test(input-file): don't rely on os ordering --- .../core/test/integration/input/InputFileResolver.it.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/core/test/integration/input/InputFileResolver.it.spec.ts b/packages/core/test/integration/input/InputFileResolver.it.spec.ts index 0f464608c7..19225b405b 100644 --- a/packages/core/test/integration/input/InputFileResolver.it.spec.ts +++ b/packages/core/test/integration/input/InputFileResolver.it.spec.ts @@ -29,10 +29,11 @@ describe(`${InputFileResolver.name} integration`, () => { process.chdir(originalCwd); }); - it('should by default resolve reasonable project source files to be mutated', async () => { + it.only('should by default resolve reasonable project source files to be mutated', async () => { process.chdir(resolveTestResource()); const inputFiles = await sut.resolve(); - expect(inputFiles.filesToMutate.map((file) => file.name)).deep.eq([ + // eslint-disable-next-line @typescript-eslint/require-array-sort-compare + expect(inputFiles.filesToMutate.map((file) => file.name).sort()).deep.eq([ resolveTestResource('lib', 'string-utils.js'), resolveTestResource('src', 'app.ts'), resolveTestResource('src', 'components', 'calculator', 'calculator.component.tsx'), From d53cb562ec7e12615b9e51324defb43ab5477552 Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Fri, 25 Sep 2020 18:09:49 +0200 Subject: [PATCH 5/6] Remove `.only` --- .../core/test/integration/input/InputFileResolver.it.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/integration/input/InputFileResolver.it.spec.ts b/packages/core/test/integration/input/InputFileResolver.it.spec.ts index 19225b405b..c4a8d36d39 100644 --- a/packages/core/test/integration/input/InputFileResolver.it.spec.ts +++ b/packages/core/test/integration/input/InputFileResolver.it.spec.ts @@ -29,7 +29,7 @@ describe(`${InputFileResolver.name} integration`, () => { process.chdir(originalCwd); }); - it.only('should by default resolve reasonable project source files to be mutated', async () => { + it('should by default resolve reasonable project source files to be mutated', async () => { process.chdir(resolveTestResource()); const inputFiles = await sut.resolve(); // eslint-disable-next-line @typescript-eslint/require-array-sort-compare From 57108fc16a60a6ad4761747b221f330e9aaea6fd Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Fri, 25 Sep 2020 18:37:51 +0200 Subject: [PATCH 6/6] deterministic order of mutants --- packages/core/src/input/InputFileResolver.ts | 6 ++++-- .../input/InputFileResolver.it.spec.ts | 3 +-- .../test/unit/input/InputFileResolver.spec.ts | 18 +++++++++--------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/core/src/input/InputFileResolver.ts b/packages/core/src/input/InputFileResolver.ts index 340e616afc..17c7b74a43 100644 --- a/packages/core/src/input/InputFileResolver.ts +++ b/packages/core/src/input/InputFileResolver.ts @@ -3,7 +3,7 @@ import * as path from 'path'; import fs = require('fs'); import { from } from 'rxjs'; -import { filter, mergeMap, toArray } from 'rxjs/operators'; +import { filter, map, mergeMap, toArray } from 'rxjs/operators'; import { File, StrykerOptions } from '@stryker-mutator/api/core'; import { Logger } from '@stryker-mutator/api/logging'; import { commonTokens, tokens } from '@stryker-mutator/api/plugin'; @@ -163,7 +163,9 @@ export default class InputFileResolver { .pipe( mergeMap((fileName) => this.readFile(fileName), MAX_CONCURRENT_FILE_IO), filter(notEmpty), - toArray() + toArray(), + // Filter the files here, so we force a deterministic instrumentation process + map((files) => files.sort((a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0))) ) .toPromise(); return files; diff --git a/packages/core/test/integration/input/InputFileResolver.it.spec.ts b/packages/core/test/integration/input/InputFileResolver.it.spec.ts index c4a8d36d39..0f464608c7 100644 --- a/packages/core/test/integration/input/InputFileResolver.it.spec.ts +++ b/packages/core/test/integration/input/InputFileResolver.it.spec.ts @@ -32,8 +32,7 @@ describe(`${InputFileResolver.name} integration`, () => { it('should by default resolve reasonable project source files to be mutated', async () => { process.chdir(resolveTestResource()); const inputFiles = await sut.resolve(); - // eslint-disable-next-line @typescript-eslint/require-array-sort-compare - expect(inputFiles.filesToMutate.map((file) => file.name).sort()).deep.eq([ + expect(inputFiles.filesToMutate.map((file) => file.name)).deep.eq([ resolveTestResource('lib', 'string-utils.js'), resolveTestResource('src', 'app.ts'), resolveTestResource('src', 'components', 'calculator', 'calculator.component.tsx'), diff --git a/packages/core/test/unit/input/InputFileResolver.spec.ts b/packages/core/test/unit/input/InputFileResolver.spec.ts index d2a1ba193e..150ad62bad 100644 --- a/packages/core/test/unit/input/InputFileResolver.spec.ts +++ b/packages/core/test/unit/input/InputFileResolver.spec.ts @@ -147,9 +147,9 @@ describe(InputFileResolver.name, () => { }); const files = await sut.resolve(); assertions.expectTextFilesEqual(files.files, [ - new File(path.resolve('å.js'), ''), - new File(path.resolve('src/🐱‍👓ninja.cat.js'), ''), new File(path.resolve('a\\test\\file.js'), ''), + new File(path.resolve('src/🐱‍👓ninja.cat.js'), ''), + new File(path.resolve('å.js'), ''), ]); }); @@ -199,10 +199,10 @@ describe(InputFileResolver.name, () => { expect(result.filesToMutate.map((_) => _.name)).to.deep.equal([path.resolve('/mute1.js'), path.resolve('/mute2.js')]); expect(result.files.map((file) => file.name)).to.deep.equal([ path.resolve('/file1.js'), - path.resolve('/mute1.js'), path.resolve('/file2.js'), - path.resolve('/mute2.js'), path.resolve('/file3.js'), + path.resolve('/mute1.js'), + path.resolve('/mute2.js'), ]); }); @@ -221,10 +221,10 @@ describe(InputFileResolver.name, () => { await sut.resolve(); const expected: SourceFile[] = [ { path: path.resolve('/file1.js'), content: 'file 1 content' }, - { path: path.resolve('/mute1.js'), content: 'mutate 1 content' }, { path: path.resolve('/file2.js'), content: 'file 2 content' }, - { path: path.resolve('/mute2.js'), content: 'mutate 2 content' }, { path: path.resolve('/file3.js'), content: 'file 3 content' }, + { path: path.resolve('/mute1.js'), content: 'mutate 1 content' }, + { path: path.resolve('/mute2.js'), content: 'mutate 2 content' }, ]; expect(reporterMock.onAllSourceFilesRead).calledWith(expected); }); @@ -236,10 +236,10 @@ describe(InputFileResolver.name, () => { await sut.resolve(); const expected: SourceFile[] = [ { path: path.resolve('/file1.js'), content: 'file 1 content' }, - { path: path.resolve('/mute1.js'), content: 'mutate 1 content' }, { path: path.resolve('/file2.js'), content: 'file 2 content' }, - { path: path.resolve('/mute2.js'), content: 'mutate 2 content' }, { path: path.resolve('/file3.js'), content: 'file 3 content' }, + { path: path.resolve('/mute1.js'), content: 'mutate 1 content' }, + { path: path.resolve('/mute2.js'), content: 'mutate 2 content' }, ]; expected.forEach((sourceFile) => expect(reporterMock.onSourceFileRead).calledWith(sourceFile)); }); @@ -333,7 +333,7 @@ describe(InputFileResolver.name, () => { it('should order files by expression order', async () => { testInjector.options.files = ['file2', 'file*']; const result = await createSut().resolve(); - assertFilesEqual(result.files, files(['/file2.js', 'file 2 content'], ['/file1.js', 'file 1 content'], ['/file3.js', 'file 3 content'])); + assertFilesEqual(result.files, files(['/file1.js', 'file 1 content'], ['/file2.js', 'file 2 content'], ['/file3.js', 'file 3 content'])); }); });