Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Dec 9, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8d14efe
Remove some synchronized blocks from FirebaseAppDistributionImpl
lfkellogg Nov 22, 2022
218af55
Rename UpdateTaskCache.java to TaskCache.java
lfkellogg Nov 22, 2022
2f398e6
Remove more sync blocks around new release cache
lfkellogg Nov 22, 2022
4c9243e
Some refactoring
lfkellogg Dec 7, 2022
8c8ac7b
Java formatting
lfkellogg Dec 7, 2022
1355cb1
Add tests
lfkellogg Dec 7, 2022
928ff1a
Add javadocs
lfkellogg Dec 8, 2022
f618340
Simplify SequentialReference and update tests
lfkellogg Dec 8, 2022
1472f3e
Update registrar
lfkellogg Dec 9, 2022
0b6a977
Fix test and add static imports from TestUtils
lfkellogg Dec 9, 2022
e1bd329
Incorporate some feedback
lfkellogg Dec 9, 2022
9ef0e50
Use actual lightweight executor in test
lfkellogg Dec 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions firebase-appdistribution/firebase-appdistribution.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
5 changes: 5 additions & 0 deletions ...rc/main/java/com/google/firebase/appdistribution/impl/AppDistributionReleaseInternal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
162 changes: 80 additions & 82 deletions ...n/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
Comment on lines +231 to +234
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also pulled this out of the getOrCreateTask because it does not actually depend on whether there is a cached task.


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
Expand All @@ -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
Expand All @@ -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)));
}
}
}
Expand All @@ -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(
Expand Down
15 changes: 12 additions & 3 deletions .../main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand All @@ -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);
Expand All @@ -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;
Expand Down
Loading