diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts b/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts index c601b889a6..8479c82625 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 {LockFileAsync} from '../lock_file'; +import {LockFile} 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: LockFileAsync) {} + private pkgJsonUpdater: PackageJsonUpdater, private lockFile: LockFile) {} 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 38883beeec..30efb22f46 100644 --- a/packages/compiler-cli/ngcc/src/execution/lock_file.ts +++ b/packages/compiler-cli/ngcc/src/execution/lock_file.ts @@ -6,44 +6,78 @@ * found in the LICENSE file at https://angular.io/license */ import * as process from 'process'; +import {FileSystem} from '../../../src/ngtsc/file_system'; -import {CachedFileSystem, FileSystem} from '../../../src/ngtsc/file_system'; -import {Logger} from '../logging/logger'; - -export abstract class LockFileBase { +/** + * 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 { lockFilePath = this.fs.resolve(require.resolve('@angular/compiler-cli/ngcc'), '../__ngcc_lock_file__'); - constructor(protected fs: FileSystem) {} + constructor(private fs: FileSystem) {} - protected writeLockFile(): void { + /** + * 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(); try { - 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; + 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(); + } } } /** - * 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. + * Write a lock file to disk, or error if there is already one there. */ - protected readLockFile(): string { + protected create() { try { - 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); + 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; } - return this.fs.readFile(this.lockFilePath); - } catch { - return '{unknown}'; + + // 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}.)`); } } @@ -57,25 +91,18 @@ export abstract class LockFileBase { } } - /** - * Capture CTRL-C and terminal closing events. - * When these occur we remove the lockfile and exit. - */ protected addSignalHandlers() { - process.addListener('SIGINT', this.signalHandler); - process.addListener('SIGHUP', this.signalHandler); + process.once('SIGINT', this.signalHandler); + process.once('SIGHUP', this.signalHandler); } - /** - * Clear the event handlers to prevent leakage. - */ protected removeSignalHandlers() { process.removeListener('SIGINT', this.signalHandler); process.removeListener('SIGHUP', this.signalHandler); } /** - * This handler needs to be defined as a property rather than a method + * This handle needs to be defined as a property rather than a method * so that it can be passed around as a bound function. */ protected signalHandler = @@ -92,113 +119,3 @@ export abstract class LockFileBase { 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 29775ae068..d06dceab3c 100644 --- a/packages/compiler-cli/ngcc/src/execution/single_process_executor.ts +++ b/packages/compiler-cli/ngcc/src/execution/single_process_executor.ts @@ -10,56 +10,47 @@ import {Logger} from '../logging/logger'; import {PackageJsonUpdater} from '../writing/package_json_updater'; import {AnalyzeEntryPointsFn, CreateCompileFn, Executor} from './api'; -import {LockFileAsync, LockFileSync} from './lock_file'; +import {LockFile} 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 SingleProcessExecutorSync extends SingleProcessorExecutorBase implements Executor { - constructor(logger: Logger, pkgJsonUpdater: PackageJsonUpdater, private lockfile: LockFileSync) { - super(logger, pkgJsonUpdater); - } +export class SingleProcessExecutor implements Executor { + constructor( + private logger: Logger, private pkgJsonUpdater: PackageJsonUpdater, + private lockFile: LockFile) {} + execute(analyzeEntryPoints: AnalyzeEntryPointsFn, createCompileFn: CreateCompileFn): void { - this.lockfile.lock(() => this.doExecute(analyzeEntryPoints, createCompileFn)); + 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.`); + }); } } /** * An `Executor` that processes all tasks serially, but still completes asynchronously. */ -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)); +export class AsyncSingleProcessExecutor extends SingleProcessExecutor { + async execute(...args: Parameters): Promise { + return super.execute(...args); } } diff --git a/packages/compiler-cli/ngcc/src/main.ts b/packages/compiler-cli/ngcc/src/main.ts index 05cdaf9f13..8f79b4afde 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 {LockFileAsync, LockFileSync} from './execution/lock_file'; -import {SingleProcessExecutorAsync, SingleProcessExecutorSync} from './execution/single_process_executor'; +import {LockFile} from './execution/lock_file'; +import {AsyncSingleProcessExecutor, SingleProcessExecutor} 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, fileSystem); + const executor = getExecutor(async, inParallel, logger, pkgJsonUpdater, new LockFile(fileSystem)); return executor.execute(analyzeEntryPoints, createCompileFn); } @@ -336,21 +336,16 @@ function getTaskQueue( function getExecutor( async: boolean, inParallel: boolean, logger: Logger, pkgJsonUpdater: PackageJsonUpdater, - 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); - } + 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); } else { - // Execute serially, on a single thread (sync). - return new SingleProcessExecutorSync(logger, pkgJsonUpdater, new LockFileSync(fileSystem)); + // Execute serially, on a single thread (either sync or async). + return async ? new AsyncSingleProcessExecutor(logger, pkgJsonUpdater, lockFile) : + new SingleProcessExecutor(logger, pkgJsonUpdater, lockFile); } } 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 d42d62479c..6c6609f313 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 {MockLockFileAsync} from '../../helpers/mock_lock_file'; +import {MockLockFile} 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: MockLockFileAsync; + let mockLockFile: MockLockFile; let executor: ClusterExecutor; beforeEach(() => { @@ -34,7 +34,7 @@ describe('ClusterExecutor', () => { .and.returnValue(Promise.resolve('CusterWorker#run()')); mockLogger = new MockLogger(); - mockLockFile = new MockLockFileAsync(); + mockLockFile = new MockLockFile(); 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', async() => { + it('should log debug info about the executor', () => { const anyFn: () => any = () => undefined; - await executor.execute(anyFn, anyFn); + 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 MockLockFileAsync({throwOnCreate: true}); + const lockFile = new MockLockFile({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 MockLockFileAsync({throwOnRemove: true}); + const lockFile = new MockLockFile({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', async() => { + it('should not log debug info about the executor', () => { const anyFn: () => any = () => undefined; - await executor.execute(anyFn, anyFn); + 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 3bfbe29068..78da66fae0 100644 --- a/packages/compiler-cli/ngcc/test/execution/lock_file_spec.ts +++ b/packages/compiler-cli/ngcc/test/execution/lock_file_spec.ts @@ -6,130 +6,44 @@ * found in the LICENSE file at https://angular.io/license */ import * as process from 'process'; - -import {CachedFileSystem, FileSystem, getFileSystem} from '../../../src/ngtsc/file_system'; +import {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'; +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})`); } +} runInEachFileSystem(() => { - 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()', () => { + describe('LockFile', () => { + describe('lock() - synchronous', () => { it('should guard the `fn()` with calls to `create()` and `remove()`', () => { const fs = getFileSystem(); const lockFile = new LockFileUnderTest(fs); @@ -187,6 +101,72 @@ 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(); @@ -208,180 +188,21 @@ runInEachFileSystem(() => { `(If you are sure no ngcc process is running then you should delete the lockfile at ${lockFile.lockFilePath}.)`); }); }); - }); - 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() => { + 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); - await lockFile.create(); - expect(fs.exists(lockFile.lockFilePath)).toBe(true); }); - it('should retry if another process is locking', async() => { + it('should not error if the lock file does not exist', () => { const fs = getFileSystem(); const lockFile = new LockFileUnderTest(fs); - 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.' - ]]); - 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()']); + expect(() => lockFile.remove()).not.toThrow(); }); - - 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); - 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 6ae1473124..e55b26b531 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 {SingleProcessExecutorSync} from '../../src/execution/single_process_executor'; +import {SingleProcessExecutor} 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 {MockLockFileSync} from '../helpers/mock_lock_file'; +import {MockLockFile} from '../helpers/mock_lock_file'; import {MockLogger} from '../helpers/mock_logger'; describe('SingleProcessExecutor', () => { let mockLogger: MockLogger; - let mockLockFile: MockLockFileSync; - let executor: SingleProcessExecutorSync; + let mockLockFile: MockLockFile; + let executor: SingleProcessExecutor; beforeEach(() => { mockLogger = new MockLogger(); - mockLockFile = new MockLockFileSync(); - executor = new SingleProcessExecutorSync( - mockLogger, null as unknown as PackageJsonUpdater, mockLockFile); + mockLockFile = new MockLockFile(); + executor = + new SingleProcessExecutor(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 MockLockFileSync({throwOnCreate: true}); + const lockFile = new MockLockFile({throwOnCreate: true}); const analyzeFn: () => any = () => { lockFile.log.push('analyzeFn'); }; const anyFn: () => any = () => undefined; - executor = new SingleProcessExecutorSync( - mockLogger, null as unknown as PackageJsonUpdater, lockFile); + executor = + new SingleProcessExecutor(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 MockLockFileSync({throwOnRemove: true}); - executor = new SingleProcessExecutorSync( - mockLogger, null as unknown as PackageJsonUpdater, lockFile); + const lockFile = new MockLockFile({throwOnRemove: true}); + executor = + new SingleProcessExecutor(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 873bebf2b7..77b2040fb5 100644 --- a/packages/compiler-cli/ngcc/test/helpers/mock_lock_file.ts +++ b/packages/compiler-cli/ngcc/test/helpers/mock_lock_file.ts @@ -6,10 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ import {MockFileSystemNative} from '../../../src/ngtsc/file_system/testing'; -import {LockFileAsync, LockFileSync} from '../../src/execution/lock_file'; -import {MockLogger} from './mock_logger'; +import {LockFile} from '../../src/execution/lock_file'; -export class MockLockFileSync extends LockFileSync { +export class MockLockFile extends LockFile { log: string[] = []; constructor(private options: {throwOnCreate?: boolean, throwOnRemove?: boolean} = {}) { // This `MockLockFile` is not used in tests that are run via `runInEachFileSystem()` @@ -25,20 +24,3 @@ export class MockLockFileSync extends LockFileSync { 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 1637f491e3..56bd9064b5 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 {LockFileSync} from '../../src/execution/lock_file'; +import {LockFile} 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 LockFileSync(fs).lockFilePath)); + fs.ensureDir(fs.dirname(new LockFile(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 cea871856e..1f81b7c1e9 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 call the delegate `readFile()` if the path for the cached file has been invalidated', + it('should use the delegate for `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 call the delegate `exists()` if the path for the cached file has been invalidated', + it('should use 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