Remove synchronized blocks from ApkInstaller #4486

lfkellogg · 2022-12-22T12:24:17Z

Uses a TaskCompletionSourceCache, which allows caching tasks which can then be resolved later in a thread-safe manner.

google-oss-bot · 2022-12-22T12:24:49Z

	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-22T12:32:01Z

Coverage Report ¹

Affected Products

`firebase-appdistribution`

Overall coverage changed from ? (a28c43e) to 77.08% (c2e4d4d) by ?.

43 individual files with coverage change

Filename	Base (`a28c43e`)	Merge (`c2e4d4d`)	Diff
AabUpdater.java	?	98.67%	?
ApkInstaller.java	?	100.00%	?
ApkUpdater.java	?	93.97%	?
AppDistributionReleaseImpl.java	?	100.00%	?
AppDistributionReleaseInternal.java	?	100.00%	?
AppIconSource.java	?	84.62%	?
AutoValue_AppDistributionReleaseImpl.java	?	65.45%	?
AutoValue_AppDistributionReleaseInternal.java	?	71.58%	?
AutoValue_ImageUtils_ImageSize.java	?	35.00%	?
AutoValue_TesterApiDisabledErrorDetails.java	?	29.41%	?
AutoValue_TesterApiDisabledErrorDetails_HelpLink.java	?	54.17%	?
AutoValue_UpdateProgressImpl.java	?	65.96%	?
ErrorMessages.java	?	0.00%	?
FeedbackActivity.java	?	1.12%	?
FeedbackSender.java	?	84.62%	?
FirebaseAppDistributionExceptions.java	?	80.00%	?
FirebaseAppDistributionFileProvider.java	?	0.00%	?
FirebaseAppDistributionImpl.java	?	90.00%	?
FirebaseAppDistributionLifecycleNotifier.java	?	92.91%	?
FirebaseAppDistributionNotificationsManager.java	?	87.18%	?
FirebaseAppDistributionRegistrar.java	?	86.27%	?
FirebaseAppDistributionTesterApiClient.java	?	88.54%	?
HttpsUrlConnectionFactory.java	?	50.00%	?
ImageUtils.java	?	100.00%	?
InstallActivity.java	?	2.67%	?
LogWrapper.java	?	100.00%	?
NewReleaseFetcher.java	?	80.85%	?
PackageInfoUtils.java	?	42.86%	?
ReleaseIdentifier.java	?	85.54%	?
ReleaseUtils.java	?	83.33%	?
ScreenshotTaker.java	?	41.18%	?
SequentialReference.java	?	100.00%	?
SignInResultActivity.java	?	0.00%	?
SignInStorage.java	?	100.00%	?
TakeScreenshotAndStartFeedbackActivity.java	?	0.00%	?
TaskCache.java	?	100.00%	?
TaskCompletionSourceCache.java	?	73.91%	?
TaskUtils.java	?	92.50%	?
TesterApiDisabledErrorDetails.java	?	93.75%	?
TesterApiHttpClient.java	?	90.18%	?
TesterSignInManager.java	?	96.12%	?
UpdateProgressImpl.java	?	100.00%	?
UpdateTaskImpl.java	?	76.00%	?

Test Logs

Merge (c2e4d4d): https://github.com/firebase/firebase-android-sdk/actions/runs/3838798601

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

github-actions · 2022-12-22T12:32:59Z

Unit Test Results

166 tests 166 ✔️ 48s ⏱️
  16 suites     0 💤
  16 files     0 ❌

Results for commit bec32df.

♻️ This comment has been updated with latest results.

google-oss-bot · 2022-12-22T12:39:04Z

Size Report ¹

Affected Products

base
Type Base (a28c43e) Merge (c2e4d4d) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-annotations
Type Base (a28c43e) Merge (c2e4d4d) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 9.46 kB ? (?)
firebase-appdistribution
Type Base (a28c43e) Merge (c2e4d4d) Diff
aar ? 154 kB ? (?)
apk (aggressive) ? 833 kB ? (?)
apk (release) ? 2.01 MB ? (?)
firebase-appdistribution-api
Type Base (a28c43e) Merge (c2e4d4d) Diff
aar ? 16.0 kB ? (?)
apk (aggressive) ? 95.8 kB ? (?)
apk (release) ? 706 kB ? (?)
firebase-common
Type Base (a28c43e) Merge (c2e4d4d) Diff
aar ? 67.4 kB ? (?)
apk (aggressive) ? 95.1 kB ? (?)
apk (release) ? 700 kB ? (?)
firebase-components
Type Base (a28c43e) Merge (c2e4d4d) Diff
aar ? 44.9 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 33.6 kB ? (?)
firebase-installations
Type Base (a28c43e) Merge (c2e4d4d) Diff
aar ? 55.0 kB ? (?)
apk (aggressive) ? 96.6 kB ? (?)
apk (release) ? 724 kB ? (?)
firebase-installations-interop
Type Base (a28c43e) Merge (c2e4d4d) Diff
aar ? 8.05 kB ? (?)
apk (aggressive) ? 65.0 kB ? (?)
apk (release) ? 651 kB ? (?)

Test Logs

Merge (c2e4d4d): https://github.com/firebase/firebase-android-sdk/actions/runs/3838798601

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

google-oss-bot · 2022-12-22T12:50:54Z

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 (c2e4d4d) and the base commit (a28c43e) are not available.

No macrobenchmark data found for the base commit (a28c43e). Analysis for the CI merge commit (c2e4d4d) can be found at:

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

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

kaibolay · 2023-01-04T00:14:53Z

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

@@ -84,12 +69,9 @@ private void startInstallActivity(String path, Activity currentActivity) {
  }

  void trySetInstallTaskError() {


does this still need to be called "try"?

I'll do you one better: this method doesn't even need to exist! It's only called once so I'm going to inline it.

kaibolay · 2023-01-04T00:21:06Z

...bution/src/main/java/com/google/firebase/appdistribution/impl/TaskCompletionSourceCache.java

+   * Constructor for a {@link TaskCompletionSourceCache} that controls access using its own
+   * sequential executor backed by the given base executor.
+   *
+   * @param baseExecutor Executor (typically {@link Lightweight}) to back the sequential executor.


Are there any cases where this should not (or is not) the "lightweight" executor?

If so, I think it should be documented which circumstances require a different executor. Does it depend on what TaskCompletionSourceProducer.produce() does?

If not, maybe change "typically" to "always" and annotate the consturctor parameter accordingly.

Good point. It does depend on what TaskCompletionSourceProducer.produce() does. I've reworded it.

kaibolay · 2023-01-04T00:25:08Z

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

@@ -144,5 +145,12 @@ static <T> UpdateTask onSuccessUpdateTask(
    return updateTask;
  }

+  /** Set a {@link TaskCompletionSource} to be resolved with the result of another {@link Task}. */
+  static <T> void shadowTask(TaskCompletionSource<T> taskCompletionSource, Task<T> task) {


Can/should this be a method on TaskCompletionSource itself?
If so, can/should it use the TaskCompletionsSource's executor instead of the "direct" one?

TaskCompletionSource is provided by the play-services library, so we can't add it there (although we could file a feature request).

Also just a note: TaskCompletionSource doesn't actually have its own executor. When you set a result on it, it passes that through to the underlying TaskImpl, which executes any handlers on the executors that were specified when they were added. Using direct executor here ensures that any handlers that were themselves added using a direct executor will behave as expected: they'll be executed on the thread that set the result. (I'll add a note to that effect).

kaibolay · 2023-01-04T00:53:30Z

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

-  private final Object installTaskLock = new Object();
-
-  ApkInstaller(FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier) {
+  ApkInstaller(


should this constructor be annotated with @VisibleForTesting?

lfkellogg · 2022-12-22T19:46:39Z

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

-    this(FirebaseAppDistributionLifecycleNotifier.getInstance());
-  }
-
-  void onActivityStarted(@Nullable Activity activity) {


This method was literally doing nothing

lfkellogg · 2022-12-22T19:47:22Z

...ppdistribution/src/test/java/com/google/firebase/appdistribution/impl/ApkInstallerTests.java

-  public void
-      setCurrentActivity_appInForegroundAfterAnInstallAttempt_installIntentOnCurrentActivity() {


This was testing the onActivityStarted() that I removed.

lfkellogg · 2023-01-03T19:51:06Z

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

    firebaseAppDistribution.signOutTester();
+    awaitAsyncOperations(backgroundExecutor);


Noticed this test was flaky without this, which makes sense since the update to SharedPreferences happens on the background thread.

lfkellogg · 2023-01-04T13:33:22Z

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

-  private final Object installTaskLock = new Object();
-
-  ApkInstaller(FirebaseAppDistributionLifecycleNotifier lifeCycleNotifier) {
+  ApkInstaller(


lfkellogg · 2023-01-04T13:34:43Z

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

@@ -84,12 +69,9 @@ private void startInstallActivity(String path, Activity currentActivity) {
  }

  void trySetInstallTaskError() {


I'll do you one better: this method doesn't even need to exist! It's only called once so I'm going to inline it.

lfkellogg · 2023-01-04T13:47:46Z

...bution/src/main/java/com/google/firebase/appdistribution/impl/TaskCompletionSourceCache.java

+   * Constructor for a {@link TaskCompletionSourceCache} that controls access using its own
+   * sequential executor backed by the given base executor.
+   *
+   * @param baseExecutor Executor (typically {@link Lightweight}) to back the sequential executor.


Good point. It does depend on what TaskCompletionSourceProducer.produce() does. I've reworded it.

lfkellogg · 2023-01-04T14:06:38Z

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

@@ -144,5 +145,12 @@ static <T> UpdateTask onSuccessUpdateTask(
    return updateTask;
  }

+  /** Set a {@link TaskCompletionSource} to be resolved with the result of another {@link Task}. */
+  static <T> void shadowTask(TaskCompletionSource<T> taskCompletionSource, Task<T> task) {


TaskCompletionSource is provided by the play-services library, so we can't add it there (although we could file a feature request).

Also just a note: TaskCompletionSource doesn't actually have its own executor. When you set a result on it, it passes that through to the underlying TaskImpl, which executes any handlers on the executors that were specified when they were added. Using direct executor here ensures that any handlers that were themselves added using a direct executor will behave as expected: they'll be executed on the thread that set the result. (I'll add a note to that effect).

* Remove synchronized blocks from ApkInstaller * Fix broken test * Address Kai's feedback

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

lfkellogg force-pushed the lk/no-sync-apk-installer branch from 8432a15 to e452ad6 Compare December 22, 2022 22:22

lfkellogg changed the base branch from fad/in-app-feedback to lk/harden-signinstorage December 22, 2022 22:23

lfkellogg mentioned this pull request Dec 22, 2022

Remove synchronized blocks from ApkUpdater #4494

Merged

Base automatically changed from lk/harden-signinstorage to fad/in-app-feedback January 3, 2023 18:21

Remove synchronized blocks from ApkInstaller

ec13be6

lfkellogg force-pushed the lk/no-sync-apk-installer branch from e452ad6 to ec13be6 Compare January 3, 2023 18:34

Fix broken test

10ea377

lfkellogg requested a review from kaibolay January 3, 2023 23:55

kaibolay approved these changes Jan 4, 2023

View reviewed changes

kaibolay reviewed Jan 4, 2023

View reviewed changes

Address Kai's feedback

bec32df

lfkellogg commented Jan 4, 2023

View reviewed changes

lfkellogg merged commit 8a54521 into fad/in-app-feedback Jan 4, 2023

lfkellogg deleted the lk/no-sync-apk-installer branch January 4, 2023 14:54

lfkellogg added a commit that referenced this pull request Jan 5, 2023

Remove synchronized blocks from ApkInstaller (#4486)

3c46f47

* Remove synchronized blocks from ApkInstaller * Fix broken test * Address Kai's feedback

kaibolay pushed a commit that referenced this pull request Jan 23, 2023

Remove synchronized blocks from ApkInstaller (#4486)

da3b722

* Remove synchronized blocks from ApkInstaller * Fix broken test * Address Kai's feedback

lfkellogg added a commit that referenced this pull request Jan 25, 2023

Remove synchronized blocks from ApkInstaller (#4486)

872e586

* Remove synchronized blocks from ApkInstaller * Fix broken test * Address Kai's feedback

lfkellogg added a commit that referenced this pull request Jan 25, 2023

Remove synchronized blocks from ApkInstaller (#4486)

5c70722

* Remove synchronized blocks from ApkInstaller * Fix broken test * Address Kai's feedback

firebase locked and limited conversation to collaborators Feb 4, 2023

		@@ -84,12 +69,9 @@ private void startInstallActivity(String path, Activity currentActivity) {
		}

		void trySetInstallTaskError() {

		public void
		setCurrentActivity_appInForegroundAfterAnInstallAttempt_installIntentOnCurrentActivity() {

		firebaseAppDistribution.signOutTester();
		awaitAsyncOperations(backgroundExecutor);

Remove synchronized blocks from ApkInstaller #4486

Remove synchronized blocks from ApkInstaller #4486

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-bot commented Dec 22, 2022

Uh oh!

google-oss-bot commented Dec 22, 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!

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

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Dec 22, 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!

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

Uh oh!

Startup Time Report 1

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lfkellogg commented Dec 22, 2022 •

edited

Loading

google-oss-bot commented Dec 22, 2022 •

edited

Loading

Coverage Report ¹

`firebase-appdistribution`

github-actions bot commented Dec 22, 2022 •

edited

Loading

google-oss-bot commented Dec 22, 2022 •

edited

Loading

Size Report ¹

`base`

`firebase-annotations`

`firebase-appdistribution`

`firebase-appdistribution-api`

`firebase-common`

`firebase-components`

`firebase-installations`

`firebase-installations-interop`

google-oss-bot commented Dec 22, 2022 •

edited

Loading

Startup Time Report ¹