Skip to content

Commit ddee930

Browse files
lfkellogglfkellogg
authored
Remove synchronized blocks from AabUpdater (#4522)
1 parent 7505b3a commit ddee930

File tree

4 files changed

+128
-156
lines changed

4 files changed

+128
-156
lines changed

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

Lines changed: 55 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,18 @@
1717
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.DOWNLOAD_FAILURE;
1818
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.NETWORK_FAILURE;
1919
import static com.google.firebase.appdistribution.impl.TaskUtils.runAsyncInTask;
20-
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
2120

2221
import android.app.Activity;
2322
import android.content.Intent;
2423
import android.net.Uri;
25-
import androidx.annotation.GuardedBy;
2624
import androidx.annotation.NonNull;
2725
import androidx.annotation.VisibleForTesting;
26+
import com.google.firebase.annotations.concurrent.Blocking;
27+
import com.google.firebase.annotations.concurrent.Lightweight;
2828
import com.google.firebase.appdistribution.FirebaseAppDistribution;
2929
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
3030
import com.google.firebase.appdistribution.UpdateStatus;
31+
import com.google.firebase.appdistribution.UpdateTask;
3132
import java.io.IOException;
3233
import java.util.concurrent.Executor;
3334
import javax.net.ssl.HttpsURLConnection;
@@ -38,71 +39,77 @@ class AabUpdater {
3839

3940
private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
4041
private final HttpsUrlConnectionFactory httpsUrlConnectionFactory;
41-
private final Executor blockingExecutor;
42+
@Blocking private final Executor blockingExecutor;
43+
@Lightweight private final Executor lightweightExecutor;
44+
private final UpdateTaskCache updateTaskCache;
4245

43-
private final Object updateAabLock = new Object();
44-
45-
@GuardedBy("updateAabLock")
46-
private UpdateTaskImpl cachedUpdateTask;
47-
48-
@GuardedBy("updateAabLock")
4946
private AppDistributionReleaseInternal aabReleaseInProgress;
50-
51-
@GuardedBy("updateAabLock")
5247
private boolean hasBeenSentToPlayForCurrentTask = false;
5348

54-
AabUpdater(@NonNull Executor blockingExecutor) {
49+
AabUpdater(
50+
@NonNull @Blocking Executor blockingExecutor,
51+
@NonNull @Lightweight Executor lightweightExecutor) {
5552
this(
5653
FirebaseAppDistributionLifecycleNotifier.getInstance(),
5754
new HttpsUrlConnectionFactory(),
58-
blockingExecutor);
55+
blockingExecutor,
56+
lightweightExecutor);
5957
}
6058

6159
AabUpdater(
6260
@NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
6361
@NonNull HttpsUrlConnectionFactory httpsUrlConnectionFactory,
64-
@NonNull Executor blockingExecutor) {
62+
@NonNull @Blocking Executor blockingExecutor,
63+
@NonNull @Lightweight Executor lightweightExecutor) {
6564
this.lifecycleNotifier = lifecycleNotifier;
6665
this.httpsUrlConnectionFactory = httpsUrlConnectionFactory;
6766
lifecycleNotifier.addOnActivityStartedListener(this::onActivityStarted);
6867
this.blockingExecutor = blockingExecutor;
68+
this.lightweightExecutor = lightweightExecutor;
69+
this.updateTaskCache = new UpdateTaskCache(lightweightExecutor);
6970
}
7071

7172
@VisibleForTesting
7273
void onActivityStarted(Activity activity) {
7374
if (activity instanceof SignInResultActivity || activity instanceof InstallActivity) {
7475
return;
7576
}
76-
// If app resumes and update is in progress, assume that installation didn't happen and cancel
77-
// the task
78-
synchronized (updateAabLock) {
79-
if (awaitingUpdateFromPlay()) {
80-
this.tryCancelAabUpdateTask();
81-
}
77+
// If app resumes and update is in progress, assume that installation didn't happen and fail the
78+
// task
79+
if (hasBeenSentToPlayForCurrentTask) {
80+
updateTaskCache
81+
.setException(
82+
new FirebaseAppDistributionException(
83+
ErrorMessages.UPDATE_CANCELED,
84+
FirebaseAppDistributionException.Status.INSTALLATION_CANCELED,
85+
ReleaseUtils.convertToAppDistributionRelease(aabReleaseInProgress)))
86+
.addOnSuccessListener(
87+
lightweightExecutor,
88+
unused -> LogWrapper.e(TAG, "App Resumed without update completing."));
8289
}
8390
}
8491

85-
UpdateTaskImpl updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
86-
synchronized (updateAabLock) {
87-
if (cachedUpdateTask != null && !cachedUpdateTask.isComplete()) {
88-
return cachedUpdateTask;
89-
}
90-
91-
cachedUpdateTask = new UpdateTaskImpl();
92-
aabReleaseInProgress = newRelease;
93-
hasBeenSentToPlayForCurrentTask = false;
94-
95-
// On a background thread, fetch the redirect URL and open it in the Play app
96-
runAsyncInTask(blockingExecutor, () -> fetchDownloadRedirectUrl(newRelease.getDownloadUrl()))
97-
.onSuccessTask(
98-
blockingExecutor,
99-
redirectUrl ->
100-
lifecycleNotifier.consumeForegroundActivity(
101-
activity -> openRedirectUrlInPlay(redirectUrl, activity)))
102-
.addOnFailureListener(blockingExecutor, this::setUpdateTaskCompletionError);
103-
104-
return cachedUpdateTask;
105-
}
92+
UpdateTask updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
93+
return updateTaskCache.getOrCreateUpdateTask(
94+
() -> {
95+
UpdateTaskImpl cachedUpdateTask = new UpdateTaskImpl();
96+
aabReleaseInProgress = newRelease;
97+
hasBeenSentToPlayForCurrentTask = false;
98+
99+
// On a background thread, fetch the redirect URL and open it in the Play app
100+
runAsyncInTask(
101+
blockingExecutor, () -> fetchDownloadRedirectUrl(newRelease.getDownloadUrl()))
102+
.onSuccessTask(
103+
lightweightExecutor,
104+
redirectUrl ->
105+
lifecycleNotifier.consumeForegroundActivity(
106+
activity -> openRedirectUrlInPlay(redirectUrl, activity)))
107+
.addOnFailureListener(
108+
lightweightExecutor,
109+
e -> cachedUpdateTask.setException(FirebaseAppDistributionExceptions.wrap(e)));
110+
111+
return cachedUpdateTask;
112+
});
106113
}
107114

108115
private String fetchDownloadRedirectUrl(String downloadUrl)
@@ -158,39 +165,12 @@ private void openRedirectUrlInPlay(String redirectUrl, Activity hostActivity) {
158165
// scenario where the activity moves to the background while we're awaiting the lock.
159166
hostActivity.startActivity(updateIntent);
160167

161-
synchronized (updateAabLock) {
162-
cachedUpdateTask.updateProgress(
163-
UpdateProgressImpl.builder()
164-
.setApkBytesDownloaded(-1)
165-
.setApkFileTotalBytes(-1)
166-
.setUpdateStatus(UpdateStatus.REDIRECTED_TO_PLAY)
167-
.build());
168-
hasBeenSentToPlayForCurrentTask = true;
169-
}
170-
}
171-
172-
private void setUpdateTaskCompletionError(Exception e) {
173-
synchronized (updateAabLock) {
174-
safeSetTaskException(cachedUpdateTask, FirebaseAppDistributionExceptions.wrap(e));
175-
}
176-
}
177-
178-
void tryCancelAabUpdateTask() {
179-
synchronized (updateAabLock) {
180-
safeSetTaskException(
181-
cachedUpdateTask,
182-
new FirebaseAppDistributionException(
183-
ErrorMessages.UPDATE_CANCELED,
184-
FirebaseAppDistributionException.Status.INSTALLATION_CANCELED,
185-
ReleaseUtils.convertToAppDistributionRelease(aabReleaseInProgress)));
186-
}
187-
}
188-
189-
private boolean awaitingUpdateFromPlay() {
190-
synchronized (updateAabLock) {
191-
return cachedUpdateTask != null
192-
&& !cachedUpdateTask.isComplete()
193-
&& hasBeenSentToPlayForCurrentTask;
194-
}
168+
updateTaskCache.setProgress(
169+
UpdateProgressImpl.builder()
170+
.setApkBytesDownloaded(-1)
171+
.setApkFileTotalBytes(-1)
172+
.setUpdateStatus(UpdateStatus.REDIRECTED_TO_PLAY)
173+
.build());
174+
hasBeenSentToPlayForCurrentTask = true;
195175
}
196176
}

‎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
@@ -129,7 +129,7 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
129129
new ApkInstaller(lightweightExecutor),
130130
blockingExecutor,
131131
lightweightExecutor),
132-
new AabUpdater(blockingExecutor),
132+
new AabUpdater(blockingExecutor, lightweightExecutor),
133133
signInStorage,
134134
lifecycleNotifier,
135135
releaseIdentifier,

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

Lines changed: 72 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,35 @@
1616
import static com.google.common.truth.Truth.assertThat;
1717
import static com.google.firebase.appdistribution.impl.TestUtils.assertTaskFailure;
1818
import static com.google.firebase.appdistribution.impl.TestUtils.awaitAsyncOperations;
19+
import static com.google.firebase.appdistribution.impl.TestUtils.awaitCondition;
1920
import static org.junit.Assert.assertEquals;
21+
import static org.mockito.ArgumentMatchers.anyString;
22+
import static org.mockito.Mockito.times;
23+
import static org.mockito.Mockito.verify;
2024
import static org.mockito.Mockito.when;
2125
import static org.robolectric.Shadows.shadowOf;
2226
import static org.robolectric.annotation.LooperMode.Mode.PAUSED;
2327

2428
import android.app.Activity;
2529
import android.net.Uri;
2630
import com.google.firebase.FirebaseApp;
31+
import com.google.firebase.annotations.concurrent.Blocking;
32+
import com.google.firebase.annotations.concurrent.Lightweight;
2733
import com.google.firebase.appdistribution.BinaryType;
2834
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
2935
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
3036
import com.google.firebase.appdistribution.UpdateProgress;
3137
import com.google.firebase.appdistribution.UpdateStatus;
3238
import com.google.firebase.appdistribution.UpdateTask;
39+
import com.google.firebase.concurrent.FirebaseExecutors;
40+
import com.google.firebase.concurrent.TestOnlyExecutors;
3341
import java.io.ByteArrayInputStream;
3442
import java.io.IOException;
43+
import java.util.ArrayList;
3544
import java.util.List;
45+
import java.util.concurrent.CountDownLatch;
46+
import java.util.concurrent.ExecutionException;
3647
import java.util.concurrent.ExecutorService;
37-
import java.util.concurrent.Executors;
3848
import javax.net.ssl.HttpsURLConnection;
3949
import org.junit.Before;
4050
import org.junit.Test;
@@ -62,9 +72,11 @@ public class AabUpdaterTest {
6272
.setDownloadUrl("https://test-url")
6373
.build();
6474

75+
@Blocking private final ExecutorService blockingExecutor = TestOnlyExecutors.blocking();
76+
@Lightweight private final ExecutorService lightweightExecutor = TestOnlyExecutors.lite();
77+
6578
private AabUpdater aabUpdater;
6679
private ShadowActivity shadowActivity;
67-
private ExecutorService testExecutor;
6880
@Mock private HttpsURLConnection mockHttpsUrlConnection;
6981
@Mock private HttpsUrlConnectionFactory mockHttpsUrlConnectionFactory;
7082
@Mock private FirebaseAppDistributionLifecycleNotifier mockLifecycleNotifier;
@@ -78,7 +90,6 @@ public void setup() throws IOException, FirebaseAppDistributionException {
7890

7991
FirebaseApp.clearInstancesForTest();
8092

81-
testExecutor = Executors.newSingleThreadExecutor();
8293
activity = Robolectric.buildActivity(TestActivity.class).create().get();
8394
shadowActivity = shadowOf(activity);
8495

@@ -90,7 +101,11 @@ public void setup() throws IOException, FirebaseAppDistributionException {
90101

91102
aabUpdater =
92103
Mockito.spy(
93-
new AabUpdater(mockLifecycleNotifier, mockHttpsUrlConnectionFactory, testExecutor));
104+
new AabUpdater(
105+
mockLifecycleNotifier,
106+
mockHttpsUrlConnectionFactory,
107+
blockingExecutor,
108+
lightweightExecutor));
94109

95110
TestUtils.mockForegroundActivity(mockLifecycleNotifier, activity);
96111
}
@@ -102,7 +117,8 @@ public void updateAppTask_whenOpenConnectionFails_setsNetworkFailure()
102117
when(mockHttpsUrlConnectionFactory.openConnection(TEST_URL)).thenThrow(caughtException);
103118

104119
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
105-
awaitAsyncOperations(testExecutor);
120+
awaitAsyncOperations(blockingExecutor);
121+
awaitAsyncOperations(lightweightExecutor);
106122

107123
assertTaskFailure(
108124
updateTask, Status.NETWORK_FAILURE, "Failed to open connection", caughtException);
@@ -114,7 +130,8 @@ public void updateAppTask_isNotRedirectResponse_setsDownloadFailure()
114130
when(mockHttpsUrlConnection.getResponseCode()).thenReturn(200);
115131

116132
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
117-
awaitAsyncOperations(testExecutor);
133+
awaitAsyncOperations(blockingExecutor);
134+
awaitAsyncOperations(lightweightExecutor);
118135

119136
assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "Expected redirect");
120137
}
@@ -125,7 +142,8 @@ public void updateAppTask_missingLocationHeader_setsDownloadFailure()
125142
when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn(null);
126143

127144
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
128-
awaitAsyncOperations(testExecutor);
145+
awaitAsyncOperations(blockingExecutor);
146+
awaitAsyncOperations(lightweightExecutor);
129147

130148
assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "No Location header");
131149
}
@@ -135,18 +153,26 @@ public void updateAppTask_emptyLocationHeader_setsDownloadFailure() throws Inter
135153
when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn("");
136154

137155
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
138-
awaitAsyncOperations(testExecutor);
156+
awaitAsyncOperations(blockingExecutor);
157+
awaitAsyncOperations(lightweightExecutor);
139158

140159
assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "Empty Location header");
141160
}
142161

143162
@Test
144163
public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Exception {
145-
TestOnProgressListener listener = TestOnProgressListener.withExpectedCount(1);
164+
// Block thread actually making the request on a latch, which gives us time to add listeners to
165+
// the returned UpdateTask in time to get all the progress updates
166+
CountDownLatch countDownLatch = mockOpenConnectionWithLatch();
167+
168+
// Start update
146169
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
147-
updateTask.addOnProgressListener(testExecutor, listener);
148170

149-
List<UpdateProgress> progressEvents = listener.await();
171+
// Listen for progress events
172+
List<UpdateProgress> progressEvents = new ArrayList<>();
173+
updateTask.addOnProgressListener(FirebaseExecutors.directExecutor(), progressEvents::add);
174+
countDownLatch.countDown();
175+
awaitCondition(() -> progressEvents.size() == 1);
150176

151177
// Task is not completed in this case, because app is expected to terminate during update
152178
assertThat(shadowActivity.getNextStartedActivity().getData())
@@ -164,7 +190,8 @@ public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Excep
164190
@Test
165191
public void updateAppTask_onAppResume_setsUpdateCancelled() throws InterruptedException {
166192
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
167-
awaitAsyncOperations(testExecutor);
193+
awaitAsyncOperations(blockingExecutor);
194+
awaitAsyncOperations(lightweightExecutor);
168195
aabUpdater.onActivityStarted(activity);
169196

170197
FirebaseAppDistributionException exception =
@@ -175,10 +202,41 @@ public void updateAppTask_onAppResume_setsUpdateCancelled() throws InterruptedEx
175202
}
176203

177204
@Test
178-
public void updateApp_whenCalledMultipleTimesWithAAB_returnsSameUpdateTask() {
205+
public void updateApp_whenCalledMultipleTimesWithAAB_onlyMakesOneRequest()
206+
throws IOException, FirebaseAppDistributionException, ExecutionException,
207+
InterruptedException {
208+
// Block thread actually making the request on a latch, which gives us time to add listeners to
209+
// the returned UpdateTask in time to get all the progress updates
210+
CountDownLatch countDownLatch = mockOpenConnectionWithLatch();
211+
212+
// Start update twice
179213
UpdateTask updateTask1 = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
180214
UpdateTask updateTask2 = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
181215

182-
assertEquals(updateTask1, updateTask2);
216+
// Listen for progress events
217+
List<UpdateProgress> progressEvents1 = new ArrayList<>();
218+
updateTask1.addOnProgressListener(FirebaseExecutors.directExecutor(), progressEvents1::add);
219+
List<UpdateProgress> progressEvents2 = new ArrayList<>();
220+
updateTask2.addOnProgressListener(FirebaseExecutors.directExecutor(), progressEvents2::add);
221+
countDownLatch.countDown();
222+
awaitCondition(() -> progressEvents1.size() == 1);
223+
awaitCondition(() -> progressEvents2.size() == 1);
224+
225+
verify(mockHttpsUrlConnectionFactory, times(1)).openConnection(anyString());
226+
}
227+
228+
private CountDownLatch mockOpenConnectionWithLatch() throws IOException {
229+
CountDownLatch countDownLatch = new CountDownLatch(1);
230+
when(mockHttpsUrlConnectionFactory.openConnection(TEST_URL))
231+
.thenAnswer(
232+
invocation -> {
233+
try {
234+
countDownLatch.await();
235+
} catch (InterruptedException e) {
236+
throw new AssertionError("Interrupted while waiting in mock");
237+
}
238+
return mockHttpsUrlConnection;
239+
});
240+
return countDownLatch;
183241
}
184242
}

0 commit comments

Comments
 (0)