Skip to content

Commit aafced0

Browse files
lfkellogglfkellogg
authored
Allow AAB install to be started from a background thread (#3348)
* Allow AAB install to be started from a background thread * Address Manny's feedback
1 parent dd75b6c commit aafced0

File tree

6 files changed

+89
-62
lines changed

6 files changed

+89
-62
lines changed

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

Lines changed: 52 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,11 @@
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.TaskUtils.combineWithResultOf;
20+
import static com.google.firebase.appdistribution.TaskUtils.runAsyncInTask;
1921
import static com.google.firebase.appdistribution.TaskUtils.safeSetTaskException;
2022

2123
import android.app.Activity;
22-
import android.content.Context;
2324
import android.content.Intent;
2425
import android.net.Uri;
2526
import androidx.annotation.GuardedBy;
@@ -42,29 +43,28 @@ class AabUpdater {
4243
private final HttpsUrlConnectionFactory httpsUrlConnectionFactory;
4344
private final Executor executor;
4445

46+
private final Object updateAabLock = new Object();
47+
4548
@GuardedBy("updateAabLock")
4649
private UpdateTaskImpl cachedUpdateTask;
4750

4851
@GuardedBy("updateAabLock")
4952
private AppDistributionReleaseInternal aabReleaseInProgress;
5053

51-
private final Object updateAabLock = new Object();
52-
private final Context context;
54+
@GuardedBy("updateAabLock")
55+
private boolean hasBeenSentToPlayForCurrentTask = false;
5356

54-
AabUpdater(@NonNull Context context) {
57+
AabUpdater() {
5558
this(
56-
context,
5759
FirebaseAppDistributionLifecycleNotifier.getInstance(),
5860
new HttpsUrlConnectionFactory(),
5961
Executors.newSingleThreadExecutor());
6062
}
6163

6264
AabUpdater(
63-
@NonNull Context context,
6465
@NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
6566
@NonNull HttpsUrlConnectionFactory httpsUrlConnectionFactory,
6667
@NonNull Executor executor) {
67-
this.context = context;
6868
this.lifecycleNotifier = lifecycleNotifier;
6969
this.httpsUrlConnectionFactory = httpsUrlConnectionFactory;
7070
lifecycleNotifier.addOnActivityStartedListener(this::onActivityStarted);
@@ -76,9 +76,13 @@ void onActivityStarted(Activity activity) {
7676
if (activity instanceof SignInResultActivity || activity instanceof InstallActivity) {
7777
return;
7878
}
79-
// If app resumes and aab update task is in progress, assume that installation didn't happen so
80-
// cancel the task
81-
this.tryCancelAabUpdateTask();
79+
// If app resumes and update is in progress, assume that installation didn't happen and cancel
80+
// the task
81+
synchronized (updateAabLock) {
82+
if (awaitingUpdateFromPlay()) {
83+
this.tryCancelAabUpdateTask();
84+
}
85+
}
8286
}
8387

8488
UpdateTaskImpl updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
@@ -89,7 +93,15 @@ UpdateTaskImpl updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
8993

9094
cachedUpdateTask = new UpdateTaskImpl();
9195
aabReleaseInProgress = newRelease;
92-
redirectToPlayForAabUpdate(newRelease.getDownloadUrl());
96+
hasBeenSentToPlayForCurrentTask = false;
97+
98+
// On a background thread, fetch the redirect URL and open it in the Play app
99+
runAsyncInTask(executor, () -> fetchDownloadRedirectUrl(newRelease.getDownloadUrl()))
100+
.onSuccessTask(combineWithResultOf(() -> lifecycleNotifier.getForegroundActivity()))
101+
.addOnSuccessListener(
102+
urlAndActivity ->
103+
openRedirectUrlInPlay(urlAndActivity.first(), urlAndActivity.second()))
104+
.addOnFailureListener(this::setUpdateTaskCompletionError);
93105

94106
return cachedUpdateTask;
95107
}
@@ -138,38 +150,30 @@ private static boolean isRedirectResponse(int responseCode) {
138150
return responseCode >= 300 && responseCode < 400;
139151
}
140152

141-
private void redirectToPlayForAabUpdate(String downloadUrl) {
142-
// The 302 redirect is obtained here to open the play store directly and avoid opening chrome
143-
executor.execute( // Execute the network calls on a background thread
144-
() -> {
145-
String redirectUrl;
146-
try {
147-
redirectUrl = fetchDownloadRedirectUrl(downloadUrl);
148-
} catch (FirebaseAppDistributionException e) {
149-
setUpdateTaskCompletionError(e);
150-
return;
151-
}
152-
153-
Intent updateIntent = new Intent(Intent.ACTION_VIEW);
154-
updateIntent.setData(Uri.parse(redirectUrl));
155-
updateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
156-
LogWrapper.getInstance().v(TAG + "Redirecting to play");
157-
158-
synchronized (updateAabLock) {
159-
context.startActivity(updateIntent);
160-
cachedUpdateTask.updateProgress(
161-
UpdateProgress.builder()
162-
.setApkBytesDownloaded(-1)
163-
.setApkFileTotalBytes(-1)
164-
.setUpdateStatus(UpdateStatus.REDIRECTED_TO_PLAY)
165-
.build());
166-
}
167-
});
153+
private void openRedirectUrlInPlay(String redirectUrl, Activity hostActivity) {
154+
Intent updateIntent = new Intent(Intent.ACTION_VIEW);
155+
updateIntent.setData(Uri.parse(redirectUrl));
156+
updateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
157+
LogWrapper.getInstance().v(TAG + "Redirecting to play");
158+
159+
// Launch the intent before the synchronized block to avoid failing to update in the rare
160+
// scenario where the activity moves to the background while we're awaiting the lock.
161+
hostActivity.startActivity(updateIntent);
162+
163+
synchronized (updateAabLock) {
164+
cachedUpdateTask.updateProgress(
165+
UpdateProgress.builder()
166+
.setApkBytesDownloaded(-1)
167+
.setApkFileTotalBytes(-1)
168+
.setUpdateStatus(UpdateStatus.REDIRECTED_TO_PLAY)
169+
.build());
170+
hasBeenSentToPlayForCurrentTask = true;
171+
}
168172
}
169173

170-
private void setUpdateTaskCompletionError(FirebaseAppDistributionException e) {
174+
private void setUpdateTaskCompletionError(Exception e) {
171175
synchronized (updateAabLock) {
172-
safeSetTaskException(cachedUpdateTask, e);
176+
safeSetTaskException(cachedUpdateTask, FirebaseAppDistributionException.wrap(e));
173177
}
174178
}
175179

@@ -183,4 +187,12 @@ void tryCancelAabUpdateTask() {
183187
ReleaseUtils.convertToAppDistributionRelease(aabReleaseInProgress)));
184188
}
185189
}
190+
191+
private boolean awaitingUpdateFromPlay() {
192+
synchronized (updateAabLock) {
193+
return cachedUpdateTask != null
194+
&& !cachedUpdateTask.isComplete()
195+
&& hasBeenSentToPlayForCurrentTask;
196+
}
197+
}
186198
}

‎firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public class FirebaseAppDistribution {
102102
new FirebaseAppDistributionTesterApiClient(),
103103
firebaseInstallationsApiProvider),
104104
new ApkUpdater(firebaseApp, new ApkInstaller()),
105-
new AabUpdater(firebaseApp.getApplicationContext()),
105+
new AabUpdater(),
106106
signInStorage,
107107
lifecycleNotifier);
108108
}

‎firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/TaskUtils.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ static <T1, T2> CombinedTaskResults<T1, T2> create(T1 first, T2 second) {
122122
*
123123
* <pre>{@code
124124
* runFirstAsyncTask()
125-
* .onSuccessTask(combineWithResultOf(() -> startSecondAsyncTask())
125+
* .onSuccessTask(combineWithResultOf(executor, () -> startSecondAsyncTask())
126126
* .addOnSuccessListener(
127127
* results ->
128128
* doSomethingWithBothResults(results.result1(), results.result2()));

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

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -15,24 +15,24 @@
1515

1616
import static com.google.common.truth.Truth.assertThat;
1717
import static com.google.firebase.appdistribution.TestUtils.assertTaskFailure;
18+
import static com.google.firebase.appdistribution.TestUtils.awaitAsyncOperations;
1819
import static org.junit.Assert.assertEquals;
19-
import static org.junit.Assert.assertThrows;
2020
import static org.mockito.Mockito.when;
2121
import static org.robolectric.Shadows.shadowOf;
22+
import static org.robolectric.annotation.LooperMode.Mode.PAUSED;
2223

2324
import android.app.Activity;
2425
import android.net.Uri;
25-
import androidx.test.core.app.ApplicationProvider;
2626
import com.google.android.gms.tasks.Tasks;
2727
import com.google.firebase.FirebaseApp;
28+
import com.google.firebase.appdistribution.Constants.ErrorMessages;
2829
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
2930
import java.io.ByteArrayInputStream;
3031
import java.io.IOException;
3132
import java.util.ArrayList;
3233
import java.util.List;
3334
import java.util.concurrent.ExecutorService;
3435
import java.util.concurrent.Executors;
35-
import java.util.concurrent.TimeUnit;
3636
import javax.net.ssl.HttpsURLConnection;
3737
import org.junit.Before;
3838
import org.junit.Test;
@@ -42,9 +42,11 @@
4242
import org.mockito.MockitoAnnotations;
4343
import org.robolectric.Robolectric;
4444
import org.robolectric.RobolectricTestRunner;
45+
import org.robolectric.annotation.LooperMode;
4546
import org.robolectric.shadows.ShadowActivity;
4647

4748
@RunWith(RobolectricTestRunner.class)
49+
@LooperMode(PAUSED)
4850
public class AabUpdaterTest {
4951
private static final String TEST_URL = "https://test-url";
5052
private static final String REDIRECT_TO_PLAY = "https://redirect-to-play-url";
@@ -85,11 +87,7 @@ public void setup() throws IOException, FirebaseAppDistributionException {
8587

8688
aabUpdater =
8789
Mockito.spy(
88-
new AabUpdater(
89-
ApplicationProvider.getApplicationContext(),
90-
mockLifecycleNotifier,
91-
mockHttpsUrlConnectionFactory,
92-
testExecutor));
90+
new AabUpdater(mockLifecycleNotifier, mockHttpsUrlConnectionFactory, testExecutor));
9391
when(mockLifecycleNotifier.getForegroundActivity()).thenReturn(Tasks.forResult(activity));
9492
}
9593

@@ -100,7 +98,7 @@ public void updateAppTask_whenOpenConnectionFails_setsNetworkFailure()
10098
when(mockHttpsUrlConnectionFactory.openConnection(TEST_URL)).thenThrow(caughtException);
10199

102100
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
103-
testExecutor.awaitTermination(1, TimeUnit.SECONDS);
101+
awaitAsyncOperations(testExecutor);
104102

105103
assertTaskFailure(
106104
updateTask, Status.NETWORK_FAILURE, "Failed to open connection", caughtException);
@@ -112,7 +110,7 @@ public void updateAppTask_isNotRedirectResponse_setsDownloadFailure()
112110
when(mockHttpsUrlConnection.getResponseCode()).thenReturn(200);
113111

114112
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
115-
testExecutor.awaitTermination(1, TimeUnit.SECONDS);
113+
awaitAsyncOperations(testExecutor);
116114

117115
assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "Expected redirect");
118116
}
@@ -123,7 +121,7 @@ public void updateAppTask_missingLocationHeader_setsDownloadFailure()
123121
when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn(null);
124122

125123
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
126-
testExecutor.awaitTermination(1, TimeUnit.SECONDS);
124+
awaitAsyncOperations(testExecutor);
127125

128126
assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "No Location header");
129127
}
@@ -133,18 +131,18 @@ public void updateAppTask_emptyLocationHeader_setsDownloadFailure() throws Inter
133131
when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn("");
134132

135133
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
136-
testExecutor.awaitTermination(1, TimeUnit.SECONDS);
134+
awaitAsyncOperations(testExecutor);
137135

138136
assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "Empty Location header");
139137
}
140138

141139
@Test
142-
public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws InterruptedException {
140+
public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Exception {
143141
List<UpdateProgress> progressEvents = new ArrayList<>();
144142

145143
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
146144
updateTask.addOnProgressListener(testExecutor, progressEvents::add);
147-
testExecutor.awaitTermination(1, TimeUnit.SECONDS);
145+
awaitAsyncOperations(testExecutor);
148146

149147
assertThat(shadowActivity.getNextStartedActivity().getData())
150148
.isEqualTo(Uri.parse(REDIRECT_TO_PLAY));
@@ -159,14 +157,13 @@ public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Inter
159157
}
160158

161159
@Test
162-
public void updateAppTask_onAppResume_setsUpdateCancelled() {
163-
TestOnCompleteListener<Void> onCompleteListener = new TestOnCompleteListener<>();
160+
public void updateAppTask_onAppResume_setsUpdateCancelled() throws InterruptedException {
164161
UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
165-
updateTask.addOnCompleteListener(testExecutor, onCompleteListener);
166-
162+
awaitAsyncOperations(testExecutor);
167163
aabUpdater.onActivityStarted(activity);
164+
168165
FirebaseAppDistributionException exception =
169-
assertThrows(FirebaseAppDistributionException.class, onCompleteListener::await);
166+
assertTaskFailure(updateTask, Status.INSTALLATION_CANCELED, ErrorMessages.UPDATE_CANCELED);
170167
assertEquals(
171168
ReleaseUtils.convertToAppDistributionRelease(TEST_RELEASE_NEWER_AAB_INTERNAL),
172169
exception.getRelease());

‎firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/TestOnCompleteListener.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626
* Helper listener that works around a limitation of the Tasks API where await() cannot be called on
2727
* the main thread. This listener works around it by running itself on a different thread, thus
2828
* allowing the main thread to be woken up when the Tasks complete.
29+
*
30+
* <p>Note: Calling {@link #await()} from a Robolectric test does block the main thread, since those
31+
* tests are executed on the main thread.
2932
*/
3033
public class TestOnCompleteListener<TResult> implements OnCompleteListener<TResult> {
3134
private static final long TIMEOUT_MS = 5000;

‎firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/TestUtils.java

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,40 @@
1414

1515
package com.google.firebase.appdistribution;
1616

17+
import static android.os.Looper.getMainLooper;
1718
import static com.google.common.truth.Truth.assertThat;
19+
import static org.robolectric.Shadows.shadowOf;
1820

1921
import com.google.android.gms.tasks.Task;
2022
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
23+
import java.util.concurrent.ExecutorService;
24+
import java.util.concurrent.TimeUnit;
2125

2226
final class TestUtils {
2327
private TestUtils() {}
2428

25-
static void assertTaskFailure(Task task, Status status, String messageSubstring) {
29+
static FirebaseAppDistributionException assertTaskFailure(
30+
Task task, Status status, String messageSubstring) {
2631
assertThat(task.isSuccessful()).isFalse();
2732
assertThat(task.getException()).isInstanceOf(FirebaseAppDistributionException.class);
2833
FirebaseAppDistributionException e = (FirebaseAppDistributionException) task.getException();
2934
assertThat(e.getErrorCode()).isEqualTo(status);
3035
assertThat(e).hasMessageThat().contains(messageSubstring);
36+
return e;
3137
}
3238

3339
static void assertTaskFailure(
3440
Task task, Status status, String messageSubstring, Throwable cause) {
3541
assertTaskFailure(task, status, messageSubstring);
3642
assertThat(task.getException()).hasCauseThat().isEqualTo(cause);
3743
}
44+
45+
static void awaitAsyncOperations(ExecutorService executorService) throws InterruptedException {
46+
// Await anything enqueued to the executor
47+
executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
48+
49+
// Idle the main looper, which is also running these tests, so any Task or lifecycle callbacks
50+
// can be handled. See http://robolectric.org/blog/2019/06/04/paused-looper/ for more info.
51+
shadowOf(getMainLooper()).idle();
52+
}
3853
}

0 commit comments

Comments
 (0)