Android Background Platform Channels #29147

gaaclarke · 2021-10-13T00:04:36Z

issue flutter/flutter#91635 (fixed for android)
google issue: b/203443395

Everything has unit tests. There aren't integration tests since the only possible test harness would be scenario app which we are removing. All existing platform channels go through this code so broken existing behavior is covered with existing integration tests and performance tests like platform_channels_benchmarks

In a separate PR I plan on expanding the benchmarks tests to cover background task queues too (the the flutter repo).

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2021-10-13T23:23:05Z

This now works, tested manually with:

public class AndroidPlugin implements FlutterPlugin, MethodCallHandler {
  private MethodChannel channel;

  @Override
  public void
  onAttachedToEngine(@NonNull FlutterPluginBinding flutterPluginBinding) {
    BinaryMessenger.TaskQueue taskQueue =
        flutterPluginBinding.getBinaryMessenger().makeBackgroundTaskQueue();
    channel = new MethodChannel(flutterPluginBinding.getBinaryMessenger(),
                                "android_plugin", StandardMethodCodec.INSTANCE,
                                taskQueue);
    channel.setMethodCallHandler(this);
  }

  @Override
  public void onMethodCall(@NonNull MethodCall call, @NonNull Result result) {
    if (call.method.equals("getPlatformVersion")) {
      result.success("Android " + android.os.Build.VERSION.RELEASE);
    } else {
      result.notImplemented();
    }
  }

  @Override
  public void onDetachedFromEngine(@NonNull FlutterPluginBinding binding) {
    channel.setMethodCallHandler(null);
  }
}

gaaclarke · 2021-10-13T23:28:53Z

@dnfield We have a functioning implementation for Android. Feel free to take an early look.

Outstanding work:

Tests, this is going to be a doozy. We don't have end to end platform channel tests in the engine.
Docstrings
Probably going to rename PlatformMessageHandler to PlatformMessageHandlerAndroid
Outstanding question about the API and the best way for users to get a TaskQueue (helper function versus factory method on BinaryMessenger).
Breaking Change notice since we are adding a method to BinaryMessenger (very unlikely this affects anyone).
(optimization) move the ui thread hop up from the completer to the PlatformMessageHandler to remove the mutex
Turn my comments about what thread executes what into asserts, which requires plumbing task runners.

dnfield

Just starting with the core engine class - havne't looked super closely at the other stuff just yet.

shell/common/platform_view.h

shell/common/shell.cc

shell/platform/android/platform_message_handler.cc

shell/common/shell.cc

shell/platform/android/android_shell_holder.cc

shell/platform/android/platform_message_handler.cc

jason-simmons · 2021-10-14T21:07:38Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

@@ -858,14 +858,23 @@ public void setPlatformMessageHandler(@Nullable PlatformMessageHandler platformM
    this.platformMessageHandler = platformMessageHandler;
  }

+  private native void nativeCleanupMessageData(long messageData);
+
+  public void cleanupMessageData(long messageData) {


Make this a static method

I think we're avoiding that to make it easier to mock this out in testing, right?

Although I also wonder if this shoul dbe on a separate class from FlutterJNI. That might be deferred to a separate PR, but we'd have to make sure this gets called from the right thread, and the right thread might not be the platform thread, in which case other methods on flutterJNI won't work.

I'm just following the established pattern. I know in java it's frowned upon to have static functions. Is there any concern here? This call can happen on any thread, at this point we know we have exclusive control over the message data because of the refactor I did previously moving messages to std::unique_ptr.

shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

gaaclarke · 2021-10-15T16:52:41Z

Thanks for taking an early look, I'll look through everything closer this afternoon. I was OOO and still have a weird schedule because my family is sick. I know there is some work to do to make this threadsafe.

We need to make sure the engine doesn't deconstruct when executing the reply on the background thread (maybe moving the reply to the platform thread which really isn't ideal)
We need to make sure that calling into the PlatformMessageHandler isn't routed through the PlatformView since that could also be deleted on the platform thread while we are accessing the PlatformMessageHandler from him.

I have a few outstanding issues I have to circle back to then come back to this.

dnfield · 2021-10-15T21:37:40Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

+  // detach FlutterJNI on the platform thread while a background thread invokes
+  // a message response.  Typically accessing the shell holder happens on the
+  // platform thread and doesn't require locking.
+  private ReentrantReadWriteLock shellHolderLock = new ReentrantReadWriteLock();


Rather than this, could we just have a different Java object that the UI thread works with?

We could avoid the lock on this side completely if we used API similar to FlutterEnginePostDartObject in the embedder API.

That may be possible. So this is protecting the engine from being deleted on the platform thread while a reply is being invoked in the engine from some arbitrary thread, which will eventually post to the ui thread. We could percolate up the hop to the ui thread, but even if we were executing here on the ui thread we'd have to have a mutex to make sure the platform thread doesn't change the shellHolder while the ui thread is accessing it.

I think in practice that might be ok to not have the mutex since the ui thread should be shutdown as part of deleting the engine on the platform thread. We'd have to plumb through a way to dispatch to the ui thread from java though and that approach relies on logic that exists somewhere else to protect us, instead of just being able to read through the code and knowing it's safe. It's harder to reason about and prove it's correct.

It's not clear to me that reading this lets us know its safe (you still have to assume some things about the platform/UI thread lifecycles), and it's a little confusing to have all but one method on this class be called exclusively from the platform thread.

It's probably fine to at least start with the mutex and see if we can get rid of it later, but I think I'd still feel more comfortable with this being a different Java object, even if only to make sure that the caller(s) of this method don't get confused about which other methods they can call and only find out at runtime that they're wrong.

part 1: moved dispatch to embedder

PlatformMessageHandlerAndroid This eliminates the potential race condition where we are calling into a platform view when it is being deallocated (That may not be possible in practice but this is safer).

attaching, deatching and invoking the reply with locks.

2) remove it's ownership from android_shell_holder, now just shell and the platform_view have a reference

shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java

jason-simmons · 2021-10-19T20:11:05Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

@@ -98,6 +99,12 @@
 @Keep
 public class FlutterJNI {
  private static final String TAG = "FlutterJNI";
+  // This serializes the invocation of platform message responses and the


Why does the shellHolderLock need to be reentrant?

That's the only concrete implementation of ReadWriteLock in the jdk.

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java

gaaclarke · 2021-10-20T22:53:36Z

Okay, @dnfield. I think you'll like this change. Now the TaskQueue definition looks like this:

public interface TaskQueue {}

There is no way that a user can make their own TaskQueue, and when they get a TaskQueue they can't execute anything on it. This is the most locked down we can make it.

nt4f04uNd · 2021-10-21T14:04:44Z

shell/platform/android/io/flutter/plugin/common/BinaryMessenger.java

-  void setMessageHandler(@NonNull String channel, @Nullable BinaryMessageHandler handler);
+  void setMessageHandler(
+      @NonNull String channel,
+      @Nullable BinaryMessageHandler handler,
+      @Nullable TaskQueue taskQueue);


This is a breaking change, right? It is possible to keep the old method as an overload, by analogy with EventChannel constructors? (here and elsewhere)

This change is unavoidable because BinaryMessenger is an interface, a hypothetical implementor of the BinaryMessenger would have to implement the new method or accommodate the new parameter, same difference.

See https://flutter.dev/docs/resources/compatibility

The criteria is whether this breaks a test, including contributed tests.

(Or rather, especially contributed tests, including tests in Google's internal monorepo).

IOW, this can probably break some people, but if they didn't contribute tests for it, we don't really know, and so we'll just make the change.

Turns out it breaks flutter/plugins tests, which should probably qualify it as breaking.

+1 for using default implementation. Isn't every plugin broken by this change?

(We'd still need default for makeBackgroundTaskQueue).

Only unit tests are breaking, because they're expecting method signatures that are no longer valid.

And in particular, only unit tests that mock the binary messenger itself.

ah - got it

There is one legitimate breakage in production code for an internal customer. They have a class that wraps up a BinaryMessenger, but doesn't implement the BinaryMessenger interface. Splitting up setMessageHandler and setTaskQueue would avoid the problem. I still think the breaking change is better since it leads to cleaner bookkeeping instead of creating a more complicated bookkeeping to accommodate very rare usage, where the fix is trivial. Setting the handler and the thread it executes on should be an atomic choice.

dnfield · 2021-10-21T21:05:43Z

shell/platform/android/io/flutter/plugin/common/BinaryMessenger.java

@@ -26,6 +26,18 @@
 * @see EventChannel , which supports communication using event streams.
 */
 public interface BinaryMessenger {
+  /** An abstraction over dispatching a Runnable to be executed on some thread. */


Consider documenting a bit more about how/where this is used.

nit: instead of an abstraction, maybe something like An opaque dispatching interface for servicing plugin method calls or something.

Done, I don't want to specifically say it's opaque. That might not be true in the future. (edit, almost done, waiting for the local checks fyi).

dnfield

LGTM with nit about doc on TaskQueue.

This reverts commit 67c8f20.

This reverts commit 7ed91e14ccc1f5ffc4cb13d1d37e27d370c8f7cd.

* Revert "Revert "Android Background Platform Channels (#29147)"" This reverts commit 7ed91e14ccc1f5ffc4cb13d1d37e27d370c8f7cd. * Made this PR less of a breaking change by keeping setMessageHandler's signature the same

flutter-dashboard bot added the platform-android label Oct 13, 2021

google-cla bot added the cla: yes label Oct 13, 2021

gaaclarke force-pushed the background-platform-channels-android branch 7 times, most recently from 222c2d2 to 3b19ead Compare October 13, 2021 20:23

dnfield reviewed Oct 14, 2021

View reviewed changes

shell/common/platform_view.h Outdated Show resolved Hide resolved

shell/common/shell.cc Outdated Show resolved Hide resolved

jason-simmons reviewed Oct 14, 2021

View reviewed changes

dnfield reviewed Oct 15, 2021

View reviewed changes

gaaclarke added 13 commits October 19, 2021 11:32

Implemented background platform channels for Android

9034cb9

part 1: moved dispatch to embedder

Part 2: Introduced TaskQueue API

68c41df

Part 3: Introduced PlatformMessageHandler

c8de4a1

introduced PlatformMessageHandler, renamed PlatformMessageHandler to

19bd38c

PlatformMessageHandlerAndroid This eliminates the potential race condition where we are calling into a platform view when it is being deallocated (That may not be possible in practice but this is safer).

Made it safe to invoke the reply on background threads by protecting

fb4402e

attaching, deatching and invoking the reply with locks.

jasons feedback

2471665

added android embedder test

fb4f77f

1) removed javaism from the platform_message_handler

9b3453d

2) remove it's ownership from android_shell_holder, now just shell and the platform_view have a reference

added shell unittest

d8b1dbe

added java unittests

3ded42e

some random cleanup

a3e5eb5

updated licenses

30ca22e

added docstrings

734f5d8

gaaclarke force-pushed the background-platform-channels-android branch from 3992cc4 to 734f5d8 Compare October 19, 2021 18:40

gaaclarke marked this pull request as ready for review October 19, 2021 18:49

jason-simmons reviewed Oct 20, 2021

View reviewed changes

gaaclarke added 2 commits October 20, 2021 09:44

jason's feedback

b9a87c6

started naming generated threads

76d6c8f

jason-simmons reviewed Oct 20, 2021

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java Outdated Show resolved Hide resolved

gaaclarke added 2 commits October 20, 2021 13:44

submit -> execute

56b2023

blocked users from using their own taskqueues

69c47f6

gaaclarke requested a review from dnfield October 20, 2021 22:53

nt4f04uNd reviewed Oct 21, 2021

View reviewed changes

dnfield reviewed Oct 21, 2021

View reviewed changes

dnfield approved these changes Oct 21, 2021

View reviewed changes

updated docstring

141520f

gaaclarke added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 21, 2021

jason-simmons approved these changes Oct 21, 2021

View reviewed changes

fluttergithubbot merged commit 67c8f20 into flutter:master Oct 21, 2021

engine-flutter-autoroll mentioned this pull request Oct 22, 2021

Roll Engine from d8a54333e57b to e898106f5306 (10 revisions) flutter/flutter#92308

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2021

67c8f20 Android Background Platform Channels (flutter/engine#29147)

4443f9d

engine-flutter-autoroll mentioned this pull request Oct 22, 2021

Roll Engine from d8a54333e57b to e898106f5306 (10 revisions) flutter/flutter#92311

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2021

67c8f20 Android Background Platform Channels (flutter/engine#29147)

5f2d546

kylinchen pushed a commit to XianyuTech/engine that referenced this pull request Oct 22, 2021

Android Background Platform Channels (flutter#29147)

df4fb3b

gaaclarke mentioned this pull request Oct 25, 2021

upgraded usage of BinaryMessenger flutter/plugins#4451

Merged

11 tasks

dnfield added a commit that referenced this pull request Oct 26, 2021

Revert "Android Background Platform Channels (#29147)"

fcd6ec5

This reverts commit 67c8f20.

dnfield mentioned this pull request Oct 26, 2021

Revert "Android Background Platform Channels" #29344

Merged

gaaclarke pushed a commit that referenced this pull request Oct 26, 2021

Revert "Android Background Platform Channels (#29147)" (#29344)

c076baa

This reverts commit 67c8f20.

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Oct 26, 2021

Revert "Revert "Android Background Platform Channels (flutter#29147)""

47409d5

This reverts commit 7ed91e14ccc1f5ffc4cb13d1d37e27d370c8f7cd.

gaaclarke mentioned this pull request Nov 20, 2021

documented background platform channels flutter/website#6494

Merged

3 tasks

guillermin mentioned this pull request Dec 15, 2021

[path_provider] Android implementation of getDownloadsDirectory flutter/plugins#4559

Closed

11 tasks

Nayuta403 mentioned this pull request Jul 26, 2022

[Multiple Flutter] Occasional MissingPluginException flutter/flutter#103483

Open

cbracken mentioned this pull request Aug 24, 2022

Windows: Client Wrapper: Expose TaskRunner #31134

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android Background Platform Channels #29147

Android Background Platform Channels #29147

gaaclarke commented Oct 13, 2021 •

edited

Loading

gaaclarke commented Oct 13, 2021

gaaclarke commented Oct 13, 2021

dnfield left a comment

jason-simmons Oct 14, 2021

dnfield Oct 14, 2021

dnfield Oct 14, 2021

gaaclarke Oct 15, 2021

gaaclarke commented Oct 15, 2021

dnfield Oct 15, 2021

gaaclarke Oct 15, 2021 •

edited

Loading

dnfield Oct 15, 2021

jason-simmons Oct 19, 2021

gaaclarke Oct 20, 2021

gaaclarke commented Oct 20, 2021

nt4f04uNd Oct 21, 2021

gaaclarke Oct 21, 2021

dnfield Oct 21, 2021

dnfield Oct 21, 2021

stuartmorgan-g Oct 25, 2021

blasten Oct 25, 2021

dnfield Oct 25, 2021

stuartmorgan-g Oct 25, 2021

blasten Oct 25, 2021

gaaclarke Oct 26, 2021

dnfield Oct 21, 2021

gaaclarke Oct 21, 2021 •

edited

Loading

dnfield left a comment

Android Background Platform Channels #29147

Android Background Platform Channels #29147

Conversation

gaaclarke commented Oct 13, 2021 • edited Loading

Pre-launch Checklist

gaaclarke commented Oct 13, 2021

gaaclarke commented Oct 13, 2021

dnfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Oct 15, 2021

Choose a reason for hiding this comment

gaaclarke Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

gaaclarke commented Oct 13, 2021 •

edited

Loading

gaaclarke Oct 15, 2021 •

edited

Loading

gaaclarke Oct 21, 2021 •

edited

Loading