Skip to content

Commit 1ae9488

Browse files
lfkellogglfkellogg
authored
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 abdd518 commit 1ae9488

File tree

2 files changed

+137
-104
lines changed

2 files changed

+137
-104
lines changed

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

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

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

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

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

156154
@NonNull
@@ -220,37 +218,35 @@ public void signOutTester() {
220218
@Override
221219
@NonNull
222220
public synchronized Task<AppDistributionRelease> checkForNewRelease() {
223-
if (cachedCheckForNewReleaseTask != null && !cachedCheckForNewReleaseTask.isComplete()) {
224-
LogWrapper.getInstance().v("Response in progress");
225-
return cachedCheckForNewReleaseTask;
226-
}
227-
if (!isTesterSignedIn()) {
228-
return Tasks.forException(
229-
new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
230-
}
221+
return checkForNewReleaseTaskCache.getOrCreateTask(
222+
() -> {
223+
if (!isTesterSignedIn()) {
224+
return Tasks.forException(
225+
new FirebaseAppDistributionException(
226+
"Tester is not signed in", AUTHENTICATION_FAILURE));
227+
}
231228

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

256252
@Override
@@ -407,25 +403,20 @@ private Task<Void> showUpdateConfirmationDialog(
407403
return showUpdateDialogTask.getTask();
408404
}
409405

410-
private void setCachedUpdateIfNewReleaseCompletionError(Exception e) {
411-
synchronized (updateIfNewReleaseTaskLock) {
412-
safeSetTaskException(cachedUpdateIfNewReleaseTask, e);
413-
}
406+
private void setCachedUpdateIfNewReleaseCompletionError(UpdateTaskImpl updateTask, Exception e) {
407+
safeSetTaskException(updateTask, e);
414408
dismissDialogs();
415409
}
416410

417-
private void postProgressToCachedUpdateIfNewReleaseTask(UpdateProgress progress) {
418-
synchronized (updateIfNewReleaseTaskLock) {
419-
if (cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete()) {
420-
cachedUpdateIfNewReleaseTask.updateProgress(progress);
421-
}
411+
private void postProgressToCachedUpdateIfNewReleaseTask(
412+
UpdateTaskImpl updateTask, UpdateProgress progress) {
413+
if (updateTask != null && !updateTask.isComplete()) {
414+
updateTask.updateProgress(progress);
422415
}
423416
}
424417

425-
private void setCachedUpdateIfNewReleaseResult() {
426-
synchronized (updateIfNewReleaseTaskLock) {
427-
safeSetTaskResult(cachedUpdateIfNewReleaseTask);
428-
}
418+
private void setCachedUpdateIfNewReleaseResult(UpdateTaskImpl updateTask) {
419+
safeSetTaskResult(updateTask);
429420
dismissDialogs();
430421
}
431422

@@ -444,12 +435,6 @@ private UpdateTaskImpl getErrorUpdateTask(Exception e) {
444435
return updateTask;
445436
}
446437

447-
private boolean updateIfNewReleaseAvailableIsTaskInProgress() {
448-
synchronized (updateIfNewReleaseTaskLock) {
449-
return cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete();
450-
}
451-
}
452-
453438
private boolean awaitingSignInDialogConfirmation() {
454439
return (showSignInDialogTask != null
455440
&& !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)