Remove synchronized blocks from NewReleaseFetcher

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;
@@ -65,22 +66,23 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   private final ApkUpdater apkUpdater;
   private final AabUpdater aabUpdater;
   private final SignInStorage signInStorage;
-  private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
   private final ReleaseIdentifier releaseIdentifier;
   private final ScreenshotTaker screenshotTaker;
-  private final Executor blockingExecutor;
+  @Lightweight private final Executor lightweightExecutor;
+  @Blocking private final Executor blockingExecutor;
+  private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
+  private final TaskCache<AppDistributionRelease> checkForNewReleaseTaskCache;
+  private final UpdateTaskCache updateIfNewReleaseAvailableTaskCache;
   private final FirebaseAppDistributionNotificationsManager notificationsManager;
-  private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
-  private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
-  @Lightweight private Executor lightweightExecutor;
-  private AlertDialog updateConfirmationDialog;
-  private AlertDialog signInConfirmationDialog;
-  @Nullable private Activity dialogHostActivity = null;
+  private final AtomicBoolean feedbackInProgress = new AtomicBoolean(false);
+
+  @Nullable private AlertDialog updateConfirmationDialog;
+  @Nullable private AlertDialog signInConfirmationDialog;
+  @Nullable private Activity dialogHostActivity;
   private boolean remakeSignInConfirmationDialog = false;
   private boolean remakeUpdateConfirmationDialog = false;
-  private TaskCompletionSource<Void> showSignInDialogTask = null;
-  private TaskCompletionSource<Void> showUpdateDialogTask = null;
-  private final AtomicBoolean feedbackInProgress = new AtomicBoolean(false);
+  @Nullable private TaskCompletionSource<Void> showSignInDialogTask;
+  @Nullable private TaskCompletionSource<Void> showUpdateDialogTask;
 
   @VisibleForTesting
   FirebaseAppDistributionImpl(
@@ -94,7 +96,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
       @NonNull ReleaseIdentifier releaseIdentifier,
       @NonNull ScreenshotTaker screenshotTaker,
       @NonNull @Lightweight Executor lightweightExecutor,
-      @NonNull Executor blockingExecutor) {
+      @NonNull @Blocking Executor blockingExecutor) {
     this.firebaseApp = firebaseApp;
     this.testerSignInManager = testerSignInManager;
     this.newReleaseFetcher = newReleaseFetcher;
@@ -103,10 +105,12 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
     this.signInStorage = signInStorage;
     this.releaseIdentifier = releaseIdentifier;
     this.lifecycleNotifier = lifecycleNotifier;
-    this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
-    this.lightweightExecutor = lightweightExecutor;
     this.screenshotTaker = screenshotTaker;
+    this.lightweightExecutor = lightweightExecutor;
     this.blockingExecutor = blockingExecutor;
+    this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
+    this.checkForNewReleaseTaskCache = new TaskCache<>(lightweightExecutor);
+    this.updateIfNewReleaseAvailableTaskCache = new UpdateTaskCache(lightweightExecutor);
     this.notificationsManager =
         new FirebaseAppDistributionNotificationsManager(firebaseApp.getApplicationContext());
     lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
@@ -117,7 +121,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/FirebaseAppDistributionRegistrar.java

@@ -120,7 +120,10 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
             new TesterSignInManager(
                 firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
             new NewReleaseFetcher(
-                firebaseApp.getApplicationContext(), testerApiClient, releaseIdentifier),
+                firebaseApp.getApplicationContext(),
+                testerApiClient,
+                releaseIdentifier,
+                lightweightExecutor),
             new ApkUpdater(
                 firebaseApp,
                 new ApkInstaller(lightweightExecutor),

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

@@ -16,16 +16,17 @@
 
 import static com.google.firebase.appdistribution.impl.PackageInfoUtils.getPackageInfo;
 
-import android.annotation.SuppressLint;
 import android.content.Context;
 import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 import androidx.core.content.pm.PackageInfoCompat;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.Tasks;
+import com.google.firebase.annotations.concurrent.Lightweight;
 import com.google.firebase.appdistribution.BinaryType;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
+import java.util.concurrent.Executor;
 
 /**
  * Class that handles fetching the latest release from App Distribution and determining if it is a
@@ -36,33 +37,32 @@ class NewReleaseFetcher {
 
   private final FirebaseAppDistributionTesterApiClient firebaseAppDistributionTesterApiClient;
   private final ReleaseIdentifier releaseIdentifier;
+  private @Lightweight Executor lightweightExecutor;
   private final Context context;
 
-  Task<AppDistributionReleaseInternal> cachedCheckForNewRelease = null;
+  TaskCache<AppDistributionReleaseInternal> cachedCheckForNewRelease;
 
   NewReleaseFetcher(
       @NonNull Context applicationContext,
       @NonNull FirebaseAppDistributionTesterApiClient firebaseAppDistributionTesterApiClient,
-      ReleaseIdentifier releaseIdentifier) {
+      ReleaseIdentifier releaseIdentifier,
+      @Lightweight Executor lightweightExecutor) {
     this.firebaseAppDistributionTesterApiClient = firebaseAppDistributionTesterApiClient;
     this.context = applicationContext;
     this.releaseIdentifier = releaseIdentifier;
+    this.lightweightExecutor = lightweightExecutor;
+    this.cachedCheckForNewRelease = new TaskCache<>(lightweightExecutor);
   }
 
-  // TODO(b/261014422): Use an explicit executor in continuations.
-  @SuppressLint("TaskMainThread")
   @NonNull
-  public synchronized Task<AppDistributionReleaseInternal> checkForNewRelease() {
-    if (cachedCheckForNewRelease != null && !cachedCheckForNewRelease.isComplete()) {
-      return cachedCheckForNewRelease;
-    }
-
-    this.cachedCheckForNewRelease =
-        firebaseAppDistributionTesterApiClient
-            .fetchNewRelease()
-            .onSuccessTask(release -> Tasks.forResult(isNewerRelease(release) ? release : null));
-
-    return cachedCheckForNewRelease;
+  public Task<AppDistributionReleaseInternal> checkForNewRelease() {
+    return cachedCheckForNewRelease.getOrCreateTask(
+        () ->
+            firebaseAppDistributionTesterApiClient
+                .fetchNewRelease()
+                .onSuccessTask(
+                    lightweightExecutor,
+                    release -> Tasks.forResult(isNewerRelease(release) ? release : null)));
   }
 
   private boolean isNewerRelease(AppDistributionReleaseInternal retrievedNewRelease)

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();
   }
 }