From 1a1f99af371ffb15fed522570a05f5171cdd90e5 Mon Sep 17 00:00:00 2001
From: Pete Bacon Darwin <pete@bacondarwin.com>
Date: Thu, 25 Jun 2020 16:43:05 +0100
Subject: [PATCH] fix(ngcc): ensure lockfile is removed when analyzeFn fails
 (#37739)

Previously an error thrown in the `analyzeFn` would cause
the ngcc process to exit immediately without removing the
lockfile, and potentially before the unlocker process had been
successfully spawned resulting in the lockfile being orphaned
and left behind.

Now we catch these errors and remove the lockfile as needed.

PR Close #37739
---
 .../ngcc/src/execution/cluster/executor.ts    |  4 ++--
 .../ngcc/src/locking/async_locker.ts          |  6 +++++-
 .../test/execution/cluster/executor_spec.ts   | 20 +++++++++++++++++--
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts b/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts
index 24e230ba36..6351ee77e1 100644
--- a/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts
+++ b/packages/compiler-cli/ngcc/src/execution/cluster/executor.ts
@@ -28,13 +28,13 @@ export class ClusterExecutor implements Executor {
 
   async execute(analyzeEntryPoints: AnalyzeEntryPointsFn, _createCompileFn: CreateCompileFn):
       Promise<void> {
-    return this.lockFile.lock(() => {
+    return this.lockFile.lock(async () => {
       this.logger.debug(
           `Running ngcc on ${this.constructor.name} (using ${this.workerCount} worker processes).`);
       const master = new ClusterMaster(
           this.workerCount, this.fileSystem, this.logger, this.fileWriter, this.pkgJsonUpdater,
           analyzeEntryPoints, this.createTaskCompletedCallback);
-      return master.run();
+      return await master.run();
     });
   }
 }
diff --git a/packages/compiler-cli/ngcc/src/locking/async_locker.ts b/packages/compiler-cli/ngcc/src/locking/async_locker.ts
index 6124b0985b..8d95d43c20 100644
--- a/packages/compiler-cli/ngcc/src/locking/async_locker.ts
+++ b/packages/compiler-cli/ngcc/src/locking/async_locker.ts
@@ -37,7 +37,11 @@ export class AsyncLocker {
    */
   async lock<T>(fn: () => Promise<T>): Promise<T> {
     await this.create();
-    return fn().finally(() => this.lockFile.remove());
+    try {
+      return await fn();
+    } finally {
+      this.lockFile.remove();
+    }
   }
 
   protected async create() {
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 215952dda6..fa8397e833 100644
--- a/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts
+++ b/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts
@@ -34,7 +34,7 @@ runInEachFileSystem(() => {
 
     beforeEach(() => {
       masterRunSpy = spyOn(ClusterMaster.prototype, 'run')
-                         .and.returnValue(Promise.resolve('CusterMaster#run()' as any));
+                         .and.returnValue(Promise.resolve('ClusterMaster#run()' as any));
       createTaskCompletedCallback = jasmine.createSpy('createTaskCompletedCallback');
 
       mockLogger = new MockLogger();
@@ -63,7 +63,7 @@ runInEachFileSystem(() => {
         const createCompilerFnSpy = jasmine.createSpy('createCompilerFn');
 
         expect(await executor.execute(analyzeEntryPointsSpy, createCompilerFnSpy))
-            .toBe('CusterMaster#run()' as any);
+            .toBe('ClusterMaster#run()' as any);
 
         expect(masterRunSpy).toHaveBeenCalledWith();
 
@@ -78,6 +78,22 @@ runInEachFileSystem(() => {
            expect(lockFileLog).toEqual(['write()', 'remove()']);
          });
 
+      it('should call LockFile.write() and LockFile.remove() if analyzeFn fails', async () => {
+        const analyzeEntryPointsSpy =
+            jasmine.createSpy('analyzeEntryPoints').and.throwError('analyze error');
+        const createCompilerFnSpy = jasmine.createSpy('createCompilerFn');
+        let error = '';
+        try {
+          await executor.execute(analyzeEntryPointsSpy, createCompilerFnSpy);
+        } catch (e) {
+          error = e.message;
+        }
+        expect(analyzeEntryPointsSpy).toHaveBeenCalledWith();
+        expect(createCompilerFnSpy).not.toHaveBeenCalled();
+        expect(error).toEqual('analyze error');
+        expect(lockFileLog).toEqual(['write()', 'remove()']);
+      });
+
       it('should call LockFile.write() and LockFile.remove() if master runner fails', async () => {
         const anyFn: () => any = () => undefined;
         masterRunSpy.and.returnValue(Promise.reject(new Error('master runner error')));