Skip to content

Allow AAB install to be started from a background thread #3348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 25, 2022
Merged

Allow AAB install to be started from a background thread #3348

merged 2 commits into from
Jan 25, 2022

Conversation

lfkellogg
Copy link
Contributor

No description provided.

lfkellogg
lfkellogg commented Jan 25, 2022
View reviewed changes
firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java
@GuardedBy("updateAabLock")
private UpdateTaskImpl cachedUpdateTask;

@GuardedBy("updateAabLock")
private AppDistributionReleaseInternal aabReleaseInProgress;

private final Object updateAabLock = new Object();
private final Context context;
Copy link
Contributor Author

@lfkellogg lfkellogg Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This context reference was unused

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 25, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-appdistribution

    Overall coverage changed from 73.67% (522ce69) to 73.75% (d5ad4e4) by +0.08%.

    FilenameBase (522ce69)Merge (d5ad4e4)Diff
    AabUpdater.java98.59%98.68%+0.09%
    FirebaseAppDistribution.java95.68%95.65%-0.02%

Test Logs

Notes

  • Commit (d5ad4e4) is created by Prow via merging PR base commit (522ce69) and head commit (27f5ebb).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

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

@lfkellogg lfkellogg force-pushed the lk/aab-intent-from-background branch from 5c74757 to c7c34e0 Compare January 25, 2022 17:24
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 25, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-appdistribution

    TypeBase (522ce69)Merge (d5ad4e4)Diff
    aar124 kB125 kB+635 B (+0.5%)
    apk (aggressive)741 kB741 kB-112 B (-0.0%)
    apk (release)1.55 MB1.55 MB+208 B (+0.0%)

Test Logs

Notes

  • Commit (d5ad4e4) is created by Prow via merging PR base commit (522ce69) and head commit (27f5ebb).

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

manny-jimenez
manny-jimenez reviewed Jan 25, 2022
View reviewed changes
firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/AabUpdaterTest.java Outdated
@@ -100,7 +95,7 @@ public void updateAppTask_whenOpenConnectionFails_setsNetworkFailure()
when(mockHttpsUrlConnectionFactory.openConnection(TEST_URL)).thenThrow(caughtException);

UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
testExecutor.awaitTermination(1, TimeUnit.SECONDS);
testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there some timeout issue that you changed the waiting from 1 second to 100 ms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to reduce the maximum waiting time of running these tests. 1 second seems like overkill.

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java Outdated
// On a background thread, fetch the redirect URL and open it in the Play app
runAsyncInTask(executor, () -> fetchDownloadRedirectUrl(newRelease.getDownloadUrl()))
.onSuccessTask(
executor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for this chain of tasks we use this executor but we don't do this elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. We actually do this inconsistently. For example ApkUpdater uses its own executor, while TesterSignInManager does not (so callbacks run on the main thread).

In this case since we're already using it for the fetchDownloadRedirectUrl it seemed to make sense to use it everywhere. This also makes testing easier for the reasons we were discussing earlier. Robolectric unit tests run on the main thread, so awaiting Tasks to finish blocks execution of callbacks that also run on the main thread.

I think we should come up with a more consistent approach though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually went ahead and removed these, since it was confusing. Got the tests to pass as well.

@lfkellogg lfkellogg force-pushed the lk/aab-intent-from-background branch from 2f35adb to 2a3725c Compare January 25, 2022 20:05
@lfkellogg lfkellogg force-pushed the lk/aab-intent-from-background branch from 2a3725c to 27f5ebb Compare January 25, 2022 20:37
@lfkellogg
Copy link
Contributor Author

/test check-changed

1 similar comment
@lfkellogg
Copy link
Contributor Author

/test check-changed

@lfkellogg lfkellogg merged commit aafced0 into master Jan 25, 2022
@lfkellogg lfkellogg deleted the lk/aab-intent-from-background branch January 25, 2022 21:33
@firebase firebase locked and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@manny-jimenez manny-jimenez manny-jimenez approved these changes

@mrichards mrichards Awaiting requested review from mrichards

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lfkellogg @google-oss-bot @manny-jimenez