Skip to content

Remove synchronized blocks from TesterSignInManager #4520

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 2 commits into from
Jan 4, 2023
Merged

Remove synchronized blocks from TesterSignInManager #4520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6f79fd
Some improvements to the task caches
lfkellogg Jan 4, 2023
c527645
Remove synchronized blocks from TesterSignInManager
lfkellogg Jan 4, 2023
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
2 changes: 1 addition & 1 deletion .../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 @@ -118,7 +118,7 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
new FirebaseAppDistributionImpl(
firebaseApp,
new TesterSignInManager(
firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
firebaseApp, firebaseInstallationsApiProvider, signInStorage, lightweightExecutor),
new NewReleaseFetcher(
firebaseApp.getApplicationContext(),
testerApiClient,
Expand Down
61 changes: 44 additions & 17 deletions ...ion/src/main/java/com/google/firebase/appdistribution/impl/TaskCompletionSourceCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ Task<T> getOrCreateTaskFromCompletionSource(TaskCompletionSourceProducer<T> prod
TaskCompletionSource<T> taskCompletionSource = new TaskCompletionSource<>();
sequentialExecutor.execute(
() -> {
if (!isOngoing(cachedTaskCompletionSource)) {
if (cachedTaskCompletionSource == null
|| cachedTaskCompletionSource.getTask().isComplete()) {
cachedTaskCompletionSource = producer.produce();
}
TaskUtils.shadowTask(taskCompletionSource, cachedTaskCompletionSource.getTask());
Expand All @@ -68,36 +69,62 @@ Task<T> getOrCreateTaskFromCompletionSource(TaskCompletionSourceProducer<T> prod
}

/**
* Sets the result on the cached {@link TaskCompletionSource}, if there is one and it is not
* completed.
* Sets the result on the cached {@link TaskCompletionSource}.
*
* <p>The task must be created using {@link #getOrCreateTaskFromCompletionSource} before calling
* this method.
*
* @return A task that resolves when the change is applied, or fails with {@link
* IllegalStateException} if the task was not created yet or was already completed.
*/
Task<Void> setResult(T result) {
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
sequentialExecutor.execute(
() -> {
if (isOngoing(cachedTaskCompletionSource)) {
cachedTaskCompletionSource.setResult(result);
}
});
return taskCompletionSource.getTask();
return executeIfOngoing(() -> cachedTaskCompletionSource.setResult(result));
}

/**
* Sets the exception on the cached {@link TaskCompletionSource}, if there is one and it is not
* completed.
*
* <p>The task must be created using {@link #getOrCreateTaskFromCompletionSource} before calling
* this method.
*
* @return A task that resolves when the change is applied, or fails with {@link
* IllegalStateException} if the task was not created yet or was already completed.
*/
Task<Void> setException(Exception e) {
return executeIfOngoing(() -> cachedTaskCompletionSource.setException(e));
}

/**
* Returns a task indicating whether the cached {@link TaskCompletionSource} has been initialized
* and is not yet completed.
*/
Task<Boolean> isOngoing() {
TaskCompletionSource<Boolean> taskCompletionSource = new TaskCompletionSource<>();
sequentialExecutor.execute(
() ->
taskCompletionSource.setResult(
cachedTaskCompletionSource != null
&& !cachedTaskCompletionSource.getTask().isComplete()));
return taskCompletionSource.getTask();
}

private Task<Void> executeIfOngoing(Runnable r) {
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
sequentialExecutor.execute(
() -> {
if (isOngoing(cachedTaskCompletionSource)) {
cachedTaskCompletionSource.setException(e);
if (cachedTaskCompletionSource == null) {
taskCompletionSource.setException(
new IllegalStateException(
"Tried to set exception before calling getOrCreateTaskFromCompletionSource()"));
} else if (cachedTaskCompletionSource.getTask().isComplete()) {
taskCompletionSource.setException(
new IllegalStateException("Tried to set exception on a completed task"));
} else {
r.run();
taskCompletionSource.setResult(null);
}
});
return taskCompletionSource.getTask();
}

private static <T> boolean isOngoing(TaskCompletionSource<T> taskCompletionSource) {
return taskCompletionSource != null && !taskCompletionSource.getTask().isComplete();
}
}
138 changes: 45 additions & 93 deletions ...tribution/src/main/java/com/google/firebase/appdistribution/impl/TesterSignInManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,12 @@
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_FAILURE;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UNKNOWN;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;

import android.annotation.SuppressLint;
import android.app.Activity;
import android.content.Context;
import android.content.Intent;
import android.content.pm.ResolveInfo;
import android.net.Uri;
import androidx.annotation.GuardedBy;
import androidx.annotation.NonNull;
import androidx.annotation.VisibleForTesting;
import androidx.browser.customtabs.CustomTabsIntent;
Expand All @@ -35,7 +31,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.FirebaseAppDistributionException;
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
import com.google.firebase.inject.Provider;
Expand All @@ -55,28 +51,22 @@ 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;
@Lightweight private final Executor lightweightExecutor;
private final TaskCompletionSourceCache<Void> signInTaskCompletionSourceCache;

private final Object signInTaskLock = new Object();

@GuardedBy("signInTaskLock")
private boolean hasBeenSentToBrowserForCurrentTask = false;

@GuardedBy("signInTaskLock")
private TaskCompletionSource<Void> signInTaskCompletionSource = null;

TesterSignInManager(
@NonNull FirebaseApp firebaseApp,
@NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
@NonNull final SignInStorage signInStorage,
@Blocking Executor blockingExecutor) {
@Lightweight Executor lightweightExecutor) {
this(
firebaseApp,
firebaseInstallationsApiProvider,
signInStorage,
FirebaseAppDistributionLifecycleNotifier.getInstance(),
blockingExecutor);
lightweightExecutor);
}

@VisibleForTesting
Expand All @@ -85,12 +75,13 @@ class TesterSignInManager {
@NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
@NonNull final SignInStorage signInStorage,
@NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
@Blocking Executor blockingExecutor) {
@Lightweight Executor lightweightExecutor) {
this.firebaseApp = firebaseApp;
this.firebaseInstallationsApiProvider = firebaseInstallationsApiProvider;
this.signInStorage = signInStorage;
this.lifecycleNotifier = lifecycleNotifier;
this.blockingExecutor = blockingExecutor;
this.lightweightExecutor = lightweightExecutor;
this.signInTaskCompletionSourceCache = new TaskCompletionSourceCache<>(lightweightExecutor);

lifecycleNotifier.addOnActivityCreatedListener(this::onActivityCreated);
lifecycleNotifier.addOnActivityResumedListener(this::onActivityResumed);
Expand All @@ -104,13 +95,11 @@ void onActivityCreated(Activity activity) {
LogWrapper.v(TAG, "Sign in completed");
this.signInStorage
.setSignInStatus(true)
.addOnSuccessListener(blockingExecutor, unused -> this.setSuccessfulSignInResult())
.addOnSuccessListener(
lightweightExecutor, unused -> signInTaskCompletionSourceCache.setResult(null))
.addOnFailureListener(
blockingExecutor,
e ->
this.setSignInTaskCompletionError(
new FirebaseAppDistributionException(
"Error storing tester sign in state", UNKNOWN, e)));
lightweightExecutor,
handleTaskFailure("Error storing tester sign in state", UNKNOWN));
}
}

Expand All @@ -120,27 +109,26 @@ void onActivityResumed(Activity activity) {
// SignInResult and InstallActivity are internal to the SDK and should not be treated as
// reentering the app
return;
} else {
// Throw error if app reentered during sign in
synchronized (signInTaskLock) {
if (awaitingResultFromBrowser()) {
LogWrapper.e(TAG, "App Resumed without sign in flow completing.");
setSignInTaskCompletionError(
}

// Throw error if app reentered during sign in
if (hasBeenSentToBrowserForCurrentTask) {
signInTaskCompletionSourceCache
.setException(
new FirebaseAppDistributionException(
ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED));
}
}
ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED))
.addOnSuccessListener(
lightweightExecutor,
unused -> LogWrapper.e(TAG, "App resumed without sign in flow completing."));
}
}

// TODO(b/261014422): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
@NonNull
public Task<Void> signInTester() {
return signInStorage
.getSignInStatus()
.onSuccessTask(
blockingExecutor,
lightweightExecutor,
signedIn -> {
if (signedIn) {
LogWrapper.v(TAG, "Tester is already signed in.");
Expand All @@ -150,80 +138,44 @@ public Task<Void> signInTester() {
});
}

// TODO(b/261014422): Use an explicit executor in continuations.
@SuppressLint("TaskMainThread")
private Task<Void> doSignInTester() {
synchronized (signInTaskLock) {
if (signInTaskCompletionSource != null
&& !signInTaskCompletionSource.getTask().isComplete()) {
LogWrapper.v(TAG, "Detected In-Progress sign in task. Returning the same task.");
return signInTaskCompletionSource.getTask();
}

signInTaskCompletionSource = new TaskCompletionSource<>();
hasBeenSentToBrowserForCurrentTask = false;

firebaseInstallationsApiProvider
.get()
.getId()
.addOnFailureListener(
blockingExecutor,
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE))
.addOnSuccessListener(
blockingExecutor,
fid ->
getForegroundActivityAndOpenSignInFlow(fid)
.addOnFailureListener(
blockingExecutor,
handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, UNKNOWN)));

return signInTaskCompletionSource.getTask();
}
return signInTaskCompletionSourceCache.getOrCreateTaskFromCompletionSource(
() -> {
TaskCompletionSource<Void> signInTaskCompletionSource = new TaskCompletionSource<>();
hasBeenSentToBrowserForCurrentTask = false;
firebaseInstallationsApiProvider
.get()
.getId()
.addOnFailureListener(
lightweightExecutor,
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE))
.addOnSuccessListener(
lightweightExecutor,
fid ->
getForegroundActivityAndOpenSignInFlow(fid)
.addOnFailureListener(
lightweightExecutor,
handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, UNKNOWN)));
return signInTaskCompletionSource;
});
}

private Task<Void> getForegroundActivityAndOpenSignInFlow(String fid) {
return lifecycleNotifier.consumeForegroundActivity(
activity -> {
// Launch the intent outside of the synchronized block because we don't need to wait
// for the lock, and we don't want to risk the activity leaving the foreground in
// the meantime.
openSignInFlowInBrowser(fid, activity);
// This synchronized block is required by the @GuardedBy annotation, but is not
// practically required in this case because the only reads of this variable are on
// the main thread, which this callback is also running on.
synchronized (signInTaskLock) {
hasBeenSentToBrowserForCurrentTask = true;
}
hasBeenSentToBrowserForCurrentTask = true;
});
}

private OnFailureListener handleTaskFailure(String message, Status status) {
return e -> {
LogWrapper.e(TAG, message, e);
setSignInTaskCompletionError(new FirebaseAppDistributionException(message, status, e));
signInTaskCompletionSourceCache.setException(
new FirebaseAppDistributionException(message, status, e));
};
}

private boolean awaitingResultFromBrowser() {
synchronized (signInTaskLock) {
return signInTaskCompletionSource != null
&& !signInTaskCompletionSource.getTask().isComplete()
&& hasBeenSentToBrowserForCurrentTask;
}
}

private void setSignInTaskCompletionError(FirebaseAppDistributionException e) {
synchronized (signInTaskLock) {
safeSetTaskException(signInTaskCompletionSource, e);
}
}

private void setSuccessfulSignInResult() {
synchronized (signInTaskLock) {
safeSetTaskResult(signInTaskCompletionSource, null);
}
}

private static String getApplicationName(Context context) {
try {
return context.getApplicationInfo().loadLabel(context.getPackageManager()).toString();
Expand Down
Loading