Move SharedPreferences usage to background thread

Author: lfkellogg

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

@@ -116,8 +116,6 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
 
   @Override
   @NonNull
-  // TODO(b/261014422): Use an explicit executor in continuations.
-  @SuppressLint("TaskMainThread")
   public UpdateTask updateIfNewReleaseAvailable() {
     return updateIfNewReleaseAvailableTaskCache.getOrCreateTask(
         () -> {
@@ -126,9 +124,18 @@ public UpdateTask updateIfNewReleaseAvailable() {
           remakeUpdateConfirmationDialog = false;
           dialogHostActivity = null;
 
-          lifecycleNotifier
-              .applyToForegroundActivityTask(this::showSignInConfirmationDialog)
-              .onSuccessTask(lightweightExecutor, unused -> signInTester())
+          signInStorage
+              .getSignInStatus()
+              .onSuccessTask(
+                  lightweightExecutor,
+                  signedIn -> {
+                    if (signedIn) {
+                      return Tasks.forResult(null);
+                    }
+                    return lifecycleNotifier
+                        .applyToForegroundActivityTask(this::showSignInConfirmationDialog)
+                        .onSuccessTask(lightweightExecutor, unused -> signInTester());
+                  })
               .onSuccessTask(lightweightExecutor, unused -> checkForNewRelease())
               .continueWithTask(
                   lightweightExecutor,
@@ -178,10 +185,6 @@ public UpdateTask updateIfNewReleaseAvailable() {
 
   @NonNull
   private Task<Void> showSignInConfirmationDialog(Activity hostActivity) {
-    if (isTesterSignedIn()) {
-      return Tasks.forResult(null);
-    }
-
     if (showSignInDialogTask == null || showSignInDialogTask.getTask().isComplete()) {
       showSignInDialogTask = new TaskCompletionSource<>();
     }
@@ -225,7 +228,7 @@ private Task<Void> showSignInConfirmationDialog(Activity hostActivity) {
 
   @Override
   public boolean isTesterSignedIn() {
-    return signInStorage.getSignInStatus();
+    return signInStorage.getSignInStatusBlocking();
   }
 
   @Override
@@ -242,19 +245,12 @@ public void signOutTester() {
   }
 
   @Override
-  @NonNull
-  // TODO(b/261014422): Use an explicit executor in continuations.
-  @SuppressLint("TaskMainThread")
   public synchronized Task<AppDistributionRelease> checkForNewRelease() {
-    if (!isTesterSignedIn()) {
-      return Tasks.forException(
-          new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
-    }
-
     return checkForNewReleaseTaskCache.getOrCreateTask(
         () ->
-            newReleaseFetcher
-                .checkForNewRelease()
+            assertTesterIsSignedIn()
+                .onSuccessTask(
+                    lightweightExecutor, unused -> newReleaseFetcher.checkForNewRelease())
                 .onSuccessTask(lightweightExecutor, release -> cachedNewRelease.set(release))
                 .onSuccessTask(
                     lightweightExecutor,
@@ -274,6 +270,21 @@ public synchronized Task<AppDistributionRelease> checkForNewRelease() {
                     }));
   }
 
+  private Task<Void> assertTesterIsSignedIn() {
+    return signInStorage
+        .getSignInStatus()
+        .onSuccessTask(
+            lightweightExecutor,
+            signedIn -> {
+              if (!signedIn) {
+                return Tasks.forException(
+                    new FirebaseAppDistributionException(
+                        "Tester is not signed in", AUTHENTICATION_FAILURE));
+              }
+              return Tasks.forResult(null);
+            });
+  }
+
   @Override
   @NonNull
   public UpdateTask updateApp() {
@@ -285,35 +296,39 @@ public UpdateTask updateApp() {
    * basic configuration and false for advanced configuration.
    */
   private UpdateTask updateApp(boolean 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(),
+        signInStorage.getSignInStatus(),
         lightweightExecutor,
-        release -> {
-          if (release == null) {
-            LogWrapper.v(TAG, "New release not found.");
+        signedIn -> {
+          if (!signedIn) {
             return getErrorUpdateTask(
                 new FirebaseAppDistributionException(
-                    ErrorMessages.RELEASE_NOT_FOUND_ERROR, UPDATE_NOT_AVAILABLE));
-          }
-          if (release.getDownloadUrl() == null) {
-            LogWrapper.v(TAG, "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);
+                    "Tester is not signed in", AUTHENTICATION_FAILURE));
           }
+          return TaskUtils.onSuccessUpdateTask(
+              cachedNewRelease.get(),
+              lightweightExecutor,
+              release -> {
+                if (release == null) {
+                  LogWrapper.v(TAG, "New release not found.");
+                  return getErrorUpdateTask(
+                      new FirebaseAppDistributionException(
+                          ErrorMessages.RELEASE_NOT_FOUND_ERROR, UPDATE_NOT_AVAILABLE));
+                }
+                if (release.getDownloadUrl() == null) {
+                  LogWrapper.v(TAG, "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);
+                }
+              });
         });
   }
 

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

@@ -110,14 +110,15 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
             firebaseInstallationsApiProvider,
             new TesterApiHttpClient(firebaseApp),
             blockingExecutor);
-    SignInStorage signInStorage = new SignInStorage(context);
+    SignInStorage signInStorage = new SignInStorage(context, backgroundExecutor);
     FirebaseAppDistributionLifecycleNotifier lifecycleNotifier =
         FirebaseAppDistributionLifecycleNotifier.getInstance();
     ReleaseIdentifier releaseIdentifier = new ReleaseIdentifier(firebaseApp, testerApiClient);
     FirebaseAppDistribution appDistribution =
         new FirebaseAppDistributionImpl(
             firebaseApp,
-            new TesterSignInManager(firebaseApp, firebaseInstallationsApiProvider, signInStorage),
+            new TesterSignInManager(
+                firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
             new NewReleaseFetcher(
                 firebaseApp.getApplicationContext(), testerApiClient, releaseIdentifier),
             new ApkUpdater(firebaseApp, new ApkInstaller(), blockingExecutor),

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

@@ -16,32 +16,55 @@
 
 import android.content.Context;
 import android.content.SharedPreferences;
-import com.google.firebase.components.Lazy;
+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.annotations.concurrent.Background;
+import java.util.concurrent.Executor;
 
 /** Class that handles storage for App Distribution SignIn persistence. */
 class SignInStorage {
 
   private static final String SIGNIN_PREFERENCES_NAME = "FirebaseAppDistributionSignInStorage";
   private static final String SIGNIN_TAG = "firebase_app_distribution_signin";
 
-  private final Lazy<SharedPreferences> signInSharedPreferences;
-
-  SignInStorage(Context applicationContext) {
-    this.signInSharedPreferences =
-        new Lazy(
-            () ->
-                // TODO(lkellogg): This constructs a SharedPreferences object, which touches disk
-                //   and therefore should be run on a @Background executor. This could ideally be
-                //   done by a new shared component.
-                applicationContext.getSharedPreferences(
-                    SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE));
+  private final Context applicationContext;
+  private final Executor backgroundExecutor;
+
+  SignInStorage(Context applicationContext, @Background Executor backgroundExecutor) {
+    this.applicationContext = applicationContext;
+    this.backgroundExecutor = backgroundExecutor;
+  }
+
+  Task<Void> setSignInStatus(boolean testerSignedIn) {
+    return getSharedPreferences()
+        .onSuccessTask(
+            backgroundExecutor,
+            sharedPreferences -> {
+              sharedPreferences.edit().putBoolean(SIGNIN_TAG, testerSignedIn).apply();
+              return null;
+            });
+  }
+
+  Task<Boolean> getSignInStatus() {
+    return getSharedPreferences()
+        .onSuccessTask(
+            backgroundExecutor,
+            sharedPreferences -> Tasks.forResult(sharedPreferences.getBoolean(SIGNIN_TAG, false)));
+  }
+
+  boolean getSignInStatusBlocking() {
+    return getSharedPreferencesBlocking().getBoolean(SIGNIN_TAG, false);
   }
 
-  void setSignInStatus(boolean testerSignedIn) {
-    this.signInSharedPreferences.get().edit().putBoolean(SIGNIN_TAG, testerSignedIn).apply();
+  private Task<SharedPreferences> getSharedPreferences() {
+    TaskCompletionSource<SharedPreferences> taskCompletionSource = new TaskCompletionSource<>();
+    backgroundExecutor.execute(
+        () -> taskCompletionSource.setResult(getSharedPreferencesBlocking()));
+    return taskCompletionSource.getTask();
   }
 
-  boolean getSignInStatus() {
-    return signInSharedPreferences.get().getBoolean(SIGNIN_TAG, false);
+  private SharedPreferences getSharedPreferencesBlocking() {
+    return applicationContext.getSharedPreferences(SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE);
   }
 }

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

@@ -33,11 +33,14 @@
 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.FirebaseAppDistributionException;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
 import com.google.firebase.inject.Provider;
 import com.google.firebase.installations.FirebaseInstallationsApi;
 import java.util.List;
+import java.util.concurrent.Executor;
 
 /** Class that handles signing in the tester. */
 class TesterSignInManager {
@@ -51,6 +54,9 @@ class TesterSignInManager {
   private final SignInStorage signInStorage;
   private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
 
+  // TODO: remove synchronized block usage in this class so this does not have to be blocking
+  @Blocking private final Executor blockingExecutor;
+
   private final Object signInTaskLock = new Object();
 
   @GuardedBy("signInTaskLock")
@@ -62,24 +68,28 @@ class TesterSignInManager {
   TesterSignInManager(
       @NonNull FirebaseApp firebaseApp,
       @NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
-      @NonNull final SignInStorage signInStorage) {
+      @NonNull final SignInStorage signInStorage,
+      @Blocking Executor blockingExecutor) {
     this(
         firebaseApp,
         firebaseInstallationsApiProvider,
         signInStorage,
-        FirebaseAppDistributionLifecycleNotifier.getInstance());
+        FirebaseAppDistributionLifecycleNotifier.getInstance(),
+        blockingExecutor);
   }
 
   @VisibleForTesting
   TesterSignInManager(
       @NonNull FirebaseApp firebaseApp,
       @NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
       @NonNull final SignInStorage signInStorage,
-      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier) {
+      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
+      @Lightweight Executor blockingExecutor) {
     this.firebaseApp = firebaseApp;
     this.firebaseInstallationsApiProvider = firebaseInstallationsApiProvider;
     this.signInStorage = signInStorage;
     this.lifecycleNotifier = lifecycleNotifier;
+    this.blockingExecutor = blockingExecutor;
 
     lifecycleNotifier.addOnActivityCreatedListener(this::onActivityCreated);
     lifecycleNotifier.addOnActivityResumedListener(this::onActivityResumed);
@@ -119,11 +129,22 @@ void onActivityResumed(Activity activity) {
   @SuppressLint("TaskMainThread")
   @NonNull
   public Task<Void> signInTester() {
-    if (signInStorage.getSignInStatus()) {
-      LogWrapper.v(TAG, "Tester is already signed in.");
-      return Tasks.forResult(null);
-    }
+    return signInStorage
+        .getSignInStatus()
+        .onSuccessTask(
+            blockingExecutor,
+            signedIn -> {
+              if (signedIn) {
+                LogWrapper.v(TAG, "Tester is already signed in.");
+                return Tasks.forResult(null);
+              }
+              return doSignInTester();
+            });
+  }
 
+  // TODO(b/261014422): Use an explicit executor in continuations.
+  @SuppressLint("TaskMainThread")
+  private Task<Void> doSignInTester() {
     synchronized (signInTaskLock) {
       if (signInTaskCompletionSource != null
           && !signInTaskCompletionSource.getTask().isComplete()) {
@@ -138,10 +159,15 @@ public Task<Void> signInTester() {
           .get()
           .getId()
           .addOnFailureListener(
+              blockingExecutor,
               handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, Status.AUTHENTICATION_FAILURE))
-          .onSuccessTask(this::getForegroundActivityAndOpenSignInFlow)
-          // Catch any unexpected failures to be safe.
-          .addOnFailureListener(handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN));
+          .addOnSuccessListener(
+              blockingExecutor,
+              fid ->
+                  getForegroundActivityAndOpenSignInFlow(fid)
+                      .addOnFailureListener(
+                          blockingExecutor,
+                          handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, Status.UNKNOWN)));
 
       return signInTaskCompletionSource.getTask();
     }