Skip to content

Commit bc86a6f

Browse files
lfkellogglfkellogg
committed
Remove some synchronized blocks from FirebaseAppDistributionImpl
1 parent abdd518 commit bc86a6f

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: 90 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,58 @@ 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
125+
// below
126+
AppDistributionRelease release = task.getResult();
127+
if (release == null) {
128+
postProgressToCachedUpdateIfNewReleaseTask(
129+
updateTask,
130+
UpdateProgressImpl.builder()
131+
.setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
132+
.setApkBytesDownloaded(UNKNOWN_RELEASE_FILE_SIZE)
133+
.setUpdateStatus(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE)
134+
.build());
135+
setCachedUpdateIfNewReleaseResult(updateTask);
136+
return Tasks.forResult(null);
137+
}
138+
return lifecycleNotifier.applyToForegroundActivityTask(
139+
activity -> showUpdateConfirmationDialog(activity, release));
140+
})
141+
.onSuccessTask(
142+
unused ->
143+
updateApp(true)
144+
.addOnProgressListener(
145+
updateProgress ->
146+
postProgressToCachedUpdateIfNewReleaseTask(
147+
updateTask, updateProgress)))
148+
.addOnFailureListener(
149+
exception -> setCachedUpdateIfNewReleaseCompletionError(updateTask, exception));
150+
151+
return updateTask;
152+
});
154153
}
155154

156155
@NonNull
@@ -220,37 +219,35 @@ public void signOutTester() {
220219
@Override
221220
@NonNull
222221
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-
}
222+
return checkForNewReleaseTaskCache.getOrCreateTask(
223+
() -> {
224+
if (!isTesterSignedIn()) {
225+
return Tasks.forException(
226+
new FirebaseAppDistributionException(
227+
"Tester is not signed in", AUTHENTICATION_FAILURE));
228+
}
231229

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

256253
@Override
@@ -407,25 +404,20 @@ private Task<Void> showUpdateConfirmationDialog(
407404
return showUpdateDialogTask.getTask();
408405
}
409406

410-
private void setCachedUpdateIfNewReleaseCompletionError(Exception e) {
411-
synchronized (updateIfNewReleaseTaskLock) {
412-
safeSetTaskException(cachedUpdateIfNewReleaseTask, e);
413-
}
407+
private void setCachedUpdateIfNewReleaseCompletionError(UpdateTaskImpl updateTask, Exception e) {
408+
safeSetTaskException(updateTask, e);
414409
dismissDialogs();
415410
}
416411

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

425-
private void setCachedUpdateIfNewReleaseResult() {
426-
synchronized (updateIfNewReleaseTaskLock) {
427-
safeSetTaskResult(cachedUpdateIfNewReleaseTask);
428-
}
419+
private void setCachedUpdateIfNewReleaseResult(UpdateTaskImpl updateTask) {
420+
safeSetTaskResult(updateTask);
429421
dismissDialogs();
430422
}
431423

@@ -444,12 +436,6 @@ private UpdateTaskImpl getErrorUpdateTask(Exception e) {
444436
return updateTask;
445437
}
446438

447-
private boolean updateIfNewReleaseAvailableIsTaskInProgress() {
448-
synchronized (updateIfNewReleaseTaskLock) {
449-
return cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete();
450-
}
451-
}
452-
453439
private boolean awaitingSignInDialogConfirmation() {
454440
return (showSignInDialogTask != null
455441
&& !showSignInDialogTask.getTask().isComplete()

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
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+
interface TaskProducer<T extends Task> {
27+
28+
/** Produce a new Task. */
29+
T produce();
30+
}
31+
32+
private T cachedTask;
33+
34+
/**
35+
* Gets a cached task, if there is one and it is not completed, or else calls the given {@code
36+
* producer} and caches the returned task.
37+
*
38+
* @return the cached task if there is one and it is not completed, or else the result from {@code
39+
* producer.produce()}
40+
*/
41+
synchronized T getOrCreateTask(TaskProducer<T> producer) {
42+
if (cachedTask == null || cachedTask.isComplete()) {
43+
cachedTask = producer.produce();
44+
}
45+
return cachedTask;
46+
}
47+
}

0 commit comments

Comments
 (0)