Remove some synchronized blocks from FirebaseAppDistributionImpl

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

@@ -57,17 +57,13 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   private final AabUpdater aabUpdater;
   private final SignInStorage signInStorage;
 
-  private final Object updateIfNewReleaseTaskLock = new Object();
-
-  @GuardedBy("updateIfNewReleaseTaskLock")
-  private UpdateTaskImpl cachedUpdateIfNewReleaseTask;
-
   private final Object cachedNewReleaseLock = new Object();
 
   @GuardedBy("cachedNewReleaseLock")
   private AppDistributionReleaseInternal cachedNewRelease;
 
-  private Task<AppDistributionRelease> cachedCheckForNewReleaseTask;
+  private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
+  private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
   private AlertDialog updateConfirmationDialog;
   private AlertDialog signInConfirmationDialog;
   @Nullable private Activity dialogHostActivity = null;
@@ -102,55 +98,57 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   @Override
   @NonNull
   public UpdateTask updateIfNewReleaseAvailable() {
-    synchronized (updateIfNewReleaseTaskLock) {
-      if (updateIfNewReleaseAvailableIsTaskInProgress()) {
-        return cachedUpdateIfNewReleaseTask;
-      }
-      cachedUpdateIfNewReleaseTask = new UpdateTaskImpl();
-      remakeSignInConfirmationDialog = false;
-      remakeUpdateConfirmationDialog = false;
-      dialogHostActivity = null;
-    }
-
-    lifecycleNotifier
-        .applyToForegroundActivityTask(this::showSignInConfirmationDialog)
-        .onSuccessTask(unused -> signInTester())
-        .onSuccessTask(unused -> checkForNewRelease())
-        .continueWithTask(
-            task -> {
-              if (!task.isSuccessful()) {
-                postProgressToCachedUpdateIfNewReleaseTask(
-                    UpdateProgressImpl.builder()
-                        .setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
-                        .setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
-                        .setUpdateStatus(UpdateStatus.NEW_RELEASE_CHECK_FAILED)
-                        .build());
-              }
-              // if the task failed, this get() will cause the error to propagate to the handler
-              // below
-              AppDistributionRelease release = task.getResult();
-              if (release == null) {
-                postProgressToCachedUpdateIfNewReleaseTask(
-                    UpdateProgressImpl.builder()
-                        .setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
-                        .setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
-                        .setUpdateStatus(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE)
-                        .build());
-                setCachedUpdateIfNewReleaseResult();
-                return Tasks.forResult(null);
-              }
-              return lifecycleNotifier.applyToForegroundActivityTask(
-                  activity -> showUpdateConfirmationDialog(activity, release));
-            })
-        .onSuccessTask(
-            unused ->
-                updateApp(true)
-                    .addOnProgressListener(this::postProgressToCachedUpdateIfNewReleaseTask))
-        .addOnFailureListener(this::setCachedUpdateIfNewReleaseCompletionError);
-
-    synchronized (updateIfNewReleaseTaskLock) {
-      return cachedUpdateIfNewReleaseTask;
-    }
+    return updateIfNewReleaseAvailableTaskCache.getOrCreateTask(
+        () -> {
+          UpdateTaskImpl updateTask = new UpdateTaskImpl();
+          remakeSignInConfirmationDialog = false;
+          remakeUpdateConfirmationDialog = false;
+          dialogHostActivity = null;
+
+          lifecycleNotifier
+              .applyToForegroundActivityTask(this::showSignInConfirmationDialog)
+              .onSuccessTask(unused -> signInTester())
+              .onSuccessTask(unused -> checkForNewRelease())
+              .continueWithTask(
+                  task -> {
+                    if (!task.isSuccessful()) {
+                      postProgressToCachedUpdateIfNewReleaseTask(
+                          updateTask,
+                          UpdateProgressImpl.builder()
+                              .setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
+                              .setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
+                              .setUpdateStatus(UpdateStatus.NEW_RELEASE_CHECK_FAILED)
+                              .build());
+                    }
+                    // if the task failed, this get() will cause the error to propagate to the
+                    // handler below
+                    AppDistributionRelease release = task.getResult();
+                    if (release == null) {
+                      postProgressToCachedUpdateIfNewReleaseTask(
+                          updateTask,
+                          UpdateProgressImpl.builder()
+                              .setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
+                              .setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
+                              .setUpdateStatus(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE)
+                              .build());
+                      setCachedUpdateIfNewReleaseResult(updateTask);
+                      return Tasks.forResult(null);
+                    }
+                    return lifecycleNotifier.applyToForegroundActivityTask(
+                        activity -> showUpdateConfirmationDialog(activity, release));
+                  })
+              .onSuccessTask(
+                  unused ->
+                      updateApp(true)
+                          .addOnProgressListener(
+                              updateProgress ->
+                                  postProgressToCachedUpdateIfNewReleaseTask(
+                                      updateTask, updateProgress)))
+              .addOnFailureListener(
+                  exception -> setCachedUpdateIfNewReleaseCompletionError(updateTask, exception));
+
+          return updateTask;
+        });
   }
 
   @NonNull
@@ -220,37 +218,35 @@ public void signOutTester() {
   @Override
   @NonNull
   public synchronized Task<AppDistributionRelease> checkForNewRelease() {
-    if (cachedCheckForNewReleaseTask != null && !cachedCheckForNewReleaseTask.isComplete()) {
-      LogWrapper.getInstance().v("Response in progress");
-      return cachedCheckForNewReleaseTask;
-    }
-    if (!isTesterSignedIn()) {
-      return Tasks.forException(
-          new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
-    }
+    return checkForNewReleaseTaskCache.getOrCreateTask(
+        () -> {
+          if (!isTesterSignedIn()) {
+            return Tasks.forException(
+                new FirebaseAppDistributionException(
+                    "Tester is not signed in", AUTHENTICATION_FAILURE));
+          }
 
-    cachedCheckForNewReleaseTask =
-        newReleaseFetcher
-            .checkForNewRelease()
-            .onSuccessTask(
-                appDistributionReleaseInternal -> {
-                  setCachedNewRelease(appDistributionReleaseInternal);
-                  return Tasks.forResult(
-                      ReleaseUtils.convertToAppDistributionRelease(appDistributionReleaseInternal));
-                })
-            .addOnFailureListener(
-                e -> {
-                  if (e instanceof FirebaseAppDistributionException
-                      && ((FirebaseAppDistributionException) e).getErrorCode()
-                          == AUTHENTICATION_FAILURE) {
-                    // If CheckForNewRelease returns authentication error, the FID is no longer
-                    // valid or does not have access to the latest release. So sign out the tester
-                    // to force FID re-registration
-                    signOutTester();
-                  }
-                });
-
-    return cachedCheckForNewReleaseTask;
+          return newReleaseFetcher
+              .checkForNewRelease()
+              .onSuccessTask(
+                  appDistributionReleaseInternal -> {
+                    setCachedNewRelease(appDistributionReleaseInternal);
+                    return Tasks.forResult(
+                        ReleaseUtils.convertToAppDistributionRelease(
+                            appDistributionReleaseInternal));
+                  })
+              .addOnFailureListener(
+                  e -> {
+                    if (e instanceof FirebaseAppDistributionException
+                        && ((FirebaseAppDistributionException) e).getErrorCode()
+                            == AUTHENTICATION_FAILURE) {
+                      // If CheckForNewRelease returns authentication error, the FID is no longer
+                      // valid or does not have access to the latest release. So sign out the tester
+                      // to force FID re-registration
+                      signOutTester();
+                    }
+                  });
+        });
   }
 
   @Override
@@ -407,25 +403,20 @@ private Task<Void> showUpdateConfirmationDialog(
     return showUpdateDialogTask.getTask();
   }
 
-  private void setCachedUpdateIfNewReleaseCompletionError(Exception e) {
-    synchronized (updateIfNewReleaseTaskLock) {
-      safeSetTaskException(cachedUpdateIfNewReleaseTask, e);
-    }
+  private void setCachedUpdateIfNewReleaseCompletionError(UpdateTaskImpl updateTask, Exception e) {
+    safeSetTaskException(updateTask, e);
     dismissDialogs();
   }
 
-  private void postProgressToCachedUpdateIfNewReleaseTask(UpdateProgress progress) {
-    synchronized (updateIfNewReleaseTaskLock) {
-      if (cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete()) {
-        cachedUpdateIfNewReleaseTask.updateProgress(progress);
-      }
+  private void postProgressToCachedUpdateIfNewReleaseTask(
+      UpdateTaskImpl updateTask, UpdateProgress progress) {
+    if (updateTask != null && !updateTask.isComplete()) {
+      updateTask.updateProgress(progress);
     }
   }
 
-  private void setCachedUpdateIfNewReleaseResult() {
-    synchronized (updateIfNewReleaseTaskLock) {
-      safeSetTaskResult(cachedUpdateIfNewReleaseTask);
-    }
+  private void setCachedUpdateIfNewReleaseResult(UpdateTaskImpl updateTask) {
+    safeSetTaskResult(updateTask);
     dismissDialogs();
   }
 
@@ -444,12 +435,6 @@ private UpdateTaskImpl getErrorUpdateTask(Exception e) {
     return updateTask;
   }
 
-  private boolean updateIfNewReleaseAvailableIsTaskInProgress() {
-    synchronized (updateIfNewReleaseTaskLock) {
-      return cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete();
-    }
-  }
-
   private boolean awaitingSignInDialogConfirmation() {
     return (showSignInDialogTask != null
         && !showSignInDialogTask.getTask().isComplete()

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/TaskCache.java

@@ -0,0 +1,48 @@
+// Copyright 2022 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.appdistribution.impl;
+
+import com.google.android.gms.tasks.Task;
+
+/**
+ * A cache for Tasks, for use in cases where we only ever want one active task at a time for a
+ * particular operation.
+ */
+class TaskCache<T extends Task> {
+
+  /** A functional interface for a producer of a new Task. */
+  @FunctionalInterface
+  interface TaskProducer<T extends Task> {
+
+    /** Produce a new Task. */
+    T produce();
+  }
+
+  private T cachedTask;
+
+  /**
+   * Gets a cached task, if there is one and it is not completed, or else calls the given {@code
+   * producer} and caches the returned task.
+   *
+   * @return the cached task if there is one and it is not completed, or else the result from {@code
+   *     producer.produce()}
+   */
+  synchronized T getOrCreateTask(TaskProducer<T> producer) {
+    if (cachedTask == null || cachedTask.isComplete()) {
+      cachedTask = producer.produce();
+    }
+    return cachedTask;
+  }
+}