From 80172292928f22857f916add93941fd0e88af8b8 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 17 Oct 2019 23:50:08 +0300 Subject: [PATCH] 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 --- .../file_system/src/node_js_file_system.ts | 14 +++- .../test/node_js_file_system_spec.ts | 68 ++++++++++++++++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts b/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts index 61349263cd..627a75d5d2 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts +++ b/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts @@ -36,7 +36,7 @@ export class NodeJSFileSystem implements FileSystem { path = this.dirname(path); } while (parents.length) { - this.mkdir(parents.pop() !); + this.safeMkdir(parents.pop() !); } } isCaseSensitive(): boolean { @@ -70,6 +70,18 @@ export class NodeJSFileSystem implements FileSystem { // Convert backslashes to forward slashes 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; + } + } + } } /** diff --git a/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts b/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts index 8ba2b3a69a..b1e6109cd0 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts +++ b/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ 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 {AbsoluteFsPath} from '../src/types'; @@ -144,5 +144,69 @@ describe('NodeJSFileSystem', () => { expect(existsCalls).toEqual([xyzPath, xyPath, xPath]); 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); + }); }); -}); \ No newline at end of file +});