Skip to content

Migrate some executors in fad/in-app-feedback #4350

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

Merged
merged 2 commits into from
Dec 20, 2022
Merged

Migrate some executors in fad/in-app-feedback #4350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec07e81
Migrate executors in fad/in-app-feedback
lfkellogg Dec 7, 2022
8e75ef4
Add an import and format
lfkellogg Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions ...n/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
private final ReleaseIdentifier releaseIdentifier;
private final ScreenshotTaker screenshotTaker;
private final Executor taskExecutor;
private final Executor blockingExecutor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requires blocking executor here because some of the uses may wait on a lock via synchronized blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vkryachko Some of the uses in this file do not block at all. Would you recommend that we also inject the Lightweight executor, or is it up to us if we're OK using the Blocking executor for lighter weight tasks?

Copy link
Member

@vkryachko vkryachko Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

may wait on a lock via synchronized blocks

Do any of these blocks do network or disk IO synchronously or call task.get()? or are they all in memory operations?

Copy link
Member

Choose a reason for hiding this comment

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

(optional)

Additionally, speaking of synchronized blocks in the context of tasks and executors, consider refactoring to something like the following go/android-concurrent/whichexecutor#sequentialexecutor, where code that needs synchronization could run on the sequential "decorator", and code that does not can run on the executor directly.

The caveat is that we can't use ListenableFutures as described in the doc above and have to use Tasks instead. see TaskCompletionSource on how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

I've also noticed that for some continuations in this file you are not passing in the executor, i.e. onSuccessTask, continueWithTask etc. This results in the continuations to run on the main thread, is that intentional?

Copy link
Member

Choose a reason for hiding this comment

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

Do any of these blocks do network or disk IO synchronously or call task.get()? or are they all in memory operations?

Actually looking closer .signInTester() accesses SharedPreferences, which I understand could in theory touch disk.

Disk it touched only when the SharedPreferences object is constructed, and for any synchronous writes like commit, so those operations should use the @Background executor as using an unbounded pool(i.e. @Blocking) for disk io puts a lot of pressure on the file system.

Besides that the other potential blocking calls should all be in memory. They involve synchronized blocks (like this one), which in theory shouldn't block for long if at all, but could block briefly depending on various race conditions.

In this case it's best to use a blocking executor since it's unbounded in the number of threads which avoids dead locks, but it's better to refactor to a sequential as mentioned below.

I assume logging using android.util.Log does not block but maybe that's wishful thinking...

It's safe to log and it does not block.

Additionally, speaking of synchronized blocks in the context of tasks and executors, consider refactoring to something like the following go/android-concurrent/whichexecutor#sequentialexecutor

That looks great, we'll look into that. It doesn't feel great having synchronized blocks all over the place right now.

I've also noticed that for some continuations in this file you are not passing in the executor, i.e. onSuccessTask, continueWithTask etc. This results in the continuations to run on the main thread, is that intentional?

I see a couple of them are accidental, actually. I'll fix those. Others though are intentional, if not ideal. We run a lot of Task handlers on the main thread if they should execute quickly. But that's mostly for convenience, rather than create an executor to run something that should return immediately. Maybe in this new world of explicit shared executors we should use @Lightweight for these.

In my opinion this is a bad default that Tasks provide(even though at the time it was a reasonable thing to do), the general android guidance is to run as little code on the UI thread as possible, i.e. only things that must run there, like UI related code. Given this default, I agree that using executors explicitly in all continuations is a better approach and we should even add a linter warning for calls without an executor(maybe?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks Vlad, in summary we will:

  • Move construction of SharedPreferences or synchronous writes to a @Background executor
  • Avoid using synchronized blocks in favor of sequential executors or other approaches (already removed some in Remove some synchronized blocks from FirebaseAppDistributionImpl #4353)
  • Use explicit executors for Task handlers throughout instead of defaulting to UI thread

Copy link
Member

@vkryachko vkryachko Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Lee,

Move construction of SharedPreferences or synchronous writes to a @Background executor

One thing I'll add: it would be awesome if you could implement this as a self-contained class that just does the right thing, that way we would be able to put it in say firebase-common, register it as a component, and have all teams benefit from it. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

As for the UI thread, looks like all sdks use it a lot :( #4363

So it could be a follow up effort once the executor migration is done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'm going to make each of these changes in followup PRs.

private final FirebaseAppDistributionNotificationsManager notificationsManager;
private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
Expand All @@ -94,7 +94,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
@NonNull ReleaseIdentifier releaseIdentifier,
@NonNull ScreenshotTaker screenshotTaker,
@NonNull @Lightweight Executor lightweightExecutor,
@NonNull Executor taskExecutor) {
@NonNull Executor blockingExecutor) {
this.firebaseApp = firebaseApp;
this.testerSignInManager = testerSignInManager;
this.newReleaseFetcher = newReleaseFetcher;
Expand All @@ -106,7 +106,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
this.lightweightExecutor = lightweightExecutor;
this.screenshotTaker = screenshotTaker;
this.taskExecutor = taskExecutor;
this.blockingExecutor = blockingExecutor;
this.notificationsManager =
new FirebaseAppDistributionNotificationsManager(firebaseApp.getApplicationContext());
lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
Expand Down Expand Up @@ -332,13 +332,13 @@ public void startFeedback(@NonNull CharSequence infoText) {
screenshotTaker
.takeScreenshot()
.addOnFailureListener(
taskExecutor,
blockingExecutor,
e -> {
LogWrapper.w(TAG, "Failed to take screenshot for feedback", e);
doStartFeedback(infoText, null);
})
.addOnSuccessListener(
taskExecutor, screenshotUri -> doStartFeedback(infoText, screenshotUri));
blockingExecutor, screenshotUri -> doStartFeedback(infoText, screenshotUri));
}

@Override
Expand Down Expand Up @@ -378,17 +378,18 @@ private void doStartFeedback(CharSequence infoText, @Nullable Uri screenshotUri)
testerSignInManager
.signInTester()
.addOnFailureListener(
taskExecutor,
blockingExecutor,
e -> {
feedbackInProgress.set(false);
LogWrapper.e(TAG, "Failed to sign in tester. Could not collect feedback.", e);
})
.onSuccessTask(
taskExecutor,
blockingExecutor,
unused ->
releaseIdentifier
.identifyRelease()
.addOnFailureListener(
blockingExecutor,
e -> {
feedbackInProgress.set(false);
LogWrapper.e(TAG, "Failed to identify release", e);
Expand All @@ -399,11 +400,12 @@ private void doStartFeedback(CharSequence infoText, @Nullable Uri screenshotUri)
.show();
})
.onSuccessTask(
taskExecutor,
blockingExecutor,
releaseName ->
// in development-mode the releaseName might be null
launchFeedbackActivity(releaseName, infoText, screenshotUri)
.addOnFailureListener(
blockingExecutor,
e -> {
feedbackInProgress.set(false);
LogWrapper.e(TAG, "Failed to launch feedback flow", e);
Expand Down
21 changes: 13 additions & 8 deletions .../main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@

package com.google.firebase.appdistribution.impl;

import android.annotation.SuppressLint;
import android.app.Application;
import android.content.Context;
import androidx.annotation.Keep;
import androidx.annotation.NonNull;
import com.google.firebase.FirebaseApp;
import com.google.firebase.annotations.concurrent.Background;
import com.google.firebase.annotations.concurrent.Blocking;
import com.google.firebase.annotations.concurrent.Lightweight;
import com.google.firebase.appdistribution.FirebaseAppDistribution;
Expand All @@ -34,10 +34,9 @@
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;

/**
* Registers FirebaseAppDistribution.
* Registers FirebaseAppDistribution and related components.
*
* @hide
*/
Expand All @@ -49,6 +48,7 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {

@Override
public @NonNull List<Component<?>> getComponents() {
Qualified<Executor> backgroundExecutor = Qualified.qualified(Background.class, Executor.class);
Qualified<Executor> blockingExecutor = Qualified.qualified(Blocking.class, Executor.class);
Qualified<Executor> lightweightExecutor =
Qualified.qualified(Lightweight.class, Executor.class);
Expand All @@ -57,16 +57,21 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
.name(LIBRARY_NAME)
.add(Dependency.required(FirebaseApp.class))
.add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
.add(Dependency.required(backgroundExecutor))
.add(Dependency.required(blockingExecutor))
.add(Dependency.required(lightweightExecutor))
.factory(c -> buildFirebaseAppDistribution(c, blockingExecutor, lightweightExecutor))
.factory(
c ->
buildFirebaseAppDistribution(
c, backgroundExecutor, blockingExecutor, lightweightExecutor))
// construct FirebaseAppDistribution instance on startup so we can register for
// activity lifecycle callbacks before the API is called
.alwaysEager()
.build(),
Component.builder(FeedbackSender.class)
.add(Dependency.required(FirebaseApp.class))
.add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
.add(Dependency.required(backgroundExecutor))
.add(Dependency.required(blockingExecutor))
.factory(c -> buildFeedbackSender(c, c.get(blockingExecutor)))
.build(),
Expand All @@ -87,13 +92,13 @@ private FeedbackSender buildFeedbackSender(
return new FeedbackSender(testerApiClient, blockingExecutor);
}

// TODO(b/258264924): Migrate to go/firebase-android-executors
@SuppressLint("ThreadPoolCreation")
private FirebaseAppDistribution buildFirebaseAppDistribution(
ComponentContainer container,
Qualified<Executor> backgroundExecutorType,
Qualified<Executor> blockingExecutorType,
Qualified<Executor> lightweightExecutorType) {
FirebaseApp firebaseApp = container.get(FirebaseApp.class);
Executor backgroundExecutor = container.get(backgroundExecutorType);
Executor blockingExecutor = container.get(blockingExecutorType);
Executor lightweightExecutor = container.get(lightweightExecutorType);
Context context = firebaseApp.getApplicationContext();
Expand All @@ -120,9 +125,9 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
signInStorage,
lifecycleNotifier,
releaseIdentifier,
new ScreenshotTaker(firebaseApp, lifecycleNotifier),
new ScreenshotTaker(firebaseApp, lifecycleNotifier, backgroundExecutor),
lightweightExecutor,
Executors.newSingleThreadExecutor());
blockingExecutor);

if (context instanceof Application) {
Application firebaseApplication = (Application) context;
Expand Down
19 changes: 5 additions & 14 deletions ...pdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;

/** A class that takes screenshots of the host app. */
class ScreenshotTaker {
Expand All @@ -47,23 +46,15 @@ class ScreenshotTaker {

private final FirebaseApp firebaseApp;
private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
private final Executor taskExecutor;
private final Executor backgroundExecutor;

// TODO(b/258264924): Migrate to go/firebase-android-executors
@SuppressLint("ThreadPoolCreation")
ScreenshotTaker(
FirebaseApp firebaseApp, FirebaseAppDistributionLifecycleNotifier lifecycleNotifier) {
this(firebaseApp, lifecycleNotifier, Executors.newSingleThreadExecutor());
}

@VisibleForTesting
ScreenshotTaker(
FirebaseApp firebaseApp,
FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
Executor taskExecutor) {
Executor backgroundExecutor) {
this.firebaseApp = firebaseApp;
this.lifecycleNotifier = lifecycleNotifier;
this.taskExecutor = taskExecutor;
this.backgroundExecutor = backgroundExecutor;
}

/**
Expand All @@ -83,7 +74,7 @@ Task<Uri> takeScreenshot() {
/** Deletes any previously taken screenshot. */
Task<Void> deleteScreenshot() {
return TaskUtils.runAsyncInTask(
taskExecutor,
backgroundExecutor,
() -> {
firebaseApp.getApplicationContext().deleteFile(SCREENSHOT_FILE_NAME);
return null;
Expand Down Expand Up @@ -159,7 +150,7 @@ private Task<Uri> writeToFile(@Nullable Bitmap bitmap) {
return Tasks.forResult(null);
}
return TaskUtils.runAsyncInTask(
taskExecutor,
backgroundExecutor,
() -> {
try (FileOutputStream outputStream = openFileOutputStream()) {
// PNG is a lossless format, the compression factor (100) is ignored
Expand Down
3 changes: 3 additions & 0 deletions ...appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ class SignInStorage {
this.signInSharedPreferences =
new Lazy(
() ->
// TODO(lkellogg): This constructs a SharedPreferences object, which touches disk
// and therefore should be run on a @Background executor. This could ideally be
// done by a new shared component.
applicationContext.getSharedPreferences(
SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE));
}
Expand Down