From 78a44768e0bc5038ac5d813641ca6287ce2f22ea 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 f9809bd8e9..23155c3ad3 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 42e1a0604a..f0ea1cebad 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 d47d34520c..ce79c66dcd 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')));