fix(ngcc): do not fail when multiple workers try to create the same directory (#33237)

When `ngcc` is running in parallel mode (usually when run from the
command line) and the `createNewEntryPointFormats` option is set to true
(e.g. via the `--create-ivy-entry-points` command line option), it can
happen that two workers end up trying to create the same directory at
the same time. This can lead to a race condition, where both check for
the directory existence, see that the directory does not exist and both
try to create it, with the second failing due the directory's having
already been created by the first one. Note that this only affects
directories and not files, because `ngcc` tasks operate on different
sets of files.

This commit avoids this race condition by allowing `FileSystem`'s
`ensureDir()` method to not fail if one of the directories it is trying
to create already exists (and is indeed a directory). This is fine for
the `ensureDir()` method, since it's purpose is to ensure that the
specified directory exists. So, even if the `mkdir()` call failed
(because the directory exists), `ensureDir()` has still completed its
mission.

Related discussion: https://github.com/angular/angular/pull/33049#issuecomment-540485703
FW-1635 #resolve

PR Close #33237
This commit is contained in:
George Kalpakas 2019-10-17 23:50:08 +03:00 committed by Matias Niemelä
parent 755a80c7ec
commit 8017229292
2 changed files with 79 additions and 3 deletions

View File

@ -36,7 +36,7 @@ export class NodeJSFileSystem implements FileSystem {
path = this.dirname(path); path = this.dirname(path);
} }
while (parents.length) { while (parents.length) {
this.mkdir(parents.pop() !); this.safeMkdir(parents.pop() !);
} }
} }
isCaseSensitive(): boolean { isCaseSensitive(): boolean {
@ -70,6 +70,18 @@ export class NodeJSFileSystem implements FileSystem {
// Convert backslashes to forward slashes // Convert backslashes to forward slashes
return path.replace(/\\/g, '/') as T; return path.replace(/\\/g, '/') as T;
} }
private safeMkdir(path: AbsoluteFsPath): void {
try {
this.mkdir(path);
} catch (err) {
// Ignore the error, if the path already exists and points to a directory.
// Re-throw otherwise.
if (!this.exists(path) || !this.stat(path).isDirectory()) {
throw err;
}
}
}
} }
/** /**

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import * as realFs from 'fs'; import * as realFs from 'fs';
import {absoluteFrom, relativeFrom, setFileSystem} from '../src/helpers'; import {absoluteFrom, dirname, relativeFrom, setFileSystem} from '../src/helpers';
import {NodeJSFileSystem} from '../src/node_js_file_system'; import {NodeJSFileSystem} from '../src/node_js_file_system';
import {AbsoluteFsPath} from '../src/types'; import {AbsoluteFsPath} from '../src/types';
@ -144,5 +144,69 @@ describe('NodeJSFileSystem', () => {
expect(existsCalls).toEqual([xyzPath, xyPath, xPath]); expect(existsCalls).toEqual([xyzPath, xyPath, xPath]);
expect(mkdirCalls).toEqual([xPath, xyPath, xyzPath]); expect(mkdirCalls).toEqual([xPath, xyPath, xyzPath]);
}); });
it('should not fail if a directory (that did not exist before) does exist when trying to create it',
() => {
let abcPathExists = false;
spyOn(fs, 'exists').and.callFake((path: AbsoluteFsPath) => {
if (path === abcPath) {
// Pretend `abcPath` is created (e.g. by another process) right after we check if it
// exists for the first time.
const exists = abcPathExists;
abcPathExists = true;
return exists;
}
return false;
});
spyOn(fs, 'stat').and.returnValue({isDirectory: () => true});
const mkdirSyncSpy = spyOn(realFs, 'mkdirSync').and.callFake((path: string) => {
if (path === abcPath) {
throw new Error('It exists already. Supposedly.');
}
});
fs.ensureDir(abcPath);
expect(mkdirSyncSpy).toHaveBeenCalledTimes(3);
expect(mkdirSyncSpy).toHaveBeenCalledWith(abcPath);
expect(mkdirSyncSpy).toHaveBeenCalledWith(dirname(abcPath));
});
it('should fail if creating the directory throws and the directory does not exist', () => {
spyOn(fs, 'exists').and.returnValue(false);
spyOn(realFs, 'mkdirSync').and.callFake((path: string) => {
if (path === abcPath) {
throw new Error('Unable to create directory (for whatever reason).');
}
});
expect(() => fs.ensureDir(abcPath))
.toThrowError('Unable to create directory (for whatever reason).');
});
it('should fail if creating the directory throws and the path points to a file', () => {
const isDirectorySpy = jasmine.createSpy('isDirectory').and.returnValue(false);
let abcPathExists = false;
spyOn(fs, 'exists').and.callFake((path: AbsoluteFsPath) => {
if (path === abcPath) {
// Pretend `abcPath` is created (e.g. by another process) right after we check if it
// exists for the first time.
const exists = abcPathExists;
abcPathExists = true;
return exists;
}
return false;
});
spyOn(fs, 'stat').and.returnValue({isDirectory: isDirectorySpy});
spyOn(realFs, 'mkdirSync').and.callFake((path: string) => {
if (path === abcPath) {
throw new Error('It exists already. Supposedly.');
}
});
expect(() => fs.ensureDir(abcPath)).toThrowError('It exists already. Supposedly.');
expect(isDirectorySpy).toHaveBeenCalledTimes(1);
});
}); });
}); });