Skip to content

Commit 80da111

Browse files
lfkellogglfkellogg
committed
Remove some synchronized blocks from FirebaseAppDistributionImpl (#4353)
* Remove some synchronized blocks from FirebaseAppDistributionImpl * Rename UpdateTaskCache.java to TaskCache.java * Add @FunctionalInterface annotation * Reformat a comment
1 parent f79c87e commit 80da111

File tree

2 files changed

+137
-103
lines changed

2 files changed

+137
-103
lines changed

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

Lines changed: 89 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -58,17 +58,13 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
5858
private final AabUpdater aabUpdater;
5959
private final SignInStorage signInStorage;
6060

61-
private final Object updateIfNewReleaseTaskLock = new Object();
62-
63-
@GuardedBy("updateIfNewReleaseTaskLock")
64-
private UpdateTaskImpl cachedUpdateIfNewReleaseTask;
65-
6661
private final Object cachedNewReleaseLock = new Object();
6762

6863
@GuardedBy("cachedNewReleaseLock")
6964
private AppDistributionReleaseInternal cachedNewRelease;
7065

71-
private Task<AppDistributionRelease> cachedCheckForNewReleaseTask;
66+
private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
67+
private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
7268
private AlertDialog updateConfirmationDialog;
7369
private AlertDialog signInConfirmationDialog;
7470
@Nullable private Activity dialogHostActivity = null;
@@ -105,54 +101,57 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
105101
// TODO(b/261014422): Use an explicit executor in continuations.
106102
@SuppressLint("TaskMainThread")
107103
public UpdateTask updateIfNewReleaseAvailable() {
108-
synchronized (updateIfNewReleaseTaskLock) {
109-
if (updateIfNewReleaseAvailableIsTaskInProgress()) {
110-
return cachedUpdateIfNewReleaseTask;
111-
}
112-
cachedUpdateIfNewReleaseTask = new UpdateTaskImpl();
113-
remakeSignInConfirmationDialog = false;
114-
remakeUpdateConfirmationDialog = false;
115-
dialogHostActivity = null;
116-
}
117-
lifecycleNotifier
118-
.applyToForegroundActivityTask(this::showSignInConfirmationDialog)
119-
.onSuccessTask(unused -> signInTester())
120-
.onSuccessTask(unused -> checkForNewRelease())
121-
.continueWithTask(
122-
task -> {
123-
if (!task.isSuccessful()) {
124-
postProgressToCachedUpdateIfNewReleaseTask(
125-
UpdateProgressImpl.builder()
126-
.setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
127-
.setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
128-
.setUpdateStatus(UpdateStatus.NEW_RELEASE_CHECK_FAILED)
129-
.build());
130-
}
131-
// if the task failed, this get() will cause the error to propagate to the handler
132-
// below
133-
AppDistributionRelease release = task.getResult();
134-
if (release == null) {
135-
postProgressToCachedUpdateIfNewReleaseTask(
136-
UpdateProgressImpl.builder()
137-
.setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
138-
.setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
139-
.setUpdateStatus(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE)
140-
.build());
141-
setCachedUpdateIfNewReleaseResult();
142-
return Tasks.forResult(null);
143-
}
144-
return lifecycleNotifier.applyToForegroundActivityTask(
145-
activity -> showUpdateConfirmationDialog(activity, release));
146-
})
147-
.onSuccessTask(
148-
unused ->
149-
updateApp(true)
150-
.addOnProgressListener(this::postProgressToCachedUpdateIfNewReleaseTask))
151-
.addOnFailureListener(this::setCachedUpdateIfNewReleaseCompletionError);
152-
153-
synchronized (updateIfNewReleaseTaskLock) {
154-
return cachedUpdateIfNewReleaseTask;
155-
}
104+
return updateIfNewReleaseAvailableTaskCache.getOrCreateTask(
105+
() -> {
106+
UpdateTaskImpl updateTask = new UpdateTaskImpl();
107+
remakeSignInConfirmationDialog = false;
108+
remakeUpdateConfirmationDialog = false;
109+
dialogHostActivity = null;
110+
111+
lifecycleNotifier
112+
.applyToForegroundActivityTask(this::showSignInConfirmationDialog)
113+
.onSuccessTask(unused -> signInTester())
114+
.onSuccessTask(unused -> checkForNewRelease())
115+
.continueWithTask(
116+
task -> {
117+
if (!task.isSuccessful()) {
118+
postProgressToCachedUpdateIfNewReleaseTask(
119+
updateTask,
120+
UpdateProgressImpl.builder()
121+
.setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
122+
.setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
123+
.setUpdateStatus(UpdateStatus.NEW_RELEASE_CHECK_FAILED)
124+
.build());
125+
}
126+
// if the task failed, this get() will cause the error to propagate to the
127+
// handler below
128+
AppDistributionRelease release = task.getResult();
129+
if (release == null) {
130+
postProgressToCachedUpdateIfNewReleaseTask(
131+
updateTask,
132+
UpdateProgressImpl.builder()
133+
.setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
134+
.setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
135+
.setUpdateStatus(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE)
136+
.build());
137+
setCachedUpdateIfNewReleaseResult(updateTask);
138+
return Tasks.forResult(null);
139+
}
140+
return lifecycleNotifier.applyToForegroundActivityTask(
141+
activity -> showUpdateConfirmationDialog(activity, release));
142+
})
143+
.onSuccessTask(
144+
unused ->
145+
updateApp(true)
146+
.addOnProgressListener(
147+
updateProgress ->
148+
postProgressToCachedUpdateIfNewReleaseTask(
149+
updateTask, updateProgress)))
150+
.addOnFailureListener(
151+
exception -> setCachedUpdateIfNewReleaseCompletionError(updateTask, exception));
152+
153+
return updateTask;
154+
});
156155
}
157156

158157
@NonNull
@@ -224,37 +223,35 @@ public void signOutTester() {
224223
// TODO(b/261014422): Use an explicit executor in continuations.
225224
@SuppressLint("TaskMainThread")
226225
public synchronized Task<AppDistributionRelease> checkForNewRelease() {
227-
if (cachedCheckForNewReleaseTask != null && !cachedCheckForNewReleaseTask.isComplete()) {
228-
LogWrapper.getInstance().v("Response in progress");
229-
return cachedCheckForNewReleaseTask;
230-
}
231-
if (!isTesterSignedIn()) {
232-
return Tasks.forException(
233-
new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
234-
}
226+
return checkForNewReleaseTaskCache.getOrCreateTask(
227+
() -> {
228+
if (!isTesterSignedIn()) {
229+
return Tasks.forException(
230+
new FirebaseAppDistributionException(
231+
"Tester is not signed in", AUTHENTICATION_FAILURE));
232+
}
235233

236-
cachedCheckForNewReleaseTask =
237-
newReleaseFetcher
238-
.checkForNewRelease()
239-
.onSuccessTask(
240-
appDistributionReleaseInternal -> {
241-
setCachedNewRelease(appDistributionReleaseInternal);
242-
return Tasks.forResult(
243-
ReleaseUtils.convertToAppDistributionRelease(appDistributionReleaseInternal));
244-
})
245-
.addOnFailureListener(
246-
e -> {
247-
if (e instanceof FirebaseAppDistributionException
248-
&& ((FirebaseAppDistributionException) e).getErrorCode()
249-
== AUTHENTICATION_FAILURE) {
250-
// If CheckForNewRelease returns authentication error, the FID is no longer
251-
// valid or does not have access to the latest release. So sign out the tester
252-
// to force FID re-registration
253-
signOutTester();
254-
}
255-
});
256-
257-
return cachedCheckForNewReleaseTask;
234+
return newReleaseFetcher
235+
.checkForNewRelease()
236+
.onSuccessTask(
237+
appDistributionReleaseInternal -> {
238+
setCachedNewRelease(appDistributionReleaseInternal);
239+
return Tasks.forResult(
240+
ReleaseUtils.convertToAppDistributionRelease(
241+
appDistributionReleaseInternal));
242+
})
243+
.addOnFailureListener(
244+
e -> {
245+
if (e instanceof FirebaseAppDistributionException
246+
&& ((FirebaseAppDistributionException) e).getErrorCode()
247+
== AUTHENTICATION_FAILURE) {
248+
// If CheckForNewRelease returns authentication error, the FID is no longer
249+
// valid or does not have access to the latest release. So sign out the tester
250+
// to force FID re-registration
251+
signOutTester();
252+
}
253+
});
254+
});
258255
}
259256

260257
@Override
@@ -411,25 +408,20 @@ private Task<Void> showUpdateConfirmationDialog(
411408
return showUpdateDialogTask.getTask();
412409
}
413410

414-
private void setCachedUpdateIfNewReleaseCompletionError(Exception e) {
415-
synchronized (updateIfNewReleaseTaskLock) {
416-
safeSetTaskException(cachedUpdateIfNewReleaseTask, e);
417-
}
411+
private void setCachedUpdateIfNewReleaseCompletionError(UpdateTaskImpl updateTask, Exception e) {
412+
safeSetTaskException(updateTask, e);
418413
dismissDialogs();
419414
}
420415

421-
private void postProgressToCachedUpdateIfNewReleaseTask(UpdateProgress progress) {
422-
synchronized (updateIfNewReleaseTaskLock) {
423-
if (cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete()) {
424-
cachedUpdateIfNewReleaseTask.updateProgress(progress);
425-
}
416+
private void postProgressToCachedUpdateIfNewReleaseTask(
417+
UpdateTaskImpl updateTask, UpdateProgress progress) {
418+
if (updateTask != null && !updateTask.isComplete()) {
419+
updateTask.updateProgress(progress);
426420
}
427421
}
428422

429-
private void setCachedUpdateIfNewReleaseResult() {
430-
synchronized (updateIfNewReleaseTaskLock) {
431-
safeSetTaskResult(cachedUpdateIfNewReleaseTask);
432-
}
423+
private void setCachedUpdateIfNewReleaseResult(UpdateTaskImpl updateTask) {
424+
safeSetTaskResult(updateTask);
433425
dismissDialogs();
434426
}
435427

@@ -448,12 +440,6 @@ private UpdateTaskImpl getErrorUpdateTask(Exception e) {
448440
return updateTask;
449441
}
450442

451-
private boolean updateIfNewReleaseAvailableIsTaskInProgress() {
452-
synchronized (updateIfNewReleaseTaskLock) {
453-
return cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete();
454-
}
455-
}
456-
457443
private boolean awaitingSignInDialogConfirmation() {
458444
return (showSignInDialogTask != null
459445
&& !showSignInDialogTask.getTask().isComplete()

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// Copyright 2022 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.firebase.appdistribution.impl;
16+
17+
import com.google.android.gms.tasks.Task;
18+
19+
/**
20+
* A cache for Tasks, for use in cases where we only ever want one active task at a time for a
21+
* particular operation.
22+
*/
23+
class TaskCache<T extends Task> {
24+
25+
/** A functional interface for a producer of a new Task. */
26+
@FunctionalInterface
27+
interface TaskProducer<T extends Task> {
28+
29+
/** Produce a new Task. */
30+
T produce();
31+
}
32+
33+
private T cachedTask;
34+
35+
/**
36+
* Gets a cached task, if there is one and it is not completed, or else calls the given {@code
37+
* producer} and caches the returned task.
38+
*
39+
* @return the cached task if there is one and it is not completed, or else the result from {@code
40+
* producer.produce()}
41+
*/
42+
synchronized T getOrCreateTask(TaskProducer<T> producer) {
43+
if (cachedTask == null || cachedTask.isComplete()) {
44+
cachedTask = producer.produce();
45+
}
46+
return cachedTask;
47+
}
48+
}

0 commit comments

Comments
 (0)