Skip to content

Commit 98e29b9

Browse files
lfkellogglfkellogg
authored
Remove synchronized blocks from ApkUpdater (#4494)
* Remove synchronized blocks from ApkUpdater * Fix test failure in CI * Use CountDownLatch instead of Thread.sleep to delay functionality under test
1 parent 8a54521 commit 98e29b9

File tree

5 files changed

+223
-74
lines changed

5 files changed

+223
-74
lines changed

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

Lines changed: 35 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,21 @@
1616

1717
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.DOWNLOAD_FAILURE;
1818
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.NETWORK_FAILURE;
19-
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
20-
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;
2119

2220
import android.content.Context;
2321
import android.os.Build.VERSION;
2422
import android.os.Build.VERSION_CODES;
25-
import androidx.annotation.GuardedBy;
2623
import androidx.annotation.NonNull;
2724
import androidx.annotation.VisibleForTesting;
2825
import com.google.android.gms.tasks.Task;
2926
import com.google.firebase.FirebaseApp;
27+
import com.google.firebase.annotations.concurrent.Blocking;
28+
import com.google.firebase.annotations.concurrent.Lightweight;
3029
import com.google.firebase.appdistribution.FirebaseAppDistribution;
3130
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
3231
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
3332
import com.google.firebase.appdistribution.UpdateStatus;
33+
import com.google.firebase.appdistribution.UpdateTask;
3434
import java.io.BufferedOutputStream;
3535
import java.io.File;
3636
import java.io.IOException;
@@ -46,24 +46,23 @@ class ApkUpdater {
4646
private static final String REQUEST_METHOD_GET = "GET";
4747
private static final String DEFAULT_APK_FILE_NAME = "downloaded_release.apk";
4848

49-
private final Executor blockingExecutor; // Executor to run task listeners on a background thread
49+
private final @Blocking Executor blockingExecutor;
50+
private final @Lightweight Executor lightweightExecutor;
5051
private final Context context;
5152
private final ApkInstaller apkInstaller;
5253
private final FirebaseAppDistributionNotificationsManager appDistributionNotificationsManager;
5354
private final HttpsUrlConnectionFactory httpsUrlConnectionFactory;
5455
private final FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier;
55-
56-
@GuardedBy("updateTaskLock")
57-
private UpdateTaskImpl cachedUpdateTask;
58-
59-
private final Object updateTaskLock = new Object();
56+
private UpdateTaskCache cachedUpdateTask;
6057

6158
public ApkUpdater(
6259
@NonNull FirebaseApp firebaseApp,
6360
@NonNull ApkInstaller apkInstaller,
64-
@NonNull Executor blockingExecutor) {
61+
@NonNull @Blocking Executor blockingExecutor,
62+
@NonNull @Lightweight Executor lightweightExecutor) {
6563
this(
6664
blockingExecutor,
65+
lightweightExecutor,
6766
firebaseApp.getApplicationContext(),
6867
apkInstaller,
6968
new FirebaseAppDistributionNotificationsManager(firebaseApp.getApplicationContext()),
@@ -73,57 +72,45 @@ public ApkUpdater(
7372

7473
@VisibleForTesting
7574
public ApkUpdater(
76-
@NonNull Executor blockingExecutor,
75+
@NonNull @Blocking Executor blockingExecutor,
76+
@NonNull @Lightweight Executor lightweightExecutor,
7777
@NonNull Context context,
7878
@NonNull ApkInstaller apkInstaller,
7979
@NonNull FirebaseAppDistributionNotificationsManager appDistributionNotificationsManager,
8080
@NonNull HttpsUrlConnectionFactory httpsUrlConnectionFactory,
8181
@NonNull FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier) {
8282
this.blockingExecutor = blockingExecutor;
83+
this.lightweightExecutor = lightweightExecutor;
8384
this.context = context;
8485
this.apkInstaller = apkInstaller;
8586
this.appDistributionNotificationsManager = appDistributionNotificationsManager;
8687
this.httpsUrlConnectionFactory = httpsUrlConnectionFactory;
8788
this.lifeCycleNotifier = lifeCycleNotifier;
89+
this.cachedUpdateTask = new UpdateTaskCache(lightweightExecutor);
8890
}
8991

90-
UpdateTaskImpl updateApk(
92+
UpdateTask updateApk(
9193
@NonNull AppDistributionReleaseInternal newRelease, boolean showNotification) {
92-
synchronized (updateTaskLock) {
93-
if (cachedUpdateTask != null && !cachedUpdateTask.isComplete()) {
94-
return cachedUpdateTask;
95-
}
96-
97-
cachedUpdateTask = new UpdateTaskImpl();
98-
}
99-
100-
downloadApk(newRelease, showNotification)
101-
.addOnSuccessListener(blockingExecutor, file -> installApk(file, showNotification))
102-
.addOnFailureListener(
103-
blockingExecutor,
104-
e -> {
105-
setUpdateTaskCompletionErrorWithDefault(
106-
e, "Failed to download APK", Status.DOWNLOAD_FAILURE);
107-
});
108-
109-
synchronized (updateTaskLock) {
110-
return cachedUpdateTask;
111-
}
94+
return cachedUpdateTask.getOrCreateUpdateTask(
95+
() -> {
96+
downloadApk(newRelease, showNotification)
97+
.addOnSuccessListener(lightweightExecutor, file -> installApk(file, showNotification))
98+
.addOnFailureListener(
99+
lightweightExecutor,
100+
e ->
101+
setUpdateTaskCompletionErrorWithDefault(
102+
e, "Failed to download APK", Status.DOWNLOAD_FAILURE));
103+
return new UpdateTaskImpl();
104+
});
112105
}
113106

114107
private void installApk(File file, boolean showDownloadNotificationManager) {
115108
lifeCycleNotifier
116109
.applyToForegroundActivityTask(
117110
activity -> apkInstaller.installApk(file.getPath(), activity))
118-
.addOnSuccessListener(
119-
blockingExecutor,
120-
unused -> {
121-
synchronized (updateTaskLock) {
122-
safeSetTaskResult(cachedUpdateTask);
123-
}
124-
})
111+
.addOnSuccessListener(lightweightExecutor, unused -> cachedUpdateTask.setResult())
125112
.addOnFailureListener(
126-
blockingExecutor,
113+
lightweightExecutor,
127114
e -> {
128115
postUpdateProgress(
129116
file.length(),
@@ -261,17 +248,11 @@ private String getApkFileName() {
261248
}
262249
}
263250

264-
private void setUpdateTaskCompletionError(FirebaseAppDistributionException e) {
265-
synchronized (updateTaskLock) {
266-
safeSetTaskException(cachedUpdateTask, e);
267-
}
268-
}
269-
270251
private void setUpdateTaskCompletionErrorWithDefault(Exception e, String message, Status status) {
271252
if (e instanceof FirebaseAppDistributionException) {
272-
setUpdateTaskCompletionError((FirebaseAppDistributionException) e);
253+
cachedUpdateTask.setException((FirebaseAppDistributionException) e);
273254
} else {
274-
setUpdateTaskCompletionError(new FirebaseAppDistributionException(message, status, e));
255+
cachedUpdateTask.setException(new FirebaseAppDistributionException(message, status, e));
275256
}
276257
}
277258

@@ -281,14 +262,12 @@ private void postUpdateProgress(
281262
UpdateStatus status,
282263
boolean showNotification,
283264
int stringResourceId) {
284-
synchronized (updateTaskLock) {
285-
cachedUpdateTask.updateProgress(
286-
UpdateProgressImpl.builder()
287-
.setApkFileTotalBytes(totalBytes)
288-
.setApkBytesDownloaded(downloadedBytes)
289-
.setUpdateStatus(status)
290-
.build());
291-
}
265+
cachedUpdateTask.setProgress(
266+
UpdateProgressImpl.builder()
267+
.setApkFileTotalBytes(totalBytes)
268+
.setApkBytesDownloaded(downloadedBytes)
269+
.setUpdateStatus(status)
270+
.build());
292271
if (showNotification) {
293272
appDistributionNotificationsManager.showAppUpdateNotification(
294273
totalBytes, downloadedBytes, stringResourceId);

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,11 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
121121
firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
122122
new NewReleaseFetcher(
123123
firebaseApp.getApplicationContext(), testerApiClient, releaseIdentifier),
124-
new ApkUpdater(firebaseApp, new ApkInstaller(lightweightExecutor), blockingExecutor),
124+
new ApkUpdater(
125+
firebaseApp,
126+
new ApkInstaller(lightweightExecutor),
127+
blockingExecutor,
128+
lightweightExecutor),
125129
new AabUpdater(blockingExecutor),
126130
signInStorage,
127131
lifecycleNotifier,

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

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
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+
import com.google.android.gms.tasks.TaskCompletionSource;
19+
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
20+
import com.google.firebase.appdistribution.UpdateProgress;
21+
import com.google.firebase.appdistribution.UpdateTask;
22+
import com.google.firebase.concurrent.FirebaseExecutors;
23+
import java.util.concurrent.Executor;
24+
25+
/**
26+
* A cache for an {@link UpdateTaskImpl}, for use in cases where we only ever want one active task
27+
* at a time for a particular operation.
28+
*
29+
* <p>This is equivalent to {@link TaskCompletionSourceCache} but for UpdateTasks.
30+
*/
31+
class UpdateTaskCache {
32+
33+
/** A functional interface for a producer of a new {@link UpdateTaskImpl}. */
34+
@FunctionalInterface
35+
interface UpdateTaskProducer {
36+
37+
/** Produce a new {@link UpdateTaskImpl}. */
38+
UpdateTaskImpl produce();
39+
}
40+
41+
private UpdateTaskImpl cachedUpdateTaskImpl;
42+
private final Executor sequentialExecutor;
43+
44+
/**
45+
* Constructor for a {@link UpdateTaskCache} that controls access using its own sequential
46+
* executor backed by the given base executor.
47+
*
48+
* @param baseExecutor Executor to back the sequential executor, including running any {@link
49+
* UpdateTaskProducer} passed to {@link #getOrCreateUpdateTask}.
50+
*/
51+
UpdateTaskCache(Executor baseExecutor) {
52+
sequentialExecutor = FirebaseExecutors.newSequentialExecutor(baseExecutor);
53+
}
54+
55+
/**
56+
* Gets a cached {@link UpdateTask}, if there is an ongoing one, or else calls the given {@code
57+
* producer} to produce a new {@link UpdateTaskImpl} and caches the return value.
58+
*
59+
* <p>The producer's {@link UpdateTaskProducer#produce} method will be called on the sequential
60+
* executor, to prevent the task from being updated while it's producing the new one.
61+
*/
62+
UpdateTask getOrCreateUpdateTask(UpdateTaskProducer producer) {
63+
UpdateTaskImpl impl = new UpdateTaskImpl();
64+
sequentialExecutor.execute(
65+
() -> {
66+
if (!isOngoing(cachedUpdateTaskImpl)) {
67+
cachedUpdateTaskImpl = producer.produce();
68+
}
69+
impl.shadow(cachedUpdateTaskImpl);
70+
});
71+
return impl;
72+
}
73+
74+
/**
75+
* Sets the exception on the cached {@link UpdateTaskImpl}, if there is one and it is not
76+
* completed.
77+
*/
78+
Task<Void> setProgress(UpdateProgress progress) {
79+
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
80+
sequentialExecutor.execute(
81+
() -> {
82+
if (isOngoing(cachedUpdateTaskImpl)) {
83+
cachedUpdateTaskImpl.updateProgress(progress);
84+
}
85+
});
86+
return taskCompletionSource.getTask();
87+
}
88+
89+
/**
90+
* Sets the result on the cached {@link UpdateTaskImpl}, if there is one and it is not completed.
91+
*/
92+
Task<Void> setResult() {
93+
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
94+
sequentialExecutor.execute(
95+
() -> {
96+
if (isOngoing(cachedUpdateTaskImpl)) {
97+
cachedUpdateTaskImpl.setResult();
98+
}
99+
});
100+
return taskCompletionSource.getTask();
101+
}
102+
103+
/**
104+
* Sets the exception on the cached {@link UpdateTaskImpl}, if there is one and it is not
105+
* completed.
106+
*/
107+
Task<Void> setException(FirebaseAppDistributionException e) {
108+
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
109+
sequentialExecutor.execute(
110+
() -> {
111+
if (isOngoing(cachedUpdateTaskImpl)) {
112+
cachedUpdateTaskImpl.setException(e);
113+
}
114+
});
115+
return taskCompletionSource.getTask();
116+
}
117+
118+
private static boolean isOngoing(UpdateTask updateTask) {
119+
return updateTask != null && !updateTask.isComplete();
120+
}
121+
}

0 commit comments

Comments
 (0)