Skip to content

Commit 3c46f47

Browse files
lfkellogglfkellogg
committed
Remove synchronized blocks from ApkInstaller (#4486)
* Remove synchronized blocks from ApkInstaller * Fix broken test * Address Kai's feedback
1 parent 25bdc5f commit 3c46f47

File tree

7 files changed

+175
-70
lines changed

7 files changed

+175
-70
lines changed

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

Lines changed: 22 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -14,66 +14,56 @@
1414

1515
package com.google.firebase.appdistribution.impl;
1616

17-
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
18-
1917
import android.app.Activity;
2018
import android.content.Intent;
21-
import androidx.annotation.GuardedBy;
2219
import androidx.annotation.Nullable;
20+
import androidx.annotation.VisibleForTesting;
2321
import com.google.android.gms.tasks.Task;
2422
import com.google.android.gms.tasks.TaskCompletionSource;
23+
import com.google.firebase.annotations.concurrent.Lightweight;
2524
import com.google.firebase.appdistribution.FirebaseAppDistribution;
2625
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
26+
import java.util.concurrent.Executor;
2727

2828
/** Class that handles installing APKs in {@link FirebaseAppDistribution}. */
2929
class ApkInstaller {
3030
private static final String TAG = "ApkInstaller";
3131

3232
private final FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier;
33+
private final TaskCompletionSourceCache<Void> installTaskCompletionSourceCache;
34+
private final @Lightweight Executor lightweightExecutor;
3335

34-
@GuardedBy("installTaskLock")
35-
private TaskCompletionSource<Void> installTaskCompletionSource;
36-
37-
private final Object installTaskLock = new Object();
38-
39-
ApkInstaller(FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier) {
36+
@VisibleForTesting
37+
ApkInstaller(
38+
FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier,
39+
@Lightweight Executor lightweightExecutor) {
4040
this.lifeCycleNotifier = lifeCycleNotifier;
41-
lifeCycleNotifier.addOnActivityStartedListener(this::onActivityStarted);
41+
this.installTaskCompletionSourceCache = new TaskCompletionSourceCache<>(lightweightExecutor);
42+
this.lightweightExecutor = lightweightExecutor;
4243
lifeCycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
4344
}
4445

45-
ApkInstaller() {
46-
this(FirebaseAppDistributionLifecycleNotifier.getInstance());
47-
}
48-
49-
void onActivityStarted(@Nullable Activity activity) {
50-
synchronized (installTaskLock) {
51-
if (installTaskCompletionSource == null
52-
|| installTaskCompletionSource.getTask().isComplete()
53-
|| activity == null) {
54-
return;
55-
}
56-
}
46+
ApkInstaller(@Lightweight Executor lightweightExecutor) {
47+
this(FirebaseAppDistributionLifecycleNotifier.getInstance(), lightweightExecutor);
5748
}
5849

5950
void onActivityDestroyed(@Nullable Activity activity) {
6051
if (activity instanceof InstallActivity) {
6152
// Since install activity is destroyed but app is still active, installation has failed /
6253
// cancelled.
63-
this.trySetInstallTaskError();
54+
installTaskCompletionSourceCache.setException(
55+
new FirebaseAppDistributionException(
56+
ErrorMessages.APK_INSTALLATION_FAILED,
57+
FirebaseAppDistributionException.Status.INSTALLATION_FAILURE));
6458
}
6559
}
6660

6761
Task<Void> installApk(String path, Activity currentActivity) {
68-
synchronized (installTaskLock) {
69-
startInstallActivity(path, currentActivity);
70-
71-
if (this.installTaskCompletionSource == null
72-
|| this.installTaskCompletionSource.getTask().isComplete()) {
73-
this.installTaskCompletionSource = new TaskCompletionSource<>();
74-
}
75-
return installTaskCompletionSource.getTask();
76-
}
62+
return installTaskCompletionSourceCache.getOrCreateTaskFromCompletionSource(
63+
() -> {
64+
startInstallActivity(path, currentActivity);
65+
return new TaskCompletionSource<>();
66+
});
7767
}
7868

7969
private void startInstallActivity(String path, Activity currentActivity) {
@@ -82,14 +72,4 @@ private void startInstallActivity(String path, Activity currentActivity) {
8272
currentActivity.startActivity(intent);
8373
LogWrapper.v(TAG, "Prompting tester with install activity");
8474
}
85-
86-
void trySetInstallTaskError() {
87-
synchronized (installTaskLock) {
88-
safeSetTaskException(
89-
installTaskCompletionSource,
90-
new FirebaseAppDistributionException(
91-
ErrorMessages.APK_INSTALLATION_FAILED,
92-
FirebaseAppDistributionException.Status.INSTALLATION_FAILURE));
93-
}
94-
}
9575
}

‎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
@@ -121,7 +121,7 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
121121
firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
122122
new NewReleaseFetcher(
123123
firebaseApp.getApplicationContext(), testerApiClient, releaseIdentifier),
124-
new ApkUpdater(firebaseApp, new ApkInstaller(), blockingExecutor),
124+
new ApkUpdater(firebaseApp, new ApkInstaller(lightweightExecutor), blockingExecutor),
125125
new AabUpdater(blockingExecutor),
126126
signInStorage,
127127
lifecycleNotifier,

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

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
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.annotations.concurrent.Lightweight;
20+
import com.google.firebase.concurrent.FirebaseExecutors;
21+
import java.util.concurrent.Executor;
22+
23+
/**
24+
* A cache for a {@link TaskCompletionSource}, for use in cases where we only ever want one active
25+
* task at a time for a particular operation.
26+
*
27+
* <p>This is equivalent to {@link UpdateTaskCache} but for Tasks.
28+
*/
29+
class TaskCompletionSourceCache<T> {
30+
31+
/** A functional interface for a producer of a new {@link TaskCompletionSource}. */
32+
@FunctionalInterface
33+
interface TaskCompletionSourceProducer<T> {
34+
35+
/** Produce a new {@link TaskCompletionSource}. */
36+
TaskCompletionSource<T> produce();
37+
}
38+
39+
private TaskCompletionSource<T> cachedTaskCompletionSource;
40+
private final Executor sequentialExecutor;
41+
42+
/**
43+
* Constructor for a {@link TaskCompletionSourceCache} that controls access using its own
44+
* sequential executor backed by the given base executor.
45+
*
46+
* @param baseExecutor Executor to back the sequential executor. This can be a {@link Lightweight}
47+
* executor unless the {@link TaskCompletionSourceProducer} passed to {@link
48+
* #getOrCreateTaskFromCompletionSource} needs to be executed on a different executor.
49+
*/
50+
TaskCompletionSourceCache(Executor baseExecutor) {
51+
sequentialExecutor = FirebaseExecutors.newSequentialExecutor(baseExecutor);
52+
}
53+
54+
/**
55+
* Gets a cached {@link TaskCompletionSource}, if there is one and it is not completed, or else
56+
* calls the given {@code producer} and caches the return value.
57+
*/
58+
Task<T> getOrCreateTaskFromCompletionSource(TaskCompletionSourceProducer<T> producer) {
59+
TaskCompletionSource<T> taskCompletionSource = new TaskCompletionSource<>();
60+
sequentialExecutor.execute(
61+
() -> {
62+
if (!isOngoing(cachedTaskCompletionSource)) {
63+
cachedTaskCompletionSource = producer.produce();
64+
}
65+
TaskUtils.shadowTask(taskCompletionSource, cachedTaskCompletionSource.getTask());
66+
});
67+
return taskCompletionSource.getTask();
68+
}
69+
70+
/**
71+
* Sets the result on the cached {@link TaskCompletionSource}, if there is one and it is not
72+
* completed.
73+
*/
74+
Task<Void> setResult(T result) {
75+
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
76+
sequentialExecutor.execute(
77+
() -> {
78+
if (isOngoing(cachedTaskCompletionSource)) {
79+
cachedTaskCompletionSource.setResult(result);
80+
}
81+
});
82+
return taskCompletionSource.getTask();
83+
}
84+
85+
/**
86+
* Sets the exception on the cached {@link TaskCompletionSource}, if there is one and it is not
87+
* completed.
88+
*/
89+
Task<Void> setException(Exception e) {
90+
TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
91+
sequentialExecutor.execute(
92+
() -> {
93+
if (isOngoing(cachedTaskCompletionSource)) {
94+
cachedTaskCompletionSource.setException(e);
95+
}
96+
});
97+
return taskCompletionSource.getTask();
98+
}
99+
100+
private static <T> boolean isOngoing(TaskCompletionSource<T> taskCompletionSource) {
101+
return taskCompletionSource != null && !taskCompletionSource.getTask().isComplete();
102+
}
103+
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
2222
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
2323
import com.google.firebase.appdistribution.UpdateTask;
24+
import com.google.firebase.concurrent.FirebaseExecutors;
2425
import java.util.concurrent.Executor;
2526

2627
class TaskUtils {
@@ -144,5 +145,15 @@ static <T> UpdateTask onSuccessUpdateTask(
144145
return updateTask;
145146
}
146147

148+
/** Set a {@link TaskCompletionSource} to be resolved with the result of another {@link Task}. */
149+
static <T> void shadowTask(TaskCompletionSource<T> taskCompletionSource, Task<T> task) {
150+
// Using direct executor here ensures that any handlers that were themselves added using a
151+
// direct executor will behave as expected: they'll be executed on the thread that sets the
152+
// result.
153+
task.addOnSuccessListener(FirebaseExecutors.directExecutor(), taskCompletionSource::setResult)
154+
.addOnFailureListener(
155+
FirebaseExecutors.directExecutor(), taskCompletionSource::setException);
156+
}
157+
147158
private TaskUtils() {}
148159
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,9 @@ void updateProgress(@NonNull UpdateProgress updateProgress) {
7373
* progress or completing this task with the same changes.
7474
*/
7575
void shadow(UpdateTask updateTask) {
76+
// Using direct executor here ensures that any handlers that were themselves added using a
77+
// direct executor will behave as expected: they'll be executed on the thread that sets the
78+
// result or updates progress.
7679
updateTask
7780
.addOnProgressListener(FirebaseExecutors.directExecutor(), this::updateProgress)
7881
.addOnSuccessListener(FirebaseExecutors.directExecutor(), unused -> setResult())

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

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,27 @@
1414

1515
package com.google.firebase.appdistribution.impl;
1616

17+
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.INSTALLATION_FAILURE;
18+
import static com.google.firebase.appdistribution.impl.TestUtils.awaitAsyncOperations;
19+
import static com.google.firebase.appdistribution.impl.TestUtils.awaitTaskFailure;
1720
import static org.junit.Assert.assertEquals;
1821
import static org.junit.Assert.assertFalse;
22+
import static org.mockito.ArgumentMatchers.any;
23+
import static org.mockito.Mockito.doNothing;
24+
import static org.mockito.Mockito.mock;
25+
import static org.mockito.Mockito.times;
26+
import static org.mockito.Mockito.verify;
1927
import static org.robolectric.Shadows.shadowOf;
2028

29+
import android.app.Activity;
2130
import android.content.Intent;
2231
import com.google.android.gms.tasks.Task;
32+
import com.google.firebase.annotations.concurrent.Lightweight;
2333
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
2434
import com.google.firebase.appdistribution.impl.FirebaseAppDistributionServiceImplTest.TestActivity;
35+
import com.google.firebase.concurrent.TestOnlyExecutors;
36+
import java.util.concurrent.ExecutionException;
37+
import java.util.concurrent.ExecutorService;
2538
import org.junit.Before;
2639
import org.junit.Test;
2740
import org.junit.runner.RunWith;
@@ -37,58 +50,52 @@ public class ApkInstallerTests {
3750
private ShadowActivity shadowActivity;
3851
private ApkInstaller apkInstaller;
3952
@Mock private FirebaseAppDistributionLifecycleNotifier mockLifecycleNotifier;
53+
@Lightweight private ExecutorService lightweightExecutor;
4054

4155
@Before
4256
public void setUp() {
4357
MockitoAnnotations.initMocks(this);
4458

4559
activity = Robolectric.buildActivity(TestActivity.class).create().get();
4660
shadowActivity = shadowOf(activity);
47-
apkInstaller = new ApkInstaller(mockLifecycleNotifier);
61+
lightweightExecutor = TestOnlyExecutors.lite();
62+
apkInstaller = new ApkInstaller(mockLifecycleNotifier, lightweightExecutor);
4863
}
4964

5065
@Test
51-
public void installApk_currentActivityNotNull_InstallIntentOnCurrentActivity() {
66+
public void installApk_currentActivityNotNull_InstallIntentOnCurrentActivity()
67+
throws InterruptedException {
5268
String path = "path";
5369
Task<Void> installTask = apkInstaller.installApk(path, activity);
70+
awaitAsyncOperations(lightweightExecutor);
5471
Intent installIntent = shadowActivity.getNextStartedActivity();
5572

5673
assertFalse(installTask.isComplete());
5774
assertEquals(
5875
"com.google.firebase.appdistribution.impl.InstallActivity",
5976
installIntent.getComponent().getShortClassName());
6077
assertEquals(path, installIntent.getExtras().get("INSTALL_PATH"));
61-
}
62-
63-
@Test
64-
public void
65-
setCurrentActivity_appInForegroundAfterAnInstallAttempt_installIntentOnCurrentActivity() {
66-
apkInstaller.onActivityStarted(null);
67-
String path = "path123";
68-
Task<Void> installTask = apkInstaller.installApk(path, activity);
69-
apkInstaller.onActivityStarted(activity);
70-
71-
Intent installIntent = shadowActivity.getNextStartedActivity();
72-
73-
assertEquals(
74-
"com.google.firebase.appdistribution.impl.InstallActivity",
75-
installIntent.getComponent().getShortClassName());
76-
assertEquals(path, installIntent.getExtras().get("INSTALL_PATH"));
7778
assertFalse(installTask.isComplete());
7879
}
7980

8081
@Test
8182
public void installActivityDestroyed_setsInstallError() {
8283
Task<Void> installTask = apkInstaller.installApk("path", activity);
8384
apkInstaller.onActivityDestroyed(new InstallActivity());
84-
Exception ex = installTask.getException();
8585

86-
assert ex instanceof FirebaseAppDistributionException;
87-
assertEquals(
88-
ErrorMessages.APK_INSTALLATION_FAILED,
89-
((FirebaseAppDistributionException) ex).getMessage());
90-
assertEquals(
91-
FirebaseAppDistributionException.Status.INSTALLATION_FAILURE,
92-
((FirebaseAppDistributionException) ex).getErrorCode());
86+
awaitTaskFailure(installTask, INSTALLATION_FAILURE, ErrorMessages.APK_INSTALLATION_FAILED);
87+
}
88+
89+
@Test
90+
public void whenCalledMultipleTimes_onlyEmitsOneIntent()
91+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
92+
Activity mockActivity = mock(Activity.class);
93+
doNothing().when(mockActivity).startActivity(any());
94+
95+
apkInstaller.installApk("path", mockActivity);
96+
apkInstaller.installApk("path", mockActivity);
97+
awaitAsyncOperations(lightweightExecutor);
98+
99+
verify(mockActivity, times(1)).startActivity(any());
93100
}
94101
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -514,8 +514,9 @@ private AlertDialog assertAlertDialogShown() {
514514
}
515515

516516
@Test
517-
public void signOutTester_setsSignInStatusFalse() {
517+
public void signOutTester_setsSignInStatusFalse() throws InterruptedException {
518518
firebaseAppDistribution.signOutTester();
519+
awaitAsyncOperations(backgroundExecutor);
519520

520521
assertThat(signInStorage.getSignInStatusBlocking()).isFalse();
521522
}

0 commit comments

Comments
 (0)