Skip to content

Commit f08df51

Browse files
lfkelloggkaibolay
lfkellogg
authored and
kaibolay
committed
Remove synchronized blocks from TesterSignInManager (#4520)
* Some improvements to the task caches * Remove synchronized blocks from TesterSignInManager
1 parent 568083a commit f08df51

File tree

3 files changed

+55
-103
lines changed

3 files changed

+55
-103
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
118118
new FirebaseAppDistributionImpl(
119119
firebaseApp,
120120
new TesterSignInManager(
121-
firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
121+
firebaseApp, firebaseInstallationsApiProvider, signInStorage, lightweightExecutor),
122122
new NewReleaseFetcher(
123123
firebaseApp.getApplicationContext(),
124124
testerApiClient,

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

Lines changed: 45 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,12 @@
1717
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
1818
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_FAILURE;
1919
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UNKNOWN;
20-
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
21-
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;
2220

23-
import android.annotation.SuppressLint;
2421
import android.app.Activity;
2522
import android.content.Context;
2623
import android.content.Intent;
2724
import android.content.pm.ResolveInfo;
2825
import android.net.Uri;
29-
import androidx.annotation.GuardedBy;
3026
import androidx.annotation.NonNull;
3127
import androidx.annotation.VisibleForTesting;
3228
import androidx.browser.customtabs.CustomTabsIntent;
@@ -35,7 +31,7 @@
3531
import com.google.android.gms.tasks.TaskCompletionSource;
3632
import com.google.android.gms.tasks.Tasks;
3733
import com.google.firebase.FirebaseApp;
38-
import com.google.firebase.annotations.concurrent.Blocking;
34+
import com.google.firebase.annotations.concurrent.Lightweight;
3935
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
4036
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
4137
import com.google.firebase.inject.Provider;
@@ -55,28 +51,22 @@ class TesterSignInManager {
5551
private final SignInStorage signInStorage;
5652
private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
5753

58-
// TODO: remove synchronized block usage in this class so this does not have to be blocking
59-
@Blocking private final Executor blockingExecutor;
54+
@Lightweight private final Executor lightweightExecutor;
55+
private final TaskCompletionSourceCache<Void> signInTaskCompletionSourceCache;
6056

61-
private final Object signInTaskLock = new Object();
62-
63-
@GuardedBy("signInTaskLock")
6457
private boolean hasBeenSentToBrowserForCurrentTask = false;
6558

66-
@GuardedBy("signInTaskLock")
67-
private TaskCompletionSource<Void> signInTaskCompletionSource = null;
68-
6959
TesterSignInManager(
7060
@NonNull FirebaseApp firebaseApp,
7161
@NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
7262
@NonNull final SignInStorage signInStorage,
73-
@Blocking Executor blockingExecutor) {
63+
@Lightweight Executor lightweightExecutor) {
7464
this(
7565
firebaseApp,
7666
firebaseInstallationsApiProvider,
7767
signInStorage,
7868
FirebaseAppDistributionLifecycleNotifier.getInstance(),
79-
blockingExecutor);
69+
lightweightExecutor);
8070
}
8171

8272
@VisibleForTesting
@@ -85,12 +75,13 @@ class TesterSignInManager {
8575
@NonNull Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider,
8676
@NonNull final SignInStorage signInStorage,
8777
@NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
88-
@Blocking Executor blockingExecutor) {
78+
@Lightweight Executor lightweightExecutor) {
8979
this.firebaseApp = firebaseApp;
9080
this.firebaseInstallationsApiProvider = firebaseInstallationsApiProvider;
9181
this.signInStorage = signInStorage;
9282
this.lifecycleNotifier = lifecycleNotifier;
93-
this.blockingExecutor = blockingExecutor;
83+
this.lightweightExecutor = lightweightExecutor;
84+
this.signInTaskCompletionSourceCache = new TaskCompletionSourceCache<>(lightweightExecutor);
9485

9586
lifecycleNotifier.addOnActivityCreatedListener(this::onActivityCreated);
9687
lifecycleNotifier.addOnActivityResumedListener(this::onActivityResumed);
@@ -104,13 +95,11 @@ void onActivityCreated(Activity activity) {
10495
LogWrapper.v(TAG, "Sign in completed");
10596
this.signInStorage
10697
.setSignInStatus(true)
107-
.addOnSuccessListener(blockingExecutor, unused -> this.setSuccessfulSignInResult())
98+
.addOnSuccessListener(
99+
lightweightExecutor, unused -> signInTaskCompletionSourceCache.setResult(null))
108100
.addOnFailureListener(
109-
blockingExecutor,
110-
e ->
111-
this.setSignInTaskCompletionError(
112-
new FirebaseAppDistributionException(
113-
"Error storing tester sign in state", UNKNOWN, e)));
101+
lightweightExecutor,
102+
handleTaskFailure("Error storing tester sign in state", UNKNOWN));
114103
}
115104
}
116105

@@ -120,27 +109,26 @@ void onActivityResumed(Activity activity) {
120109
// SignInResult and InstallActivity are internal to the SDK and should not be treated as
121110
// reentering the app
122111
return;
123-
} else {
124-
// Throw error if app reentered during sign in
125-
synchronized (signInTaskLock) {
126-
if (awaitingResultFromBrowser()) {
127-
LogWrapper.e(TAG, "App Resumed without sign in flow completing.");
128-
setSignInTaskCompletionError(
112+
}
113+
114+
// Throw error if app reentered during sign in
115+
if (hasBeenSentToBrowserForCurrentTask) {
116+
signInTaskCompletionSourceCache
117+
.setException(
129118
new FirebaseAppDistributionException(
130-
ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED));
131-
}
132-
}
119+
ErrorMessages.AUTHENTICATION_CANCELED, AUTHENTICATION_CANCELED))
120+
.addOnSuccessListener(
121+
lightweightExecutor,
122+
unused -> LogWrapper.e(TAG, "App resumed without sign in flow completing."));
133123
}
134124
}
135125

136-
// TODO(b/261014422): Use an explicit executor in continuations.
137-
@SuppressLint("TaskMainThread")
138126
@NonNull
139127
public Task<Void> signInTester() {
140128
return signInStorage
141129
.getSignInStatus()
142130
.onSuccessTask(
143-
blockingExecutor,
131+
lightweightExecutor,
144132
signedIn -> {
145133
if (signedIn) {
146134
LogWrapper.v(TAG, "Tester is already signed in.");
@@ -150,80 +138,44 @@ public Task<Void> signInTester() {
150138
});
151139
}
152140

153-
// TODO(b/261014422): Use an explicit executor in continuations.
154-
@SuppressLint("TaskMainThread")
155141
private Task<Void> doSignInTester() {
156-
synchronized (signInTaskLock) {
157-
if (signInTaskCompletionSource != null
158-
&& !signInTaskCompletionSource.getTask().isComplete()) {
159-
LogWrapper.v(TAG, "Detected In-Progress sign in task. Returning the same task.");
160-
return signInTaskCompletionSource.getTask();
161-
}
162-
163-
signInTaskCompletionSource = new TaskCompletionSource<>();
164-
hasBeenSentToBrowserForCurrentTask = false;
165-
166-
firebaseInstallationsApiProvider
167-
.get()
168-
.getId()
169-
.addOnFailureListener(
170-
blockingExecutor,
171-
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE))
172-
.addOnSuccessListener(
173-
blockingExecutor,
174-
fid ->
175-
getForegroundActivityAndOpenSignInFlow(fid)
176-
.addOnFailureListener(
177-
blockingExecutor,
178-
handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, UNKNOWN)));
179-
180-
return signInTaskCompletionSource.getTask();
181-
}
142+
return signInTaskCompletionSourceCache.getOrCreateTaskFromCompletionSource(
143+
() -> {
144+
TaskCompletionSource<Void> signInTaskCompletionSource = new TaskCompletionSource<>();
145+
hasBeenSentToBrowserForCurrentTask = false;
146+
firebaseInstallationsApiProvider
147+
.get()
148+
.getId()
149+
.addOnFailureListener(
150+
lightweightExecutor,
151+
handleTaskFailure(ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE))
152+
.addOnSuccessListener(
153+
lightweightExecutor,
154+
fid ->
155+
getForegroundActivityAndOpenSignInFlow(fid)
156+
.addOnFailureListener(
157+
lightweightExecutor,
158+
handleTaskFailure(ErrorMessages.UNKNOWN_ERROR, UNKNOWN)));
159+
return signInTaskCompletionSource;
160+
});
182161
}
183162

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

200171
private OnFailureListener handleTaskFailure(String message, Status status) {
201172
return e -> {
202173
LogWrapper.e(TAG, message, e);
203-
setSignInTaskCompletionError(new FirebaseAppDistributionException(message, status, e));
174+
signInTaskCompletionSourceCache.setException(
175+
new FirebaseAppDistributionException(message, status, e));
204176
};
205177
}
206178

207-
private boolean awaitingResultFromBrowser() {
208-
synchronized (signInTaskLock) {
209-
return signInTaskCompletionSource != null
210-
&& !signInTaskCompletionSource.getTask().isComplete()
211-
&& hasBeenSentToBrowserForCurrentTask;
212-
}
213-
}
214-
215-
private void setSignInTaskCompletionError(FirebaseAppDistributionException e) {
216-
synchronized (signInTaskLock) {
217-
safeSetTaskException(signInTaskCompletionSource, e);
218-
}
219-
}
220-
221-
private void setSuccessfulSignInResult() {
222-
synchronized (signInTaskLock) {
223-
safeSetTaskResult(signInTaskCompletionSource, null);
224-
}
225-
}
226-
227179
private static String getApplicationName(Context context) {
228180
try {
229181
return context.getApplicationInfo().loadLabel(context.getPackageManager()).toString();

‎firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/impl/TesterSignInManagerTest.java

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
import com.google.firebase.FirebaseApp;
4646
import com.google.firebase.FirebaseOptions;
4747
import com.google.firebase.annotations.concurrent.Background;
48-
import com.google.firebase.annotations.concurrent.Blocking;
48+
import com.google.firebase.annotations.concurrent.Lightweight;
4949
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
5050
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
5151
import com.google.firebase.appdistribution.impl.FirebaseAppDistributionServiceImplTest.TestActivity;
@@ -88,7 +88,7 @@ public class TesterSignInManagerTest {
8888
private TestActivity activity;
8989
private ShadowActivity shadowActivity;
9090
private ShadowPackageManager shadowPackageManager;
91-
@Blocking private ExecutorService blockingExecutor = TestOnlyExecutors.blocking();
91+
@Lightweight private ExecutorService lightweightExecutor = TestOnlyExecutors.lite();
9292
@Background private ExecutorService backgroundExecutor = TestOnlyExecutors.background();
9393
private SignInStorage signInStorage;
9494

@@ -150,7 +150,7 @@ public void setUp() {
150150
mockFirebaseInstallationsProvider,
151151
signInStorage,
152152
mockLifecycleNotifier,
153-
blockingExecutor);
153+
lightweightExecutor);
154154
}
155155

156156
@Test
@@ -199,7 +199,7 @@ public void signInTester_whenChromeAvailable_opensCustomTab() throws Interrupted
199199

200200
testerSignInManager.signInTester();
201201
awaitAsyncOperations(backgroundExecutor);
202-
awaitAsyncOperations(blockingExecutor);
202+
awaitAsyncOperations(lightweightExecutor);
203203

204204
verify(mockFirebaseInstallations, times(1)).getId();
205205
assertThat(shadowActivity.getNextStartedActivity().getData()).isEqualTo(Uri.parse(TEST_URL));
@@ -214,7 +214,7 @@ public void signInTester_whenChromeNotAvailable_opensBrowserIntent() throws Inte
214214

215215
testerSignInManager.signInTester();
216216
awaitAsyncOperations(backgroundExecutor);
217-
awaitAsyncOperations(blockingExecutor);
217+
awaitAsyncOperations(lightweightExecutor);
218218

219219
verify(mockFirebaseInstallations, times(1)).getId();
220220
assertThat(shadowActivity.getNextStartedActivity().getData()).isEqualTo(Uri.parse(TEST_URL));
@@ -227,7 +227,7 @@ public void signInTester_whenSignInCalledMultipleTimes_secondCallHasNoEffect()
227227
testerSignInManager.signInTester();
228228

229229
awaitAsyncOperations(backgroundExecutor);
230-
awaitAsyncOperations(blockingExecutor);
230+
awaitAsyncOperations(lightweightExecutor);
231231

232232
verify(mockFirebaseInstallationsProvider, times(1)).get();
233233
}
@@ -237,7 +237,7 @@ public void signInTester_whenReturnFromSignIn_taskSucceeds()
237237
throws InterruptedException, FirebaseAppDistributionException, ExecutionException {
238238
Task signInTask = testerSignInManager.signInTester();
239239
awaitAsyncOperations(backgroundExecutor);
240-
awaitAsyncOperations(blockingExecutor);
240+
awaitAsyncOperations(lightweightExecutor);
241241

242242
// Simulate re-entering app after successful sign in, via SignInResultActivity
243243
testerSignInManager.onActivityCreated(mockSignInResultActivity);
@@ -255,7 +255,7 @@ public void signInTester_whenStorageFailsToRecordSignInStatus_taskFails()
255255
.thenReturn(Tasks.forException(expectedException));
256256
Task signInTask = testerSignInManager.signInTester();
257257
awaitAsyncOperations(backgroundExecutor);
258-
awaitAsyncOperations(blockingExecutor);
258+
awaitAsyncOperations(lightweightExecutor);
259259

260260
// Simulate re-entering app after successful sign in, via SignInResultActivity
261261
testerSignInManager.onActivityCreated(mockSignInResultActivity);
@@ -267,7 +267,7 @@ public void signInTester_whenStorageFailsToRecordSignInStatus_taskFails()
267267
public void signInTester_whenAppReenteredDuringSignIn_taskFails() throws InterruptedException {
268268
Task signInTask = testerSignInManager.signInTester();
269269
awaitAsyncOperations(backgroundExecutor);
270-
awaitAsyncOperations(blockingExecutor);
270+
awaitAsyncOperations(lightweightExecutor);
271271

272272
// Simulate re-entering app before completing sign in
273273
testerSignInManager.onActivityResumed(activity);

0 commit comments

Comments
 (0)