Use sequential executor in place of synchronized blocks for cached new release #4430

lfkellogg · 2022-12-07T20:57:10Z

Continuing to remove synchronized blocks based on conversation on #4350.

This also uses an executor for more Task continuations.

google-oss-bot · 2022-12-07T20:57:43Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

google-oss-bot · 2022-12-07T21:03:19Z

Coverage Report ¹

Affected Products

`firebase-appdistribution`

Overall coverage changed from ? (45c2d7c) to 79.52% (98ea42c) by ?.

36 individual files with coverage change

Filename	Base (`45c2d7c`)	Merge (`98ea42c`)	Diff
AabUpdater.java	?	98.67%	?
ApkInstaller.java	?	96.88%	?
ApkUpdater.java	?	93.70%	?
AppDistributionReleaseImpl.java	?	100.00%	?
AppDistributionReleaseInternal.java	?	100.00%	?
AppIconSource.java	?	85.71%	?
AutoValue_AppDistributionReleaseImpl.java	?	65.45%	?
AutoValue_AppDistributionReleaseInternal.java	?	71.58%	?
AutoValue_TesterApiDisabledErrorDetails.java	?	29.41%	?
AutoValue_TesterApiDisabledErrorDetails_HelpLink.java	?	54.17%	?
AutoValue_UpdateProgressImpl.java	?	65.96%	?
ErrorMessages.java	?	0.00%	?
FirebaseAppDistributionExceptions.java	?	80.00%	?
FirebaseAppDistributionFileProvider.java	?	0.00%	?
FirebaseAppDistributionImpl.java	?	95.60%	?
FirebaseAppDistributionLifecycleNotifier.java	?	75.00%	?
FirebaseAppDistributionNotificationsManager.java	?	80.00%	?
FirebaseAppDistributionRegistrar.java	?	91.67%	?
FirebaseAppDistributionTesterApiClient.java	?	87.63%	?
HttpsUrlConnectionFactory.java	?	50.00%	?
InstallActivity.java	?	2.53%	?
LogWrapper.java	?	50.00%	?
NewReleaseFetcher.java	?	77.55%	?
PackageInfoUtils.java	?	42.86%	?
ReleaseIdentifier.java	?	88.00%	?
ReleaseUtils.java	?	83.33%	?
SequentialReference.java	?	100.00%	?
SignInResultActivity.java	?	0.00%	?
SignInStorage.java	?	42.86%	?
TaskCache.java	?	100.00%	?
TaskUtils.java	?	91.43%	?
TesterApiDisabledErrorDetails.java	?	93.75%	?
TesterApiHttpClient.java	?	89.19%	?
TesterSignInManager.java	?	93.62%	?
UpdateProgressImpl.java	?	100.00%	?
UpdateTaskImpl.java	?	76.00%	?

Test Logs

Merge (98ea42c): https://github.com/firebase/firebase-android-sdk/actions/runs/3660013640

https://storage.googleapis.com/firebase-sdk-metric-reports/Onp7uSJlBD.html

google-oss-bot · 2022-12-07T21:14:54Z

Size Report ¹

Affected Products

base
Type Base (45c2d7c) Merge (98ea42c) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-annotations
Type Base (45c2d7c) Merge (98ea42c) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 9.46 kB ? (?)
firebase-appdistribution
Type Base (45c2d7c) Merge (98ea42c) Diff
aar ? 145 kB ? (?)
apk (aggressive) ? 787 kB ? (?)
apk (release) ? 2.01 MB ? (?)
firebase-appdistribution-api
Type Base (45c2d7c) Merge (98ea42c) Diff
aar ? 14.2 kB ? (?)
apk (aggressive) ? 95.8 kB ? (?)
apk (release) ? 707 kB ? (?)
firebase-common
Type Base (45c2d7c) Merge (98ea42c) Diff
aar ? 67.4 kB ? (?)
apk (aggressive) ? 95.1 kB ? (?)
apk (release) ? 700 kB ? (?)
firebase-components
Type Base (45c2d7c) Merge (98ea42c) Diff
aar ? 44.9 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 33.6 kB ? (?)
firebase-installations
Type Base (45c2d7c) Merge (98ea42c) Diff
aar ? 55.0 kB ? (?)
apk (aggressive) ? 96.6 kB ? (?)
apk (release) ? 724 kB ? (?)
firebase-installations-interop
Type Base (45c2d7c) Merge (98ea42c) Diff
aar ? 8.05 kB ? (?)
apk (aggressive) ? 65.0 kB ? (?)
apk (release) ? 651 kB ? (?)

Test Logs

Merge (98ea42c): https://github.com/firebase/firebase-android-sdk/actions/runs/3660013640

https://storage.googleapis.com/firebase-sdk-metric-reports/Bvk7OzPcrR.html

lfkellogg · 2022-12-08T15:51:06Z

...tion/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

+    if (!isTesterSignedIn()) {
+      return Tasks.forException(
+          new FirebaseAppDistributionException("Tester is not signed in", AUTHENTICATION_FAILURE));
+    }


I also pulled this out of the getOrCreateTask because it does not actually depend on whether there is a cached task.

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

...st/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java

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

kaibolay · 2022-12-08T16:04:47Z

...distribution/src/main/java/com/google/firebase/appdistribution/impl/SequentialReference.java

+ * A reference to an object that uses a sequential executor to ensure non-concurrent reads and
+ * writes while preventing lock contention.
+ */
+class SequentialReference<T> {


I have to admit I don't really grok this. I really hope @vkryachko does.

Or maybe there is a different paradigm that can be used? This looks like low-level infrastructure which seems to be useful in general - so I'd assume there is some established pattern to do things like this.

I've updated this to be simpler based on our conversation today.

...st/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java

vkryachko · 2022-12-08T15:51:22Z

...tion/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

@@ -82,14 +78,17 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
      @NonNull ApkUpdater apkUpdater,
      @NonNull AabUpdater aabUpdater,
      @NonNull SignInStorage signInStorage,
-      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier) {
+      @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
+      @NonNull Executor lightweightExecutor) {


nit: consider annotating this argument with @Lightweight for increased readability.

vkryachko · 2022-12-09T14:53:32Z

...distribution/src/main/java/com/google/firebase/appdistribution/impl/SequentialReference.java

+  private T value;
+
+  /** Get a {@link SequentialReference} that controls access uses the given sequential executor. */
+  static SequentialReference withSequentialExecutor(Executor sequentialExecutor) {


qq, afaicr from our discussion, this is to allow having multiple references that are still synchronized?

If so, this may be error prone as it requires all continuations to use directExecutor, otherwise there would be no guarantees of synchronization. So maybe instead of allowing to share the sequential executor we should instead have T be a composite class of all state that needs to be synchronized together, wdyt?

Ah, I actually wasn't considering synchronization in terms of needing to perform reads of multiple values at once, but makes sense that you would need to do that. Agreed, in that case we should just have all of the state be combined into a single reference. I'll remove this.

vkryachko · 2022-12-09T14:56:46Z

...distribution/src/main/java/com/google/firebase/appdistribution/impl/SequentialReference.java

+  @VisibleForTesting
+  T getSnapshot() {
+    return value;
+  }


hm, this may actually be a source of flaky tests, since this is a data race, so observing the latest write is not guaranteed. How about using reference.get().get() in tests?

Yeah I think that's a good move (technically we'll have to use Tasks.await()).

I was assuming the tests would do their own awaiting on the executor but might as well enforce it to be safe. Updated.

vkryachko · 2022-12-09T14:59:45Z

...tion/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

+                .checkForNewRelease()
+                .onSuccessTask(lightweightExecutor, release -> cachedNewRelease.set(release))
+                .onSuccessTask(
+                    FirebaseExecutors.directExecutor(),


Is directExecutor required for correctness here? if not, consider lightweight instead
https://firebase.github.io/firebase-android-sdk/components/executors.html#direct-executor

As discussed, direct executor doesn't actually improve correctness in all cases. And correctness isn't even strictly necessary here. I'll remove it.

vkryachko · 2022-12-09T15:09:49Z

...distribution/src/main/java/com/google/firebase/appdistribution/impl/SequentialReference.java

+   * <p>Unless a direct executer is passed to {@link Task#addOnSuccessListener(Executor,
+   * OnSuccessListener)} or similar methods, be aware that the value may have changed by the time it
+   * is used.


I wonder if it's actually a problem, i.e. when directExecutor is used here's the sequence of events:

state = 0

get() called(returns 0)

a concurrent set(1) is queued but waits for 1 to complete

get() continuation executes and returns the transformed task using 0

task from 3 is returned to the caller

2 updates the value to 1

the caller observes results based on 0 when it is actually now equal 1

Now let's what happens when a "real" executor is used:

state = 0

get() called(returns 0)

a concurrent set(1) and updates the state to 1

get() continuation executes and returns the transformed task using 0

task from 3 is returned to the caller

the caller observes results based on 0 when it is actually now equal 1

So regardless of the sequence of events, the outcome is the same, the value may have changed by the time the caller observes the result, unless direct executor is used throughout the whole continuation change(which generally is not a good idea).

Point being: consider using lightweight throughout unless there is some internal invariant that I am missing and will get violated by this.

Yes, good call.

I think there's another reason not to necessarily recommend the direct executor. I was able to reproduce some slightly different behavior: if I introduce an artificial delay in the execution of the initial get(), then only a direct executor ensures the entire chain off of the get() will execute with the correct value before the first set(1) happens. This is because any callbacks get called on the sequential executor's thread immediately after the read or write happens. I was able to repro this.

But this was an artificial test, and it gets complicated in the case where the get() succeeds immediately. When the task is already complete before the handlers are added, the callbacks end up being called immediately on the calling thread, not the sequential executor's thread. And in that case there's no practical difference between using a direct executor or a "real" executor.

So a direct executor really doesn't guarantee an order of execution. More reason not to recommend it IMO. I think we just need to understand when data might be old and whether that's a problem.

vkryachko · 2022-12-09T15:23:22Z

...st/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java

+          .setDownloadUrl(TEST_URL)
+          .build();
+
+  private final ExecutorService testExecutor = Executors.newSingleThreadExecutor();


It's actually preferable to use the actual executor from components as it will detect violations, i.e. disk reads, network io, etc and crash the tests. Otherwise there is no guarantee that the code under test uses the right executor which will slow down all of firebase and cause logcat warnings in developers' apps

you can use this utility class to get a hold of the right executor:
https://github.com/firebase/firebase-android-sdk/blob/master/integ-testing/src/main/java/com/google/firebase/concurrent/TestOnlyExecutors.java

Oh, that's awesome. Done.

github-actions · 2022-12-09T18:44:35Z

Unit Test Results

129 tests 129 ✔️ 33s ⏱️
  13 suites     0 💤
  13 files     0 ❌

Results for commit 9ef0e50.

♻️ This comment has been updated with latest results.

firebase-appdistribution/firebase-appdistribution.gradle

vkryachko · 2022-12-09T19:18:05Z

...st/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java

@@ -236,7 +237,7 @@ public void checkForNewRelease_authenticationFailure_signOutTester() throws Inte
    Task<AppDistributionRelease> task = firebaseAppDistribution.checkForNewRelease();

    awaitTaskFailure(task, AUTHENTICATION_FAILURE, "Test");
-    awaitTermination(testExecutor);
+    awaitTermination(lightweightExecutor);


hm, these executors cannot be shutdown so I am not sure awaitTermination would work

but looks like tests have passed, so use your judgement

Hmm, I'm realizing that all of our awaitTermination() calls are really just sleeping for 100 ms because we're never shutting them down first. We need a better way to wait for executor-run tasks throughout our tests. Since these are passing right now though I'm inclined to tackle that in a followup.

google-oss-bot · 2022-12-09T19:27:36Z

Startup Time Report ¹

Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS.

Startup time comparison between the CI merge commit (98ea42c) and the base commit (45c2d7c) are not available.

No macrobenchmark data found for the base commit (45c2d7c). Analysis for the CI merge commit (98ea42c) can be found at:

https://storage.googleapis.com/firebase-sdk-metric-reports/tSRLqgHpYX/index.html

https://storage.googleapis.com/firebase-sdk-metric-reports/FUvG35aCeN/index.html

…w release (#4430) * Remove some synchronized blocks from FirebaseAppDistributionImpl * Rename UpdateTaskCache.java to TaskCache.java * Remove more sync blocks around new release cache * Some refactoring * Java formatting * Add tests * Add javadocs * Simplify SequentialReference and update tests * Update registrar * Fix test and add static imports from TestUtils * Incorporate some feedback * Use actual lightweight executor in test

google-oss-bot added the size/L label Dec 7, 2022

lfkellogg requested review from kaibolay and vkryachko December 8, 2022 15:49

lfkellogg commented Dec 8, 2022

View reviewed changes

kaibolay reviewed Dec 8, 2022

View reviewed changes

google-oss-bot added size/XL and removed size/L labels Dec 8, 2022

kaibolay reviewed Dec 9, 2022

View reviewed changes

...st/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java Show resolved Hide resolved

lfkellogg force-pushed the lk/new-release-cache branch from f11660b to bd925b7 Compare December 9, 2022 13:43

vkryachko reviewed Dec 9, 2022

View reviewed changes

lfkellogg force-pushed the fad/next branch from 1ae9488 to 45c2d7c Compare December 9, 2022 18:32

lfkellogg added 11 commits December 9, 2022 13:35

Remove some synchronized blocks from FirebaseAppDistributionImpl

8d14efe

Rename UpdateTaskCache.java to TaskCache.java

218af55

Remove more sync blocks around new release cache

2f398e6

Some refactoring

4c9243e

Java formatting

8c8ac7b

Add tests

1355cb1

Add javadocs

928ff1a

Simplify SequentialReference and update tests

f618340

Update registrar

1472f3e

Fix test and add static imports from TestUtils

0b6a977

Incorporate some feedback

e1bd329

lfkellogg force-pushed the lk/new-release-cache branch 2 times, most recently from a29b902 to e1bd329 Compare December 9, 2022 18:36

Use actual lightweight executor in test

9ef0e50

kaibolay reviewed Dec 9, 2022

View reviewed changes

firebase-appdistribution/firebase-appdistribution.gradle Show resolved Hide resolved

kaibolay approved these changes Dec 9, 2022

View reviewed changes

vkryachko reviewed Dec 9, 2022

View reviewed changes

vkryachko approved these changes Dec 9, 2022

View reviewed changes

lfkellogg merged commit 7192ac7 into fad/next Dec 9, 2022

lfkellogg deleted the lk/new-release-cache branch December 9, 2022 21:48

firebase locked and limited conversation to collaborators Jan 9, 2023

Use sequential executor in place of synchronized blocks for cached new release #4430

Use sequential executor in place of synchronized blocks for cached new release #4430

Uh oh!

Conversation

lfkellogg commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Dec 7, 2022

Uh oh!

google-oss-bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-appdistribution

Test Logs

Uh oh!

google-oss-bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

base

firebase-annotations

firebase-appdistribution

firebase-appdistribution-api

firebase-common

firebase-components

firebase-installations

firebase-installations-interop

Test Logs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Dec 9, 2022

Startup Time Report 1

Uh oh!

lfkellogg commented Dec 7, 2022 •

edited

Loading

google-oss-bot commented Dec 7, 2022 •

edited

Loading

Coverage Report ¹

`firebase-appdistribution`

google-oss-bot commented Dec 7, 2022 •

edited

Loading

Size Report ¹

`base`

`firebase-annotations`

`firebase-appdistribution`

`firebase-appdistribution-api`

`firebase-common`

`firebase-components`

`firebase-installations`

`firebase-installations-interop`

github-actions bot commented Dec 9, 2022 •

edited

Loading

Startup Time Report ¹