Use sequential executor in place of synchronized blocks for cached new release (#4430)

* Remove some synchronized blocks from FirebaseAppDistributionImpl

* Rename UpdateTaskCache.java to TaskCache.java

* Remove more sync blocks around new release cache

* Some refactoring

* Java formatting

* Add tests

* Add javadocs

* Simplify SequentialReference and update tests

* Update registrar

* Fix test and add static imports from TestUtils

* Incorporate some feedback

* Use actual lightweight executor in test

Author: lfkellogg

firebase-appdistribution/firebase-appdistribution.gradle

@@ -49,6 +49,7 @@ dependencies {
     implementation project(':firebase-installations-interop')
     implementation project(':firebase-common')
     testImplementation project(path: ':firebase-appdistribution')
+    testImplementation project(':integ-testing')
     runtimeOnly project(':firebase-installations')
 
     testImplementation 'junit:junit:4.13.2'

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

@@ -21,6 +21,8 @@
 
 /**
  * This class represents the AppDistributionRelease object returned by the App Distribution backend.
+ *
+ * <p>TODO(lkellogg): Combine this with AppDistributionReleaseImpl
  */
 @AutoValue
 abstract class AppDistributionReleaseInternal {
@@ -65,6 +67,9 @@ static Builder builder() {
   @Nullable
   abstract String getDownloadUrl();
 
+  @NonNull
+  abstract Builder toBuilder();
+
   /** Builder for {@link AppDistributionReleaseInternal}. */
   @AutoValue.Builder
   abstract static class Builder {

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

@@ -25,14 +25,14 @@
 import android.app.Activity;
 import android.app.AlertDialog;
 import android.content.Context;
-import androidx.annotation.GuardedBy;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.tasks.Task;
 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.Lightweight;
 import com.google.firebase.appdistribution.AppDistributionRelease;
 import com.google.firebase.appdistribution.BinaryType;
 import com.google.firebase.appdistribution.FirebaseAppDistribution;
@@ -41,6 +41,7 @@
 import com.google.firebase.appdistribution.UpdateProgress;
 import com.google.firebase.appdistribution.UpdateStatus;
 import com.google.firebase.appdistribution.UpdateTask;
+import java.util.concurrent.Executor;
 
 /**
  * This class is the "real" implementation of the Firebase App Distribution API which should only be
@@ -57,14 +58,10 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   private final ApkUpdater apkUpdater;
   private final AabUpdater aabUpdater;
   private final SignInStorage signInStorage;
-
-  private final Object cachedNewReleaseLock = new Object();
-
-  @GuardedBy("cachedNewReleaseLock")
-  private AppDistributionReleaseInternal cachedNewRelease;
-
+  private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
   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;
@@ -83,14 +80,17 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
       @NonNull ApkUpdater apkUpdater,
       @NonNull AabUpdater aabUpdater,
       @NonNull SignInStorage signInStorage,
-      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier) {
+      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
+      @NonNull @Lightweight Executor lightweightExecutor) {
     this.firebaseApp = firebaseApp;
     this.testerSignInManager = testerSignInManager;
     this.newReleaseFetcher = newReleaseFetcher;
     this.apkUpdater = apkUpdater;
     this.aabUpdater = aabUpdater;
     this.signInStorage = signInStorage;
     this.lifecycleNotifier = lifecycleNotifier;
+    this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
+    this.lightweightExecutor = lightweightExecutor;
     lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
     lifecycleNotifier.addOnActivityPausedListener(this::onActivityPaused);
     lifecycleNotifier.addOnActivityResumedListener(this::onActivityResumed);
@@ -110,9 +110,10 @@ public UpdateTask updateIfNewReleaseAvailable() {
 
           lifecycleNotifier
               .applyToForegroundActivityTask(this::showSignInConfirmationDialog)
-              .onSuccessTask(unused -> signInTester())
-              .onSuccessTask(unused -> checkForNewRelease())
+              .onSuccessTask(lightweightExecutor, unused -> signInTester())
+              .onSuccessTask(lightweightExecutor, unused -> checkForNewRelease())
               .continueWithTask(
+                  lightweightExecutor,
                   task -> {
                     if (!task.isSuccessful()) {
                       postProgressToCachedUpdateIfNewReleaseTask(
@@ -141,13 +142,16 @@ public UpdateTask updateIfNewReleaseAvailable() {
                         activity -> showUpdateConfirmationDialog(activity, release));
                   })
               .onSuccessTask(
+                  lightweightExecutor,
                   unused ->
                       updateApp(true)
                           .addOnProgressListener(
+                              lightweightExecutor,
                               updateProgress ->
                                   postProgressToCachedUpdateIfNewReleaseTask(
                                       updateTask, updateProgress)))
               .addOnFailureListener(
+                  lightweightExecutor,
                   exception -> setCachedUpdateIfNewReleaseCompletionError(updateTask, exception));
 
           return updateTask;
@@ -214,44 +218,42 @@ public Task<Void> signInTester() {
 
   @Override
   public void signOutTester() {
-    setCachedNewRelease(null);
-    signInStorage.setSignInStatus(false);
+    cachedNewRelease
+        .set(null)
+        .addOnSuccessListener(lightweightExecutor, unused -> signInStorage.setSignInStatus(false));
   }
 
   @Override
   @NonNull
   // TODO(b/261014422): Use an explicit executor in continuations.
   @SuppressLint("TaskMainThread")
   public synchronized Task<AppDistributionRelease> checkForNewRelease() {
-    return checkForNewReleaseTaskCache.getOrCreateTask(
-        () -> {
-          if (!isTesterSignedIn()) {
-            return Tasks.forException(
-                new FirebaseAppDistributionException(
-                    "Tester is not signed in", AUTHENTICATION_FAILURE));
-          }
+    if (!isTesterSignedIn()) {
+      return Tasks.forException(
+          new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
+    }
 
-          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();
-                    }
-                  });
-        });
+    return checkForNewReleaseTaskCache.getOrCreateTask(
+        () ->
+            newReleaseFetcher
+                .checkForNewRelease()
+                .onSuccessTask(lightweightExecutor, release -> cachedNewRelease.set(release))
+                .onSuccessTask(
+                    lightweightExecutor,
+                    release ->
+                        Tasks.forResult(ReleaseUtils.convertToAppDistributionRelease(release)))
+                .addOnFailureListener(
+                    lightweightExecutor,
+                    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
@@ -265,34 +267,36 @@ public UpdateTask updateApp() {
    * basic configuration and false for advanced configuration.
    */
   private UpdateTask updateApp(boolean showDownloadInNotificationManager) {
-    synchronized (cachedNewReleaseLock) {
-      if (!isTesterSignedIn()) {
-        UpdateTaskImpl updateTask = new UpdateTaskImpl();
-        updateTask.setException(
-            new FirebaseAppDistributionException(
-                "Tester is not signed in", AUTHENTICATION_FAILURE));
-        return updateTask;
-      }
-      if (cachedNewRelease == null) {
-        LogWrapper.getInstance().v("New release not found.");
-        return getErrorUpdateTask(
-            new FirebaseAppDistributionException(
-                ErrorMessages.RELEASE_NOT_FOUND_ERROR, UPDATE_NOT_AVAILABLE));
-      }
-      if (cachedNewRelease.getDownloadUrl() == null) {
-        LogWrapper.getInstance().v("Download failed to execute.");
-        return getErrorUpdateTask(
-            new FirebaseAppDistributionException(
-                ErrorMessages.DOWNLOAD_URL_NOT_FOUND,
-                FirebaseAppDistributionException.Status.DOWNLOAD_FAILURE));
-      }
-
-      if (cachedNewRelease.getBinaryType() == BinaryType.AAB) {
-        return aabUpdater.updateAab(cachedNewRelease);
-      } else {
-        return apkUpdater.updateApk(cachedNewRelease, showDownloadInNotificationManager);
-      }
+    if (!isTesterSignedIn()) {
+      UpdateTaskImpl updateTask = new UpdateTaskImpl();
+      updateTask.setException(
+          new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
+      return updateTask;
     }
+    return TaskUtils.onSuccessUpdateTask(
+        cachedNewRelease.get(),
+        lightweightExecutor,
+        release -> {
+          if (release == null) {
+            LogWrapper.getInstance().v("New release not found.");
+            return getErrorUpdateTask(
+                new FirebaseAppDistributionException(
+                    ErrorMessages.RELEASE_NOT_FOUND_ERROR, UPDATE_NOT_AVAILABLE));
+          }
+          if (release.getDownloadUrl() == null) {
+            LogWrapper.getInstance().v("Download failed to execute.");
+            return getErrorUpdateTask(
+                new FirebaseAppDistributionException(
+                    ErrorMessages.DOWNLOAD_URL_NOT_FOUND,
+                    FirebaseAppDistributionException.Status.DOWNLOAD_FAILURE));
+          }
+
+          if (release.getBinaryType() == BinaryType.AAB) {
+            return aabUpdater.updateAab(release);
+          } else {
+            return apkUpdater.updateApk(release, showDownloadInNotificationManager);
+          }
+        });
   }
 
   @VisibleForTesting
@@ -313,10 +317,13 @@ void onActivityResumed(Activity activity) {
             new FirebaseAppDistributionException(
                 ErrorMessages.HOST_ACTIVITY_INTERRUPTED, HOST_ACTIVITY_INTERRUPTED));
       } else {
-        synchronized (cachedNewReleaseLock) {
-          showUpdateConfirmationDialog(
-              activity, ReleaseUtils.convertToAppDistributionRelease(cachedNewRelease));
-        }
+        cachedNewRelease
+            .get()
+            .addOnSuccessListener(
+                lightweightExecutor,
+                release ->
+                    showUpdateConfirmationDialog(
+                        activity, ReleaseUtils.convertToAppDistributionRelease(release)));
       }
     }
   }
@@ -342,17 +349,8 @@ void onActivityDestroyed(@NonNull Activity activity) {
   }
 
   @VisibleForTesting
-  void setCachedNewRelease(@Nullable AppDistributionReleaseInternal newRelease) {
-    synchronized (cachedNewReleaseLock) {
-      cachedNewRelease = newRelease;
-    }
-  }
-
-  @VisibleForTesting
-  AppDistributionReleaseInternal getCachedNewRelease() {
-    synchronized (cachedNewReleaseLock) {
-      return cachedNewRelease;
-    }
+  SequentialReference<AppDistributionReleaseInternal> getCachedNewRelease() {
+    return cachedNewRelease;
   }
 
   private Task<Void> showUpdateConfirmationDialog(

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

@@ -20,6 +20,7 @@
 import androidx.annotation.NonNull;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.annotations.concurrent.Blocking;
+import com.google.firebase.annotations.concurrent.Lightweight;
 import com.google.firebase.appdistribution.FirebaseAppDistribution;
 import com.google.firebase.components.Component;
 import com.google.firebase.components.ComponentContainer;
@@ -47,13 +48,16 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
   @Override
   public @NonNull List<Component<?>> getComponents() {
     Qualified<Executor> blockingExecutor = Qualified.qualified(Blocking.class, Executor.class);
+    Qualified<Executor> lightweightExecutor =
+        Qualified.qualified(Lightweight.class, Executor.class);
     return Arrays.asList(
         Component.builder(FirebaseAppDistribution.class)
             .name(LIBRARY_NAME)
             .add(Dependency.required(FirebaseApp.class))
             .add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
             .add(Dependency.required(blockingExecutor))
-            .factory(c -> buildFirebaseAppDistribution(c, c.get(blockingExecutor)))
+            .add(Dependency.required(lightweightExecutor))
+            .factory(c -> buildFirebaseAppDistribution(c, blockingExecutor, lightweightExecutor))
             // construct FirebaseAppDistribution instance on startup so we can register for
             // activity lifecycle callbacks before the API is called
             .alwaysEager()
@@ -62,8 +66,12 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
   }
 
   private FirebaseAppDistribution buildFirebaseAppDistribution(
-      ComponentContainer container, Executor blockingExecutor) {
+      ComponentContainer container,
+      Qualified<Executor> blockingExecutorType,
+      Qualified<Executor> lightweightExecutorType) {
     FirebaseApp firebaseApp = container.get(FirebaseApp.class);
+    Executor blockingExecutor = container.get(blockingExecutorType);
+    Executor lightweightExecutor = container.get(lightweightExecutorType);
     Context context = firebaseApp.getApplicationContext();
     Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider =
         container.getProvider(FirebaseInstallationsApi.class);
@@ -86,7 +94,8 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
             new ApkUpdater(firebaseApp, new ApkInstaller(), blockingExecutor),
             new AabUpdater(blockingExecutor),
             signInStorage,
-            lifecycleNotifier);
+            lifecycleNotifier,
+            lightweightExecutor);
 
     if (context instanceof Application) {
       Application firebaseApplication = (Application) context;