From 66effde9f3f9796f9507cd80a85692f0e04e96f5 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Sun, 12 Apr 2020 14:55:55 +0300 Subject: [PATCH] fix(ngcc): do not spawn unlocker processes on cluster workers (#36569) The current ngcc lock-file strategy spawns a new process in order to capture a potential `SIGINT` and remove the lock-file. For more information see #35861. Previously, this unlocker process was spawned as soon as the `LockFile` was instantiated in order to have it available as soon as possible (given that spawning a process is an asynchronous operation). Since the `LockFile` was instantiated and passed to the `Executor`, this meant that an unlocker process was spawned for each cluster worker, when running ngcc in parallel mode. These processes were not needed, since the `LockFile` was not used in cluster workers, but we still had to pay the overhead of each process' own memory and V8 instance. (NOTE: This overhead was small compared to the memory consumed by ngcc's normal operations, but still unnecessary.) This commit avoids the extra processes by only spawning an unlocker process when running on the cluster master process and not on worker processes. PR Close #36569 --- .../cluster/lock_file_with_child_process.ts | 42 +++++++++ .../lock_file_with_child_process/index.ts | 2 +- packages/compiler-cli/ngcc/src/main.ts | 4 +- .../lock_file_with_child_process_spec.ts | 87 +++++++++++++++++++ 4 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 packages/compiler-cli/ngcc/src/execution/cluster/lock_file_with_child_process.ts create mode 100644 packages/compiler-cli/ngcc/test/execution/cluster/lock_file_with_child_process_spec.ts diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/lock_file_with_child_process.ts b/packages/compiler-cli/ngcc/src/execution/cluster/lock_file_with_child_process.ts new file mode 100644 index 0000000000..b6354976a6 --- /dev/null +++ b/packages/compiler-cli/ngcc/src/execution/cluster/lock_file_with_child_process.ts @@ -0,0 +1,42 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/// + +import {ChildProcess} from 'child_process'; +import * as cluster from 'cluster'; + +import {AbsoluteFsPath} from '../../../../src/ngtsc/file_system'; +import {LockFileWithChildProcess} from '../../locking/lock_file_with_child_process'; + + +/** + * A `LockFileWithChildProcess` that is `cluster`-aware and does not spawn unlocker processes from + * worker processes (only from the master process, which does the locking). + */ +export class ClusterLockFileWithChildProcess extends LockFileWithChildProcess { + write(): void { + if (!cluster.isMaster) { + // This is a worker process: + // This method should only be on the master process. + throw new Error('Tried to create a lock-file from a worker process.'); + } + + return super.write(); + } + + protected createUnlocker(path: AbsoluteFsPath): ChildProcess|null { + if (cluster.isMaster) { + // This is the master process: + // Create the unlocker. + return super.createUnlocker(path); + } + + return null; + } +} diff --git a/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts index cbabff7719..9827343db9 100644 --- a/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts +++ b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts @@ -77,7 +77,7 @@ export class LockFileWithChildProcess implements LockFile { } } - protected createUnlocker(path: AbsoluteFsPath): ChildProcess { + protected createUnlocker(path: AbsoluteFsPath): ChildProcess|null { this.logger.debug('Forking unlocker child-process'); const logLevel = this.logger.level !== undefined ? this.logger.level.toString() : LogLevel.info.toString(); diff --git a/packages/compiler-cli/ngcc/src/main.ts b/packages/compiler-cli/ngcc/src/main.ts index 1f63a029bb..0d072866a1 100644 --- a/packages/compiler-cli/ngcc/src/main.ts +++ b/packages/compiler-cli/ngcc/src/main.ts @@ -27,6 +27,7 @@ import {EntryPointFinder} from './entry_point_finder/interface'; import {TargetedEntryPointFinder} from './entry_point_finder/targeted_entry_point_finder'; import {AnalyzeEntryPointsFn, CreateCompileFn, Executor} from './execution/api'; import {ClusterExecutor} from './execution/cluster/executor'; +import {ClusterLockFileWithChildProcess} from './execution/cluster/lock_file_with_child_process'; import {ClusterPackageJsonUpdater} from './execution/cluster/package_json_updater'; import {SingleProcessExecutorAsync, SingleProcessExecutorSync} from './execution/single_process_executor'; import {CreateTaskCompletedCallback, PartiallyOrderedTasks, Task, TaskProcessingOutcome, TaskQueue} from './execution/tasks/api'; @@ -428,7 +429,8 @@ function getCreateTaskCompletedCallback( function getExecutor( async: boolean, inParallel: boolean, logger: Logger, pkgJsonUpdater: PackageJsonUpdater, fileSystem: FileSystem, createTaskCompletedCallback: CreateTaskCompletedCallback): Executor { - const lockFile = new LockFileWithChildProcess(fileSystem, logger); + const lockFile = inParallel ? new ClusterLockFileWithChildProcess(fileSystem, logger) : + new LockFileWithChildProcess(fileSystem, logger); if (async) { // Execute asynchronously (either serially or in parallel) const locker = new AsyncLocker(lockFile, logger, 500, 50); diff --git a/packages/compiler-cli/ngcc/test/execution/cluster/lock_file_with_child_process_spec.ts b/packages/compiler-cli/ngcc/test/execution/cluster/lock_file_with_child_process_spec.ts new file mode 100644 index 0000000000..17501508d5 --- /dev/null +++ b/packages/compiler-cli/ngcc/test/execution/cluster/lock_file_with_child_process_spec.ts @@ -0,0 +1,87 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/// + +import {ChildProcess} from 'child_process'; +import * as cluster from 'cluster'; + +import {getFileSystem} from '../../../../src/ngtsc/file_system'; +import {runInEachFileSystem} from '../../../../src/ngtsc/file_system/testing'; +import {ClusterLockFileWithChildProcess} from '../../../src/execution/cluster/lock_file_with_child_process'; +import {LockFileWithChildProcess} from '../../../src/locking/lock_file_with_child_process'; +import {MockLogger} from '../../helpers/mock_logger'; +import {mockProperty} from '../../helpers/spy_utils'; + + +runInEachFileSystem(() => { + describe('ClusterLockFileWithChildProcess', () => { + const runAsClusterMaster = mockProperty(cluster, 'isMaster'); + const mockUnlockerProcess = {} as ChildProcess; + let lockFileWithChildProcessSpies: + Record<'createUnlocker'|'read'|'remove'|'write', jasmine.Spy>; + + beforeEach(() => { + lockFileWithChildProcessSpies = { + createUnlocker: spyOn(LockFileWithChildProcess.prototype as any, 'createUnlocker') + .and.returnValue(mockUnlockerProcess), + read: spyOn(LockFileWithChildProcess.prototype, 'read').and.returnValue('{unknown}'), + remove: spyOn(LockFileWithChildProcess.prototype, 'remove'), + write: spyOn(LockFileWithChildProcess.prototype, 'write'), + }; + }); + + it('should be an instance of `LockFileWithChildProcess`', () => { + const lockFile = new ClusterLockFileWithChildProcess(getFileSystem(), new MockLogger()); + + expect(lockFile).toEqual(jasmine.any(ClusterLockFileWithChildProcess)); + expect(lockFile).toEqual(jasmine.any(LockFileWithChildProcess)); + }); + + describe('write()', () => { + it('should create the lock-file when called on the cluster master', () => { + runAsClusterMaster(true); + const lockFile = new ClusterLockFileWithChildProcess(getFileSystem(), new MockLogger()); + + expect(lockFileWithChildProcessSpies.write).not.toHaveBeenCalled(); + + lockFile.write(); + expect(lockFileWithChildProcessSpies.write).toHaveBeenCalledWith(); + }); + + it('should throw an error when called on a cluster worker', () => { + runAsClusterMaster(false); + const lockFile = new ClusterLockFileWithChildProcess(getFileSystem(), new MockLogger()); + + expect(() => lockFile.write()) + .toThrowError('Tried to create a lock-file from a worker process.'); + expect(lockFileWithChildProcessSpies.write).not.toHaveBeenCalled(); + }); + }); + + describe('createUnlocker()', () => { + it('should create the unlocker when called on the cluster master', () => { + runAsClusterMaster(true); + const lockFile = new ClusterLockFileWithChildProcess(getFileSystem(), new MockLogger()); + + lockFileWithChildProcessSpies.createUnlocker.calls.reset(); + + expect((lockFile as any).createUnlocker(lockFile.path)).toBe(mockUnlockerProcess); + expect(lockFileWithChildProcessSpies.createUnlocker).toHaveBeenCalledWith(lockFile.path); + }); + + it('should not create the unlocker when called on a cluster worker', () => { + runAsClusterMaster(false); + const lockFile = new ClusterLockFileWithChildProcess(getFileSystem(), new MockLogger()); + + expect((lockFile as any).createUnlocker(lockFile.path)).toBeNull(); + expect(lockFileWithChildProcessSpies.createUnlocker).not.toHaveBeenCalled(); + }); + }); + }); +});