Remove synchronized usage in TaskCache

Author: lfkellogg

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

@@ -36,6 +36,7 @@
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.FirebaseApp;
+import com.google.firebase.annotations.concurrent.Blocking;
 import com.google.firebase.annotations.concurrent.Lightweight;
 import com.google.firebase.appdistribution.AppDistributionRelease;
 import com.google.firebase.appdistribution.BinaryType;
@@ -68,11 +69,11 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
   private final ReleaseIdentifier releaseIdentifier;
   private final ScreenshotTaker screenshotTaker;
-  private final Executor blockingExecutor;
-  private final FirebaseAppDistributionNotificationsManager notificationsManager;
-  private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
-  private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
+  @Blocking private final Executor blockingExecutor;
   @Lightweight private Executor lightweightExecutor;
+  private final FirebaseAppDistributionNotificationsManager notificationsManager;
+  private UpdateTaskCache updateIfNewReleaseAvailableTaskCache;
+  private TaskCache<AppDistributionRelease> checkForNewReleaseTaskCache;
   private AlertDialog updateConfirmationDialog;
   private AlertDialog signInConfirmationDialog;
   @Nullable private Activity dialogHostActivity = null;
@@ -109,6 +110,8 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
     this.blockingExecutor = blockingExecutor;
     this.notificationsManager =
         new FirebaseAppDistributionNotificationsManager(firebaseApp.getApplicationContext());
+    this.updateIfNewReleaseAvailableTaskCache = new UpdateTaskCache(lightweightExecutor);
+    this.checkForNewReleaseTaskCache = new TaskCache<>(lightweightExecutor);
     lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
     lifecycleNotifier.addOnActivityPausedListener(this::onActivityPaused);
     lifecycleNotifier.addOnActivityResumedListener(this::onActivityResumed);
@@ -117,7 +120,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   @Override
   @NonNull
   public UpdateTask updateIfNewReleaseAvailable() {
-    return updateIfNewReleaseAvailableTaskCache.getOrCreateTask(
+    return updateIfNewReleaseAvailableTaskCache.getOrCreateUpdateTask(
         () -> {
           UpdateTaskImpl updateTask = new UpdateTaskImpl();
           remakeSignInConfirmationDialog = false;

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

@@ -15,34 +15,58 @@
 package com.google.firebase.appdistribution.impl;
 
 import com.google.android.gms.tasks.Task;
+import com.google.android.gms.tasks.TaskCompletionSource;
+import com.google.firebase.annotations.concurrent.Lightweight;
+import com.google.firebase.concurrent.FirebaseExecutors;
+import java.util.concurrent.Executor;
 
 /**
- * A cache for Tasks, for use in cases where we only ever want one active task at a time for a
- * particular operation.
+ * A cache for a {@link Task}, for use in cases where we only ever want one active task at a time
+ * for a particular operation.
+ *
+ * <p>If you need a reference to an underlying TaskCompletionSource, use {@link
+ * TaskCompletionSourceCache} instead.
  */
-class TaskCache<T extends Task> {
+class TaskCache<T> {
 
-  /** A functional interface for a producer of a new Task. */
+  /** A functional interface for a producer of a new {@link Task}. */
   @FunctionalInterface
-  interface TaskProducer<T extends Task> {
+  interface TaskProducer<T> {
 
-    /** Produce a new Task. */
-    T produce();
+    /** Produce a new {@link Task}. */
+    Task<T> produce();
   }
 
-  private T cachedTask;
+  private Task<T> cachedTask;
+  private final Executor sequentialExecutor;
 
   /**
-   * 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.
+   * Constructor for a {@link TaskCache} that controls access using its own sequential executor
+   * backed by the given base executor.
    *
-   * @return the cached task if there is one and it is not completed, or else the result from {@code
-   *     producer.produce()}
+   * @param baseExecutor Executor (typically {@link Lightweight}) to back the sequential executor.
    */
-  synchronized T getOrCreateTask(TaskProducer<T> producer) {
-    if (cachedTask == null || cachedTask.isComplete()) {
-      cachedTask = producer.produce();
-    }
-    return cachedTask;
+  TaskCache(Executor baseExecutor) {
+    sequentialExecutor = FirebaseExecutors.newSequentialExecutor(baseExecutor);
+  }
+
+  /**
+   * Gets a cached {@link Task}, if there is one and it is not completed, or else calls the given
+   * {@code producer} and caches the return value.
+   */
+  Task<T> getOrCreateTask(TaskProducer<T> producer) {
+    TaskCompletionSource<T> taskCompletionSource = new TaskCompletionSource<>();
+    sequentialExecutor.execute(
+        () -> {
+          if (!isOngoing(cachedTask)) {
+            cachedTask = producer.produce();
+          }
+          TaskUtils.shadowTask(taskCompletionSource, cachedTask);
+        });
+    return taskCompletionSource.getTask();
+  }
+
+  private static <T> boolean isOngoing(Task<T> task) {
+    return task != null && !task.isComplete();
   }
 }

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/impl/ApkUpdaterTest.java

@@ -196,8 +196,7 @@ public void updateApk_whenSuccessfullyUpdated_notificationsSetCorrectly()
         .thenAnswer(answerWithValueAfterSleep(Tasks.forResult(null)));
 
     UpdateTask updateTask = apkUpdater.updateApk(TEST_RELEASE, true);
-    List<UpdateProgress> events =
-        awaitProgressEvents(updateTask, 3);
+    List<UpdateProgress> events = awaitProgressEvents(updateTask, 3);
 
     assertThat(events).hasSize(3);
     assertThat(events.get(0).getUpdateStatus()).isEqualTo(UpdateStatus.PENDING);