diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts b/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts index 8479c82625..c601b889a6 100644 --- a/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts +++ b/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts @@ -13,7 +13,7 @@ import * as cluster from 'cluster'; import {Logger} from '../../logging/logger'; import {PackageJsonUpdater} from '../../writing/package_json_updater'; import {AnalyzeEntryPointsFn, CreateCompileFn, Executor} from '../api'; -import {LockFile} from '../lock_file'; +import {LockFileAsync} from '../lock_file'; import {ClusterMaster} from './master'; import {ClusterWorker} from './worker'; @@ -26,7 +26,7 @@ import {ClusterWorker} from './worker'; export class ClusterExecutor implements Executor { constructor( private workerCount: number, private logger: Logger, - private pkgJsonUpdater: PackageJsonUpdater, private lockFile: LockFile) {} + private pkgJsonUpdater: PackageJsonUpdater, private lockFile: LockFileAsync) {} async execute(analyzeEntryPoints: AnalyzeEntryPointsFn, createCompileFn: CreateCompileFn): Promise { diff --git a/packages/compiler-cli/ngcc/src/execution/lock_file.ts b/packages/compiler-cli/ngcc/src/execution/lock_file.ts index 30efb22f46..38883beeec 100644 --- a/packages/compiler-cli/ngcc/src/execution/lock_file.ts +++ b/packages/compiler-cli/ngcc/src/execution/lock_file.ts @@ -6,78 +6,44 @@ * found in the LICENSE file at https://angular.io/license */ import * as process from 'process'; -import {FileSystem} from '../../../src/ngtsc/file_system'; -/** - * The LockFile is used to prevent more than one instance of ngcc executing at the same time. - * - * When ngcc starts executing, it creates a file in the `compiler-cli/ngcc` folder. If it finds one - * is already there then it fails with a suitable error message. - * When ngcc completes executing, it removes the file so that future ngcc executions can start. - */ -export class LockFile { +import {CachedFileSystem, FileSystem} from '../../../src/ngtsc/file_system'; +import {Logger} from '../logging/logger'; + +export abstract class LockFileBase { lockFilePath = this.fs.resolve(require.resolve('@angular/compiler-cli/ngcc'), '../__ngcc_lock_file__'); - constructor(private fs: FileSystem) {} + constructor(protected fs: FileSystem) {} - /** - * Run a function guarded by the lock file. - * - * Note that T can be a Promise. If so, we run the `remove()` call in the promise's `finally` - * handler. Otherwise we run the `remove()` call in the `try...finally` block. - * - * @param fn The function to run. - */ - lock(fn: () => T): T { - let isAsync = false; - this.create(); + protected writeLockFile(): void { try { - const result = fn(); - if (result instanceof Promise) { - isAsync = true; - // The cast is necessary because TS cannot deduce that T is now a promise here. - return result.finally(() => this.remove()) as unknown as T; - } else { - return result; - } - } finally { - if (!isAsync) { - this.remove(); - } + this.addSignalHandlers(); + // To avoid race conditions, we check for existence of the lockfile + // by actually trying to create it exclusively. + return this.fs.writeFile(this.lockFilePath, process.pid.toString(), /* exclusive */ true); + } catch (e) { + this.removeSignalHandlers(); + throw e; } } /** - * Write a lock file to disk, or error if there is already one there. + * Read the pid from the lockfile. + * + * It is feasible that the lockfile was removed between the previous check for existence + * and this file-read. If so then we still error but as gracefully as possible. */ - protected create() { + protected readLockFile(): string { try { - this.addSignalHandlers(); - // To avoid race conditions, we check for existence of the lockfile - // by actually trying to create it exclusively - this.fs.writeFile(this.lockFilePath, process.pid.toString(), /* exclusive */ true); - } catch (e) { - this.removeSignalHandlers(); - if (e.code !== 'EEXIST') { - throw e; + if (this.fs instanceof CachedFileSystem) { + // This file is "volatile", it might be changed by an external process, + // so we cannot rely upon the cached value when reading it. + this.fs.invalidateCaches(this.lockFilePath); } - - // The lockfile already exists so raise a helpful error. - // It is feasible that the lockfile was removed between the previous check for existence - // and this file-read. If so then we still error but as gracefully as possible. - let pid: string; - try { - pid = this.fs.readFile(this.lockFilePath); - } catch { - pid = '{unknown}'; - } - - throw new Error( - `ngcc is already running at process with id ${pid}.\n` + - `If you are running multiple builds in parallel then you should pre-process your node_modules via the command line ngcc tool before starting the builds;\n` + - `See https://v9.angular.io/guide/ivy#speeding-up-ngcc-compilation.\n` + - `(If you are sure no ngcc process is running then you should delete the lockfile at ${this.lockFilePath}.)`); + return this.fs.readFile(this.lockFilePath); + } catch { + return '{unknown}'; } } @@ -91,18 +57,25 @@ export class LockFile { } } + /** + * Capture CTRL-C and terminal closing events. + * When these occur we remove the lockfile and exit. + */ protected addSignalHandlers() { - process.once('SIGINT', this.signalHandler); - process.once('SIGHUP', this.signalHandler); + process.addListener('SIGINT', this.signalHandler); + process.addListener('SIGHUP', this.signalHandler); } + /** + * Clear the event handlers to prevent leakage. + */ protected removeSignalHandlers() { process.removeListener('SIGINT', this.signalHandler); process.removeListener('SIGHUP', this.signalHandler); } /** - * This handle needs to be defined as a property rather than a method + * This handler needs to be defined as a property rather than a method * so that it can be passed around as a bound function. */ protected signalHandler = @@ -119,3 +92,113 @@ export class LockFile { process.exit(code); } } + +/** + * LockFileSync is used to prevent more than one instance of ngcc executing at the same time, + * when being called in a synchronous context. + * + * * When ngcc starts executing, it creates a file in the `compiler-cli/ngcc` folder. + * * If it finds one is already there then it fails with a suitable error message. + * * When ngcc completes executing, it removes the file so that future ngcc executions can start. + */ +export class LockFileSync extends LockFileBase { + /** + * Run the given function guarded by the lock file. + * + * @param fn the function to run. + * @returns the value returned from the `fn` call. + */ + lock(fn: () => T): T { + this.create(); + try { + return fn(); + } finally { + this.remove(); + } + } + + /** + * Write a lock file to disk, or error if there is already one there. + */ + protected create(): void { + try { + this.writeLockFile(); + } catch (e) { + if (e.code !== 'EEXIST') { + throw e; + } + this.handleExistingLockFile(); + } + } + + /** + * The lockfile already exists so raise a helpful error. + */ + protected handleExistingLockFile(): void { + const pid = this.readLockFile(); + throw new Error( + `ngcc is already running at process with id ${pid}.\n` + + `If you are running multiple builds in parallel then you should pre-process your node_modules via the command line ngcc tool before starting the builds;\n` + + `See https://v9.angular.io/guide/ivy#speeding-up-ngcc-compilation.\n` + + `(If you are sure no ngcc process is running then you should delete the lockfile at ${this.lockFilePath}.)`); + } +} + +/** + * LockFileAsync is used to prevent more than one instance of ngcc executing at the same time, + * when being called in an asynchronous context. + * + * * When ngcc starts executing, it creates a file in the `compiler-cli/ngcc` folder. + * * If it finds one is already there then it pauses and waits for the file to be removed by the + * other process. If the file is not removed within a set timeout period given by + * `retryDelay*retryAttempts` an error is thrown with a suitable error message. + * * If the process locking the file changes, then we restart the timeout. + * * When ngcc completes executing, it removes the file so that future ngcc executions can start. + */ +export class LockFileAsync extends LockFileBase { + constructor( + fs: FileSystem, protected logger: Logger, private retryDelay: number, + private retryAttempts: number) { + super(fs); + } + + /** + * Run a function guarded by the lock file. + * + * @param fn The function to run. + */ + async lock(fn: () => Promise): Promise { + await this.create(); + return await fn().finally(() => this.remove()); + } + + protected async create() { + let pid: string = ''; + for (let attempts = 0; attempts < this.retryAttempts; attempts++) { + try { + return this.writeLockFile(); + } catch (e) { + if (e.code !== 'EEXIST') { + throw e; + } + const newPid = this.readLockFile(); + if (newPid !== pid) { + // The process locking the file has changed, so restart the timeout + attempts = 0; + pid = newPid; + } + if (attempts === 0) { + this.logger.info( + `Another process, with id ${pid}, is currently running ngcc.\n` + + `Waiting up to ${this.retryDelay*this.retryAttempts/1000}s for it to finish.`); + } + // The file is still locked by another process so wait for a bit and retry + await new Promise(resolve => setTimeout(resolve, this.retryDelay)); + } + } + // If we fall out of the loop then we ran out of rety attempts + throw new Error( + `Timed out waiting ${this.retryAttempts * this.retryDelay/1000}s for another ngcc process, with id ${pid}, to complete.\n` + + `(If you are sure no ngcc process is running then you should delete the lockfile at ${this.lockFilePath}.)`); + } +} diff --git a/packages/compiler-cli/ngcc/src/execution/single_process_executor.ts b/packages/compiler-cli/ngcc/src/execution/single_process_executor.ts index d06dceab3c..29775ae068 100644 --- a/packages/compiler-cli/ngcc/src/execution/single_process_executor.ts +++ b/packages/compiler-cli/ngcc/src/execution/single_process_executor.ts @@ -10,47 +10,56 @@ import {Logger} from '../logging/logger'; import {PackageJsonUpdater} from '../writing/package_json_updater'; import {AnalyzeEntryPointsFn, CreateCompileFn, Executor} from './api'; -import {LockFile} from './lock_file'; +import {LockFileAsync, LockFileSync} from './lock_file'; import {onTaskCompleted} from './utils'; +export abstract class SingleProcessorExecutorBase { + constructor(private logger: Logger, private pkgJsonUpdater: PackageJsonUpdater) {} + + doExecute(analyzeEntryPoints: AnalyzeEntryPointsFn, createCompileFn: CreateCompileFn): + void|Promise { + this.logger.debug(`Running ngcc on ${this.constructor.name}.`); + + const taskQueue = analyzeEntryPoints(); + const compile = + createCompileFn((task, outcome) => onTaskCompleted(this.pkgJsonUpdater, task, outcome)); + + // Process all tasks. + this.logger.debug('Processing tasks...'); + const startTime = Date.now(); + + while (!taskQueue.allTasksCompleted) { + const task = taskQueue.getNextTask() !; + compile(task); + taskQueue.markTaskCompleted(task); + } + + const duration = Math.round((Date.now() - startTime) / 1000); + this.logger.debug(`Processed tasks in ${duration}s.`); + } +} /** * An `Executor` that processes all tasks serially and completes synchronously. */ -export class SingleProcessExecutor implements Executor { - constructor( - private logger: Logger, private pkgJsonUpdater: PackageJsonUpdater, - private lockFile: LockFile) {} - +export class SingleProcessExecutorSync extends SingleProcessorExecutorBase implements Executor { + constructor(logger: Logger, pkgJsonUpdater: PackageJsonUpdater, private lockfile: LockFileSync) { + super(logger, pkgJsonUpdater); + } execute(analyzeEntryPoints: AnalyzeEntryPointsFn, createCompileFn: CreateCompileFn): void { - this.lockFile.lock(() => { - this.logger.debug(`Running ngcc on ${this.constructor.name}.`); - - const taskQueue = analyzeEntryPoints(); - const compile = - createCompileFn((task, outcome) => onTaskCompleted(this.pkgJsonUpdater, task, outcome)); - - // Process all tasks. - this.logger.debug('Processing tasks...'); - const startTime = Date.now(); - - while (!taskQueue.allTasksCompleted) { - const task = taskQueue.getNextTask() !; - compile(task); - taskQueue.markTaskCompleted(task); - } - - const duration = Math.round((Date.now() - startTime) / 1000); - this.logger.debug(`Processed tasks in ${duration}s.`); - }); + this.lockfile.lock(() => this.doExecute(analyzeEntryPoints, createCompileFn)); } } /** * An `Executor` that processes all tasks serially, but still completes asynchronously. */ -export class AsyncSingleProcessExecutor extends SingleProcessExecutor { - async execute(...args: Parameters): Promise { - return super.execute(...args); +export class SingleProcessExecutorAsync extends SingleProcessorExecutorBase implements Executor { + constructor(logger: Logger, pkgJsonUpdater: PackageJsonUpdater, private lockfile: LockFileAsync) { + super(logger, pkgJsonUpdater); + } + async execute(analyzeEntryPoints: AnalyzeEntryPointsFn, createCompileFn: CreateCompileFn): + Promise { + await this.lockfile.lock(async() => this.doExecute(analyzeEntryPoints, createCompileFn)); } } diff --git a/packages/compiler-cli/ngcc/src/main.ts b/packages/compiler-cli/ngcc/src/main.ts index 8f79b4afde..05cdaf9f13 100644 --- a/packages/compiler-cli/ngcc/src/main.ts +++ b/packages/compiler-cli/ngcc/src/main.ts @@ -27,8 +27,8 @@ import {TargetedEntryPointFinder} from './entry_point_finder/targeted_entry_poin import {AnalyzeEntryPointsFn, CreateCompileFn, Executor, PartiallyOrderedTasks, Task, TaskProcessingOutcome, TaskQueue} from './execution/api'; import {ClusterExecutor} from './execution/cluster/executor'; import {ClusterPackageJsonUpdater} from './execution/cluster/package_json_updater'; -import {LockFile} from './execution/lock_file'; -import {AsyncSingleProcessExecutor, SingleProcessExecutor} from './execution/single_process_executor'; +import {LockFileAsync, LockFileSync} from './execution/lock_file'; +import {SingleProcessExecutorAsync, SingleProcessExecutorSync} from './execution/single_process_executor'; import {ParallelTaskQueue} from './execution/task_selection/parallel_task_queue'; import {SerialTaskQueue} from './execution/task_selection/serial_task_queue'; import {ConsoleLogger, LogLevel} from './logging/console_logger'; @@ -290,7 +290,7 @@ export function mainNgcc( }; // The executor for actually planning and getting the work done. - const executor = getExecutor(async, inParallel, logger, pkgJsonUpdater, new LockFile(fileSystem)); + const executor = getExecutor(async, inParallel, logger, pkgJsonUpdater, fileSystem); return executor.execute(analyzeEntryPoints, createCompileFn); } @@ -336,16 +336,21 @@ function getTaskQueue( function getExecutor( async: boolean, inParallel: boolean, logger: Logger, pkgJsonUpdater: PackageJsonUpdater, - lockFile: LockFile): Executor { - if (inParallel) { - // Execute in parallel (which implies async). - // Use up to 8 CPU cores for workers, always reserving one for master. - const workerCount = Math.min(8, os.cpus().length - 1); - return new ClusterExecutor(workerCount, logger, pkgJsonUpdater, lockFile); + fileSystem: FileSystem): Executor { + if (async) { + // Execute asynchronously (either serially or in parallel) + const lockFile = new LockFileAsync(fileSystem, logger, 500, 50); + if (inParallel) { + // Execute in parallel. Use up to 8 CPU cores for workers, always reserving one for master. + const workerCount = Math.min(8, os.cpus().length - 1); + return new ClusterExecutor(workerCount, logger, pkgJsonUpdater, lockFile); + } else { + // Execute serially, on a single thread (async). + return new SingleProcessExecutorAsync(logger, pkgJsonUpdater, lockFile); + } } else { - // Execute serially, on a single thread (either sync or async). - return async ? new AsyncSingleProcessExecutor(logger, pkgJsonUpdater, lockFile) : - new SingleProcessExecutor(logger, pkgJsonUpdater, lockFile); + // Execute serially, on a single thread (sync). + return new SingleProcessExecutorSync(logger, pkgJsonUpdater, new LockFileSync(fileSystem)); } } diff --git a/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts b/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts index 6c6609f313..d42d62479c 100644 --- a/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts +++ b/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts @@ -14,7 +14,7 @@ import {ClusterExecutor} from '../../../src/execution/cluster/executor'; import {ClusterMaster} from '../../../src/execution/cluster/master'; import {ClusterWorker} from '../../../src/execution/cluster/worker'; import {PackageJsonUpdater} from '../../../src/writing/package_json_updater'; -import {MockLockFile} from '../../helpers/mock_lock_file'; +import {MockLockFileAsync} from '../../helpers/mock_lock_file'; import {MockLogger} from '../../helpers/mock_logger'; import {mockProperty} from '../../helpers/spy_utils'; @@ -24,7 +24,7 @@ describe('ClusterExecutor', () => { let masterRunSpy: jasmine.Spy; let workerRunSpy: jasmine.Spy; let mockLogger: MockLogger; - let mockLockFile: MockLockFile; + let mockLockFile: MockLockFileAsync; let executor: ClusterExecutor; beforeEach(() => { @@ -34,7 +34,7 @@ describe('ClusterExecutor', () => { .and.returnValue(Promise.resolve('CusterWorker#run()')); mockLogger = new MockLogger(); - mockLockFile = new MockLockFile(); + mockLockFile = new MockLockFileAsync(); executor = new ClusterExecutor(42, mockLogger, null as unknown as PackageJsonUpdater, mockLockFile); }); @@ -43,9 +43,9 @@ describe('ClusterExecutor', () => { describe('(on cluster master)', () => { beforeEach(() => runAsClusterMaster(true)); - it('should log debug info about the executor', () => { + it('should log debug info about the executor', async() => { const anyFn: () => any = () => undefined; - executor.execute(anyFn, anyFn); + await executor.execute(anyFn, anyFn); expect(mockLogger.logs.debug).toEqual([ ['Running ngcc on ClusterExecutor (using 42 worker processes).'], @@ -88,7 +88,7 @@ describe('ClusterExecutor', () => { it('should not call master runner if Lockfile.create() fails', async() => { const anyFn: () => any = () => undefined; - const lockFile = new MockLockFile({throwOnCreate: true}); + const lockFile = new MockLockFileAsync({throwOnCreate: true}); executor = new ClusterExecutor(42, mockLogger, null as unknown as PackageJsonUpdater, lockFile); let error = ''; @@ -104,7 +104,7 @@ describe('ClusterExecutor', () => { it('should fail if Lockfile.remove() fails', async() => { const anyFn: () => any = () => undefined; - const lockFile = new MockLockFile({throwOnRemove: true}); + const lockFile = new MockLockFileAsync({throwOnRemove: true}); executor = new ClusterExecutor(42, mockLogger, null as unknown as PackageJsonUpdater, lockFile); let error = ''; @@ -122,9 +122,9 @@ describe('ClusterExecutor', () => { describe('(on cluster worker)', () => { beforeEach(() => runAsClusterMaster(false)); - it('should not log debug info about the executor', () => { + it('should not log debug info about the executor', async() => { const anyFn: () => any = () => undefined; - executor.execute(anyFn, anyFn); + await executor.execute(anyFn, anyFn); expect(mockLogger.logs.debug).toEqual([]); }); diff --git a/packages/compiler-cli/ngcc/test/execution/lock_file_spec.ts b/packages/compiler-cli/ngcc/test/execution/lock_file_spec.ts index 78da66fae0..3bfbe29068 100644 --- a/packages/compiler-cli/ngcc/test/execution/lock_file_spec.ts +++ b/packages/compiler-cli/ngcc/test/execution/lock_file_spec.ts @@ -6,44 +6,130 @@ * found in the LICENSE file at https://angular.io/license */ import * as process from 'process'; -import {FileSystem, getFileSystem} from '../../../src/ngtsc/file_system'; -import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; -import {LockFile} from '../../src/execution/lock_file'; -/** - * This class allows us to test the protected methods of LockFile directly, - * which are normally hidden as "protected". - * - * We also add logging in here to track what is being called and in what order. - * - * Finally this class stubs out the `exit()` method to prevent unit tests from exiting the process. - */ -class LockFileUnderTest extends LockFile { - log: string[] = []; - constructor(fs: FileSystem, private handleSignals = false) { - super(fs); - fs.ensureDir(fs.dirname(this.lockFilePath)); - } - create() { - this.log.push('create()'); - super.create(); - } - remove() { - this.log.push('remove()'); - super.remove(); - } - addSignalHandlers() { - if (this.handleSignals) { - super.addSignalHandlers(); - } - } - removeSignalHandlers() { super.removeSignalHandlers(); } - exit(code: number) { this.log.push(`exit(${code})`); } -} +import {CachedFileSystem, FileSystem, getFileSystem} from '../../../src/ngtsc/file_system'; +import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; +import {LockFileAsync, LockFileBase, LockFileSync} from '../../src/execution/lock_file'; +import {MockLogger} from '../helpers/mock_logger'; runInEachFileSystem(() => { - describe('LockFile', () => { - describe('lock() - synchronous', () => { + describe('LockFileBase', () => { + /** + * This class allows us to test the abstract class LockFileBase. + */ + class LockFileUnderTest extends LockFileBase { + log: string[] = []; + constructor(fs: FileSystem, private handleSignals = false) { + super(fs); + fs.ensureDir(fs.dirname(this.lockFilePath)); + } + remove() { super.remove(); } + addSignalHandlers() { + this.log.push('addSignalHandlers()'); + if (this.handleSignals) { + super.addSignalHandlers(); + } + } + writeLockFile() { super.writeLockFile(); } + readLockFile() { return super.readLockFile(); } + removeSignalHandlers() { + this.log.push('removeSignalHandlers()'); + super.removeSignalHandlers(); + } + exit(code: number) { this.log.push(`exit(${code})`); } + } + + describe('writeLockFile()', () => { + it('should call `addSignalHandlers()`', () => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + lockFile.writeLockFile(); + expect(lockFile.log).toEqual(['addSignalHandlers()']); + }); + + it('should call `removeSignalHandlers()` if there is an error', () => { + const fs = getFileSystem(); + spyOn(fs, 'writeFile').and.throwError('WRITING ERROR'); + const lockFile = new LockFileUnderTest(fs); + expect(() => lockFile.writeLockFile()).toThrowError('WRITING ERROR'); + expect(lockFile.log).toEqual(['addSignalHandlers()', 'removeSignalHandlers()']); + }); + }); + + describe('readLockFile()', () => { + it('should return the contents of the lockfile', () => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + fs.writeFile(lockFile.lockFilePath, '188'); + expect(lockFile.readLockFile()).toEqual('188'); + }); + + it('should return `{unknown}` if the lockfile does not exist', () => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + expect(lockFile.readLockFile()).toEqual('{unknown}'); + }); + }); + + describe('remove()', () => { + it('should remove the lock file from the file-system', () => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + fs.writeFile(lockFile.lockFilePath, '188'); + lockFile.remove(); + expect(fs.exists(lockFile.lockFilePath)).toBe(false); + }); + + it('should not error if the lock file does not exist', () => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + expect(() => lockFile.remove()).not.toThrow(); + }); + + it('should call removeSignalHandlers()', () => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + fs.writeFile(lockFile.lockFilePath, '188'); + lockFile.remove(); + expect(lockFile.log).toEqual(['removeSignalHandlers()']); + }); + }); + }); + + describe('LockFileSync', () => { + /** + * This class allows us to test the protected methods of LockFileSync directly, + * which are normally hidden as "protected". + * + * We also add logging in here to track what is being called and in what order. + * + * Finally this class stubs out the `exit()` method to prevent unit tests from exiting the + * process. + */ + class LockFileUnderTest extends LockFileSync { + log: string[] = []; + constructor(fs: FileSystem, private handleSignals = false) { + super(fs); + fs.ensureDir(fs.dirname(this.lockFilePath)); + } + create() { + this.log.push('create()'); + super.create(); + } + remove() { + this.log.push('remove()'); + super.remove(); + } + addSignalHandlers() { + if (this.handleSignals) { + super.addSignalHandlers(); + } + } + removeSignalHandlers() { super.removeSignalHandlers(); } + exit(code: number) { this.log.push(`exit(${code})`); } + } + + describe('lock()', () => { it('should guard the `fn()` with calls to `create()` and `remove()`', () => { const fs = getFileSystem(); const lockFile = new LockFileUnderTest(fs); @@ -101,72 +187,6 @@ runInEachFileSystem(() => { }); }); - describe('lock() - asynchronous', () => { - it('should guard the `fn()` with calls to `create()` and `remove()`', async() => { - const fs = getFileSystem(); - const lockFile = new LockFileUnderTest(fs); - - await lockFile.lock(async() => { - lockFile.log.push('fn() - before'); - // This promise forces node to do a tick in this function, ensuring that we are truly - // testing an async scenario. - await Promise.resolve(); - lockFile.log.push('fn() - after'); - }); - expect(lockFile.log).toEqual(['create()', 'fn() - before', 'fn() - after', 'remove()']); - }); - - it('should guard the `fn()` with calls to `create()` and `remove()`, even if it throws', - async() => { - let error: string = ''; - const fs = getFileSystem(); - const lockFile = new LockFileUnderTest(fs); - lockFile.create = () => lockFile.log.push('create()'); - lockFile.remove = () => lockFile.log.push('remove()'); - - try { - await lockFile.lock(async() => { - lockFile.log.push('fn()'); - throw new Error('ERROR'); - }); - } catch (e) { - error = e.message; - } - expect(error).toEqual('ERROR'); - expect(lockFile.log).toEqual(['create()', 'fn()', 'remove()']); - }); - - it('should remove the lockfile if CTRL-C is triggered', async() => { - const fs = getFileSystem(); - const lockFile = new LockFileUnderTest(fs, /* handleSignals */ true); - - await lockFile.lock(async() => { - lockFile.log.push('SIGINT'); - process.emit('SIGINT', 'SIGINT'); - }); - // Since the test does not actually exit process, the `remove()` is called one more time. - expect(lockFile.log).toEqual(['create()', 'SIGINT', 'remove()', 'exit(1)', 'remove()']); - // Clean up the signal handlers. In practice this is not needed since the process would have - // been terminated already. - lockFile.removeSignalHandlers(); - }); - - it('should remove the lockfile if terminal is closed', async() => { - const fs = getFileSystem(); - const lockFile = new LockFileUnderTest(fs, /* handleSignals */ true); - - await lockFile.lock(async() => { - lockFile.log.push('SIGHUP'); - process.emit('SIGHUP', 'SIGHUP'); - }); - // Since this does not actually exit process, the `remove()` is called one more time. - expect(lockFile.log).toEqual(['create()', 'SIGHUP', 'remove()', 'exit(1)', 'remove()']); - // Clean up the signal handlers. In practice this is not needed since the process would have - // been terminated already. - lockFile.removeSignalHandlers(); - }); - }); - describe('create()', () => { it('should write a lock file to the file-system', () => { const fs = getFileSystem(); @@ -188,21 +208,180 @@ runInEachFileSystem(() => { `(If you are sure no ngcc process is running then you should delete the lockfile at ${lockFile.lockFilePath}.)`); }); }); + }); - describe('remove()', () => { - it('should remove the lock file from the file-system', () => { + describe('LockFileAsync', () => { + /** + * This class allows us to test the protected methods of LockFileAsync directly, + * which are normally hidden as "protected". + * + * We also add logging in here to track what is being called and in what order. + * + * Finally this class stubs out the `exit()` method to prevent unit tests from exiting the + * process. + */ + class LockFileUnderTest extends LockFileAsync { + log: string[] = []; + constructor( + fs: FileSystem, retryDelay = 100, retryAttempts = 10, private handleSignals = false) { + super(fs, new MockLogger(), retryDelay, retryAttempts); + fs.ensureDir(fs.dirname(this.lockFilePath)); + } + async create() { + this.log.push('create()'); + await super.create(); + } + remove() { + this.log.push('remove()'); + super.remove(); + } + addSignalHandlers() { + if (this.handleSignals) { + super.addSignalHandlers(); + } + } + removeSignalHandlers() { super.removeSignalHandlers(); } + exit(code: number) { this.log.push(`exit(${code})`); } + getLogger() { return this.logger as MockLogger; } + } + + describe('lock()', () => { + it('should guard the `fn()` with calls to `create()` and `remove()`', async() => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + + await lockFile.lock(async() => { + lockFile.log.push('fn() - before'); + // This promise forces node to do a tick in this function, ensuring that we are truly + // testing an async scenario. + await Promise.resolve(); + lockFile.log.push('fn() - after'); + return Promise.resolve(); + }); + expect(lockFile.log).toEqual(['create()', 'fn() - before', 'fn() - after', 'remove()']); + }); + + it('should guard the `fn()` with calls to `create()` and `remove()`, even if it throws', + async() => { + let error: string = ''; + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + try { + await lockFile.lock(async() => { + lockFile.log.push('fn()'); + throw new Error('ERROR'); + }); + } catch (e) { + error = e.message; + } + expect(error).toEqual('ERROR'); + expect(lockFile.log).toEqual(['create()', 'fn()', 'remove()']); + }); + + it('should remove the lockfile if CTRL-C is triggered', async() => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs, 100, 3, /* handleSignals */ true); + + await lockFile.lock(async() => { + lockFile.log.push('SIGINT'); + process.emit('SIGINT', 'SIGINT'); + }); + // Since the test does not actually exit process, the `remove()` is called one more time. + expect(lockFile.log).toEqual(['create()', 'SIGINT', 'remove()', 'exit(1)', 'remove()']); + // Clean up the signal handlers. In practice this is not needed since the process would have + // been terminated already. + lockFile.removeSignalHandlers(); + }); + + it('should remove the lockfile if terminal is closed', async() => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs, 100, 3, /* handleSignals */ true); + + await lockFile.lock(async() => { + lockFile.log.push('SIGHUP'); + process.emit('SIGHUP', 'SIGHUP'); + }); + // Since this does not actually exit process, the `remove()` is called one more time. + expect(lockFile.log).toEqual(['create()', 'SIGHUP', 'remove()', 'exit(1)', 'remove()']); + // Clean up the signal handlers. In practice this is not needed since the process would have + // been terminated already. + lockFile.removeSignalHandlers(); + }); + }); + + describe('create()', () => { + it('should write a lock file to the file-system', async() => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs); + expect(fs.exists(lockFile.lockFilePath)).toBe(false); + await lockFile.create(); + expect(fs.exists(lockFile.lockFilePath)).toBe(true); + }); + + it('should retry if another process is locking', async() => { const fs = getFileSystem(); const lockFile = new LockFileUnderTest(fs); fs.writeFile(lockFile.lockFilePath, '188'); - lockFile.remove(); - expect(fs.exists(lockFile.lockFilePath)).toBe(false); + const promise = lockFile.lock(async() => lockFile.log.push('fn()')); + // The lock is now waiting on the lockfile becoming free, so no `fn()` in the log. + expect(lockFile.log).toEqual(['create()']); + expect(lockFile.getLogger().logs.info).toEqual([[ + 'Another process, with id 188, is currently running ngcc.\nWaiting up to 1s for it to finish.' + ]]); + fs.removeFile(lockFile.lockFilePath); + // The lockfile has been removed, so we can create our own lockfile, call `fn()` and then + // remove the lockfile. + await promise; + expect(lockFile.log).toEqual(['create()', 'fn()', 'remove()']); }); - it('should not error if the lock file does not exist', () => { - const fs = getFileSystem(); + it('should extend the retry timeout if the other process locking the file changes', async() => { + // Use a cached file system to test that we are invalidating it correctly + const rawFs = getFileSystem(); + const fs = new CachedFileSystem(rawFs); const lockFile = new LockFileUnderTest(fs); - expect(() => lockFile.remove()).not.toThrow(); + fs.writeFile(lockFile.lockFilePath, '188'); + const promise = lockFile.lock(async() => lockFile.log.push('fn()')); + // The lock is now waiting on the lockfile becoming free, so no `fn()` in the log. + expect(lockFile.log).toEqual(['create()']); + expect(lockFile.getLogger().logs.info).toEqual([[ + 'Another process, with id 188, is currently running ngcc.\nWaiting up to 1s for it to finish.' + ]]); + // We need to write to the rawFs to ensure that we don't update the cache at this point + rawFs.writeFile(lockFile.lockFilePath, '444'); + await new Promise(resolve => setTimeout(resolve, 250)); + expect(lockFile.getLogger().logs.info).toEqual([ + [ + 'Another process, with id 188, is currently running ngcc.\nWaiting up to 1s for it to finish.' + ], + [ + 'Another process, with id 444, is currently running ngcc.\nWaiting up to 1s for it to finish.' + ] + ]); + fs.removeFile(lockFile.lockFilePath); + // The lockfile has been removed, so we can create our own lockfile, call `fn()` and then + // remove the lockfile. + await promise; + expect(lockFile.log).toEqual(['create()', 'fn()', 'remove()']); }); + + it('should error if another process does not release the lockfile before this times out', + async() => { + const fs = getFileSystem(); + const lockFile = new LockFileUnderTest(fs, 100, 2); + fs.writeFile(lockFile.lockFilePath, '188'); + const promise = lockFile.lock(async() => lockFile.log.push('fn()')); + // The lock is now waiting on the lockfile becoming free, so no `fn()` in the log. + expect(lockFile.log).toEqual(['create()']); + // Do not remove the lockfile and let the call to `lock()` timeout. + let error: Error; + await promise.catch(e => error = e); + expect(lockFile.log).toEqual(['create()']); + expect(error !.message) + .toEqual( + `Timed out waiting 0.2s for another ngcc process, with id 188, to complete.\n` + + `(If you are sure no ngcc process is running then you should delete the lockfile at ${lockFile.lockFilePath}.)`); + }); }); }); }); diff --git a/packages/compiler-cli/ngcc/test/execution/single_processor_executor_spec.ts b/packages/compiler-cli/ngcc/test/execution/single_processor_executor_spec.ts index e55b26b531..6ae1473124 100644 --- a/packages/compiler-cli/ngcc/test/execution/single_processor_executor_spec.ts +++ b/packages/compiler-cli/ngcc/test/execution/single_processor_executor_spec.ts @@ -8,23 +8,23 @@ /// -import {SingleProcessExecutor} from '../../src/execution/single_process_executor'; +import {SingleProcessExecutorSync} from '../../src/execution/single_process_executor'; import {SerialTaskQueue} from '../../src/execution/task_selection/serial_task_queue'; import {PackageJsonUpdater} from '../../src/writing/package_json_updater'; -import {MockLockFile} from '../helpers/mock_lock_file'; +import {MockLockFileSync} from '../helpers/mock_lock_file'; import {MockLogger} from '../helpers/mock_logger'; describe('SingleProcessExecutor', () => { let mockLogger: MockLogger; - let mockLockFile: MockLockFile; - let executor: SingleProcessExecutor; + let mockLockFile: MockLockFileSync; + let executor: SingleProcessExecutorSync; beforeEach(() => { mockLogger = new MockLogger(); - mockLockFile = new MockLockFile(); - executor = - new SingleProcessExecutor(mockLogger, null as unknown as PackageJsonUpdater, mockLockFile); + mockLockFile = new MockLockFileSync(); + executor = new SingleProcessExecutorSync( + mockLogger, null as unknown as PackageJsonUpdater, mockLockFile); }); describe('execute()', () => { @@ -63,11 +63,11 @@ describe('SingleProcessExecutor', () => { }); it('should not call `analyzeEntryPoints` if Lockfile.create() fails', () => { - const lockFile = new MockLockFile({throwOnCreate: true}); + const lockFile = new MockLockFileSync({throwOnCreate: true}); const analyzeFn: () => any = () => { lockFile.log.push('analyzeFn'); }; const anyFn: () => any = () => undefined; - executor = - new SingleProcessExecutor(mockLogger, null as unknown as PackageJsonUpdater, lockFile); + executor = new SingleProcessExecutorSync( + mockLogger, null as unknown as PackageJsonUpdater, lockFile); let error = ''; try { executor.execute(analyzeFn, anyFn); @@ -81,9 +81,9 @@ describe('SingleProcessExecutor', () => { it('should fail if Lockfile.remove() fails', () => { const noTasks = () => new SerialTaskQueue([] as any); const anyFn: () => any = () => undefined; - const lockFile = new MockLockFile({throwOnRemove: true}); - executor = - new SingleProcessExecutor(mockLogger, null as unknown as PackageJsonUpdater, lockFile); + const lockFile = new MockLockFileSync({throwOnRemove: true}); + executor = new SingleProcessExecutorSync( + mockLogger, null as unknown as PackageJsonUpdater, lockFile); let error = ''; try { executor.execute(noTasks, anyFn); diff --git a/packages/compiler-cli/ngcc/test/helpers/mock_lock_file.ts b/packages/compiler-cli/ngcc/test/helpers/mock_lock_file.ts index 77b2040fb5..873bebf2b7 100644 --- a/packages/compiler-cli/ngcc/test/helpers/mock_lock_file.ts +++ b/packages/compiler-cli/ngcc/test/helpers/mock_lock_file.ts @@ -6,9 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ import {MockFileSystemNative} from '../../../src/ngtsc/file_system/testing'; -import {LockFile} from '../../src/execution/lock_file'; +import {LockFileAsync, LockFileSync} from '../../src/execution/lock_file'; +import {MockLogger} from './mock_logger'; -export class MockLockFile extends LockFile { +export class MockLockFileSync extends LockFileSync { log: string[] = []; constructor(private options: {throwOnCreate?: boolean, throwOnRemove?: boolean} = {}) { // This `MockLockFile` is not used in tests that are run via `runInEachFileSystem()` @@ -24,3 +25,20 @@ export class MockLockFile extends LockFile { if (this.options.throwOnRemove) throw new Error('LockFile.remove() error'); } } + +export class MockLockFileAsync extends LockFileAsync { + log: string[] = []; + constructor(private options: {throwOnCreate?: boolean, throwOnRemove?: boolean} = {}) { + // This `MockLockFile` is not used in tests that are run via `runInEachFileSystem()` + // So we cannot use `getFileSystem()` but instead just instantiate a mock file-system. + super(new MockFileSystemNative(), new MockLogger(), 200, 2); + } + async create() { + this.log.push('create()'); + if (this.options.throwOnCreate) throw new Error('LockFile.create() error'); + } + remove() { + this.log.push('remove()'); + if (this.options.throwOnRemove) throw new Error('LockFile.remove() error'); + } +} diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 56bd9064b5..1637f491e3 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -13,7 +13,7 @@ import * as os from 'os'; import {AbsoluteFsPath, FileSystem, absoluteFrom, getFileSystem, join} from '../../../src/ngtsc/file_system'; import {Folder, MockFileSystem, TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; import {loadStandardTestFiles, loadTestFiles} from '../../../test/helpers'; -import {LockFile} from '../../src/execution/lock_file'; +import {LockFileSync} from '../../src/execution/lock_file'; import {mainNgcc} from '../../src/main'; import {markAsProcessed} from '../../src/packages/build_marker'; import {EntryPointJsonProperty, EntryPointPackageJson, SUPPORTED_FORMAT_PROPERTIES} from '../../src/packages/entry_point'; @@ -1507,7 +1507,7 @@ runInEachFileSystem(() => { function initMockFileSystem(fs: FileSystem, testFiles: Folder) { if (fs instanceof MockFileSystem) { fs.init(testFiles); - fs.ensureDir(fs.dirname(new LockFile(fs).lockFilePath)); + fs.ensureDir(fs.dirname(new LockFileSync(fs).lockFilePath)); } // a random test package that no metadata.json file so not compiled by Angular. diff --git a/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts b/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts index 1f81b7c1e9..cea871856e 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts +++ b/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts @@ -91,7 +91,7 @@ describe('CachedFileSystem', () => { }); describe('invalidateCaches()', () => { - it('should use the delegate for `readFile()` if the path for the cached file has been invalidated', + it('should call the delegate `readFile()` if the path for the cached file has been invalidated', () => { spyOn(delegate, 'lstat').and.returnValue({isSymbolicLink: () => false}); const spy = spyOn(delegate, 'readFile').and.returnValue('Some contents'); @@ -107,7 +107,7 @@ describe('CachedFileSystem', () => { expect(spy).toHaveBeenCalledWith(abcPath); }); - it('should use the delegate `exists()` if the path for the cached file has been invalidated', + it('should call the delegate `exists()` if the path for the cached file has been invalidated', () => { const spy = spyOn(delegate, 'exists').and.returnValue(true); fs.exists(abcPath); // Call once to fill the cache