Skip to content

Commit debbc7f

Browse files
lfkelloggkaibolay
lfkellogg
authored and
kaibolay
committed
Remove synchronized blocks from NewReleaseFetcher (#4510)
* Remove synchronized usage in TaskCache * Re-arrange member declarations and make more consistent * Remove synchronized blocks from NewReleaseFetcher
1 parent 02e409b commit debbc7f

File tree

3 files changed

+57
-45
lines changed

3 files changed

+57
-45
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,10 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
120120
new TesterSignInManager(
121121
firebaseApp, firebaseInstallationsApiProvider, signInStorage, blockingExecutor),
122122
new NewReleaseFetcher(
123-
firebaseApp.getApplicationContext(), testerApiClient, releaseIdentifier),
123+
firebaseApp.getApplicationContext(),
124+
testerApiClient,
125+
releaseIdentifier,
126+
lightweightExecutor),
124127
new ApkUpdater(
125128
firebaseApp,
126129
new ApkInstaller(lightweightExecutor),

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

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

1717
import static com.google.firebase.appdistribution.impl.PackageInfoUtils.getPackageInfo;
1818

19-
import android.annotation.SuppressLint;
2019
import android.content.Context;
2120
import androidx.annotation.NonNull;
2221
import androidx.annotation.VisibleForTesting;
2322
import androidx.core.content.pm.PackageInfoCompat;
2423
import com.google.android.gms.tasks.Task;
2524
import com.google.android.gms.tasks.Tasks;
25+
import com.google.firebase.annotations.concurrent.Lightweight;
2626
import com.google.firebase.appdistribution.BinaryType;
2727
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
2828
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
29+
import java.util.concurrent.Executor;
2930

3031
/**
3132
* Class that handles fetching the latest release from App Distribution and determining if it is a
@@ -36,33 +37,32 @@ class NewReleaseFetcher {
3637

3738
private final FirebaseAppDistributionTesterApiClient firebaseAppDistributionTesterApiClient;
3839
private final ReleaseIdentifier releaseIdentifier;
40+
private @Lightweight Executor lightweightExecutor;
3941
private final Context context;
4042

41-
Task<AppDistributionReleaseInternal> cachedCheckForNewRelease = null;
43+
TaskCache<AppDistributionReleaseInternal> cachedCheckForNewRelease;
4244

4345
NewReleaseFetcher(
4446
@NonNull Context applicationContext,
4547
@NonNull FirebaseAppDistributionTesterApiClient firebaseAppDistributionTesterApiClient,
46-
ReleaseIdentifier releaseIdentifier) {
48+
ReleaseIdentifier releaseIdentifier,
49+
@Lightweight Executor lightweightExecutor) {
4750
this.firebaseAppDistributionTesterApiClient = firebaseAppDistributionTesterApiClient;
4851
this.context = applicationContext;
4952
this.releaseIdentifier = releaseIdentifier;
53+
this.lightweightExecutor = lightweightExecutor;
54+
this.cachedCheckForNewRelease = new TaskCache<>(lightweightExecutor);
5055
}
5156

52-
// TODO(b/261014422): Use an explicit executor in continuations.
53-
@SuppressLint("TaskMainThread")
5457
@NonNull
55-
public synchronized Task<AppDistributionReleaseInternal> checkForNewRelease() {
56-
if (cachedCheckForNewRelease != null && !cachedCheckForNewRelease.isComplete()) {
57-
return cachedCheckForNewRelease;
58-
}
59-
60-
this.cachedCheckForNewRelease =
61-
firebaseAppDistributionTesterApiClient
62-
.fetchNewRelease()
63-
.onSuccessTask(release -> Tasks.forResult(isNewerRelease(release) ? release : null));
64-
65-
return cachedCheckForNewRelease;
58+
public Task<AppDistributionReleaseInternal> checkForNewRelease() {
59+
return cachedCheckForNewRelease.getOrCreateTask(
60+
() ->
61+
firebaseAppDistributionTesterApiClient
62+
.fetchNewRelease()
63+
.onSuccessTask(
64+
lightweightExecutor,
65+
release -> Tasks.forResult(isNewerRelease(release) ? release : null)));
6666
}
6767

6868
private boolean isNewerRelease(AppDistributionReleaseInternal retrievedNewRelease)

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

Lines changed: 37 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,12 @@
1717
import static android.os.Looper.getMainLooper;
1818
import static com.google.common.truth.Truth.assertThat;
1919
import static com.google.firebase.appdistribution.BinaryType.APK;
20+
import static com.google.firebase.appdistribution.impl.TestUtils.awaitTask;
21+
import static com.google.firebase.appdistribution.impl.TestUtils.awaitTaskFailure;
2022
import static org.junit.Assert.assertEquals;
2123
import static org.mockito.Mockito.spy;
24+
import static org.mockito.Mockito.times;
25+
import static org.mockito.Mockito.verify;
2226
import static org.mockito.Mockito.when;
2327
import static org.robolectric.Shadows.shadowOf;
2428

@@ -28,11 +32,13 @@
2832
import com.google.android.gms.tasks.Task;
2933
import com.google.android.gms.tasks.TaskCompletionSource;
3034
import com.google.android.gms.tasks.Tasks;
35+
import com.google.firebase.annotations.concurrent.Lightweight;
3136
import com.google.firebase.appdistribution.BinaryType;
3237
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
3338
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
39+
import com.google.firebase.concurrent.TestOnlyExecutors;
40+
import java.util.concurrent.ExecutionException;
3441
import java.util.concurrent.Executor;
35-
import java.util.concurrent.Executors;
3642
import org.junit.Before;
3743
import org.junit.Test;
3844
import org.junit.runner.RunWith;
@@ -58,7 +64,7 @@ public class NewReleaseFetcherTest {
5864
@Mock private FirebaseAppDistributionTesterApiClient mockFirebaseAppDistributionTesterApiClient;
5965
@Mock private ReleaseIdentifier mockReleaseIdentifier;
6066

61-
Executor testExecutor = Executors.newSingleThreadExecutor();
67+
@Lightweight Executor lightweightExecutor = TestOnlyExecutors.lite();
6268

6369
@Before
6470
public void setup() throws FirebaseAppDistributionException {
@@ -84,11 +90,13 @@ public void setup() throws FirebaseAppDistributionException {
8490
new NewReleaseFetcher(
8591
ApplicationProvider.getApplicationContext(),
8692
mockFirebaseAppDistributionTesterApiClient,
87-
mockReleaseIdentifier));
93+
mockReleaseIdentifier,
94+
lightweightExecutor));
8895
}
8996

9097
@Test
91-
public void checkForNewRelease_whenCalledMultipleTimes_returnsTheSameTask() {
98+
public void checkForNewRelease_whenCalledMultipleTimes_onlyFetchesReleasesOnce()
99+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
92100
// Start a new task completion source and do not complete it, to mimic an in progress task
93101
TaskCompletionSource<AppDistributionReleaseInternal> task = new TaskCompletionSource<>();
94102
when(mockFirebaseAppDistributionTesterApiClient.fetchNewRelease()).thenReturn(task.getTask());
@@ -98,7 +106,13 @@ public void checkForNewRelease_whenCalledMultipleTimes_returnsTheSameTask() {
98106
Task<AppDistributionReleaseInternal> checkForNewReleaseTask2 =
99107
newReleaseFetcher.checkForNewRelease();
100108

101-
assertEquals(checkForNewReleaseTask1, checkForNewReleaseTask2);
109+
// Don't set the result until after calling twice, to make sure that the task from the first
110+
// call is still ongoing.
111+
task.setResult(null);
112+
awaitTask(checkForNewReleaseTask1);
113+
awaitTask(checkForNewReleaseTask2);
114+
115+
verify(mockFirebaseAppDistributionTesterApiClient, times(1)).fetchNewRelease();
102116
}
103117

104118
@Test
@@ -107,7 +121,7 @@ public void checkForNewRelease_newApkReleaseIsAvailable_returnsRelease() throws
107121
.thenReturn(Tasks.forResult(getTestNewRelease().build()));
108122

109123
Task<AppDistributionReleaseInternal> task = newReleaseFetcher.checkForNewRelease();
110-
AppDistributionReleaseInternal appDistributionReleaseInternal = TestUtils.awaitTask(task);
124+
AppDistributionReleaseInternal appDistributionReleaseInternal = awaitTask(task);
111125

112126
assertEquals(getTestNewRelease().build(), appDistributionReleaseInternal);
113127
}
@@ -123,23 +137,23 @@ public void checkForNewRelease_apiClientFailure() {
123137
Task<AppDistributionReleaseInternal> task = newReleaseFetcher.checkForNewRelease();
124138
shadowOf(getMainLooper()).idle();
125139

126-
assertThat(task.isSuccessful()).isFalse();
127-
assertThat(task.getException()).isEqualTo(expectedException);
140+
awaitTaskFailure(task, FirebaseAppDistributionException.Status.UNKNOWN, "test");
128141
}
129142

130143
@Test
131-
public void checkForNewRelease_whenNewReleaseIsSameRelease_returnsNull() {
144+
public void checkForNewRelease_whenNewReleaseIsSameRelease_returnsNull()
145+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
132146
when(mockFirebaseAppDistributionTesterApiClient.fetchNewRelease())
133147
.thenReturn(Tasks.forResult(getTestInstalledRelease().build()));
134148

135149
Task<AppDistributionReleaseInternal> releaseTask = newReleaseFetcher.checkForNewRelease();
136-
shadowOf(getMainLooper()).idle();
137150

138-
assertThat(releaseTask.getResult()).isNull();
151+
assertThat(awaitTask(releaseTask)).isNull();
139152
}
140153

141154
@Test
142-
public void checkForNewRelease_whenNewReleaseIsLowerVersionCode_returnsNull() {
155+
public void checkForNewRelease_whenNewReleaseIsLowerVersionCode_returnsNull()
156+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
143157
when(mockFirebaseAppDistributionTesterApiClient.fetchNewRelease())
144158
.thenReturn(
145159
Tasks.forResult(
@@ -148,14 +162,13 @@ public void checkForNewRelease_whenNewReleaseIsLowerVersionCode_returnsNull() {
148162
.build()));
149163

150164
Task<AppDistributionReleaseInternal> releaseTask = newReleaseFetcher.checkForNewRelease();
151-
shadowOf(getMainLooper()).idle();
152165

153-
assertThat(releaseTask.isSuccessful()).isTrue();
154-
assertThat(releaseTask.getResult()).isNull();
166+
assertThat(awaitTask(releaseTask)).isNull();
155167
}
156168

157169
@Test
158-
public void checkForNewRelease_whenNewAabIsAvailable_returnsRelease() {
170+
public void checkForNewRelease_whenNewAabIsAvailable_returnsRelease()
171+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
159172
AppDistributionReleaseInternal expectedRelease =
160173
getTestNewRelease()
161174
.setDownloadUrl("http://fake-download-url")
@@ -166,14 +179,13 @@ public void checkForNewRelease_whenNewAabIsAvailable_returnsRelease() {
166179
.thenReturn(Tasks.forResult(expectedRelease));
167180

168181
Task<AppDistributionReleaseInternal> result = newReleaseFetcher.checkForNewRelease();
169-
shadowOf(getMainLooper()).idle();
170182

171-
assertThat(result.isSuccessful()).isTrue();
172-
assertThat(result.getResult()).isEqualTo(expectedRelease);
183+
assertThat(awaitTask(result)).isEqualTo(expectedRelease);
173184
}
174185

175186
@Test
176-
public void checkForNewRelease_differentVersionNameThanInstalled_returnsRelease() {
187+
public void checkForNewRelease_differentVersionNameThanInstalled_returnsRelease()
188+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
177189
AppDistributionReleaseInternal expectedRelease =
178190
getTestNewRelease()
179191
.setDownloadUrl("http://fake-download-url")
@@ -185,14 +197,13 @@ public void checkForNewRelease_differentVersionNameThanInstalled_returnsRelease(
185197
.thenReturn(Tasks.forResult(expectedRelease));
186198

187199
Task<AppDistributionReleaseInternal> result = newReleaseFetcher.checkForNewRelease();
188-
shadowOf(getMainLooper()).idle();
189200

190-
assertThat(result.isSuccessful()).isTrue();
191-
assertThat(result.getResult()).isEqualTo(expectedRelease);
201+
assertThat(awaitTask(result)).isEqualTo(expectedRelease);
192202
}
193203

194204
@Test
195-
public void checkForNewRelease_whenNewReleaseIsSameAsInstalledAab_returnsNull() {
205+
public void checkForNewRelease_whenNewReleaseIsSameAsInstalledAab_returnsNull()
206+
throws FirebaseAppDistributionException, ExecutionException, InterruptedException {
196207
when(mockFirebaseAppDistributionTesterApiClient.fetchNewRelease())
197208
.thenReturn(
198209
Tasks.forResult(
@@ -203,10 +214,8 @@ public void checkForNewRelease_whenNewReleaseIsSameAsInstalledAab_returnsNull()
203214
.build()));
204215

205216
Task<AppDistributionReleaseInternal> result = newReleaseFetcher.checkForNewRelease();
206-
shadowOf(getMainLooper()).idle();
207217

208-
assertThat(result.isSuccessful()).isTrue();
209-
assertThat(result.getResult()).isNull();
218+
assertThat(awaitTask(result)).isNull();
210219
}
211220

212221
@Test
@@ -216,7 +225,7 @@ public void checkForNewRelease_onlyDifferenceIsMissingApkHash_throwsError() {
216225

217226
Task<AppDistributionReleaseInternal> task = newReleaseFetcher.checkForNewRelease();
218227

219-
TestUtils.awaitTaskFailure(task, Status.UNKNOWN, "Missing APK hash");
228+
awaitTaskFailure(task, Status.UNKNOWN, "Missing APK hash");
220229
}
221230

222231
private AppDistributionReleaseInternal.Builder getTestNewRelease() {

0 commit comments

Comments
 (0)