Allow platform isolate to call background isolate

[BuzzBumbleBee]

This change is to allow platform code to call background isolates and remove the UI isolate only limitation.

This is done by registering a callback to the background isolate send port within the platform message handler. If this callback is set on a specific channel the message will be dispatched via the callbacks send port

Fixes : https://github.com/flutter/flutter/issues/119207

ToDo :

Add documentation Add iOS platform handle imp Add Linux platform handle imp Add Windows platform handle imp

Example Isolate using this addition: (using flutter_blue_plus for platform code)

  static Future<void> testIsolate(RootIsolateToken tkn) async {
    BackgroundIsolateBinaryMessenger.ensureInitialized(tkn);
    var backgroundIsolateBinaryMessenger = BackgroundIsolateBinaryMessenger.instance as BackgroundIsolateBinaryMessenger;
    backgroundIsolateBinaryMessenger.registerIsolateCallback("flutter_blue_plus/methods");

    print("HelloFromIsolate");

    var subscription = FlutterBluePlus.onScanResults.listen((results) {
        if (results.isNotEmpty) {
          ScanResult r = results.last; // the most recently found device
          print('${r.device.remoteId}: "${r.advertisementData.advName}" found!');
        }
      },
      onError: (e) => print(e),
    );
    FlutterBluePlus.cancelWhenScanComplete(subscription);

    await FlutterBluePlus.adapterState.where((val) => val == BluetoothAdapterState.on).first;
    await FlutterBluePlus.startScan(timeout: Duration(seconds:5));
    await FlutterBluePlus.isScanning.where((val) => val == false).first;

    var i = 10;
    while (i > 0) {
      Future.delayed(Duration(seconds: 2));
      i = i -1;
    }

    backgroundIsolateBinaryMessenger.removeIsolateCallback("flutter_blue_plus/methods");
  }

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] 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.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[BuzzBumbleBee]

Sorry need to rebase on master. Will do asap

[flutter-dashboard[bot]]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[BuzzBumbleBee]

I have rebased this now and am looking for some early feedback on if this sort of implementation would be accepted ?

If as a concept it ok ill continue to add docs / tests ect

[gaaclarke]

Your description talks about an api registerIsolateCallback but that doesn't exist in the PR. Can you update your description?

A hard requirement for this feature is going to be compatibility with the platform channel classes (of particular interest will be the EventChannels). So we don't want to roll out this feature without support there. That means making sure to use the BinaryMessenger API.

[BuzzBumbleBee]

@gaaclarke

Sorry this also has a flutter public API change that I missed (will push on Friday)

But the API changes add 2 methods to BackgroundIsolateBinaryMessenger

One to register the current isolate to receive all messages for a specified channel

Another to remove that registration and restore old behaviour (all messages from platform goes to UI isolate)

The rationale behind that was that this has no impact on the current behaviour until you registered your isolate for a specific channel.

My main concern with this MR tho was to get some insight on if the implementation of passing messages to and from the platform and background isolate was suitable

[BuzzBumbleBee]

here is the non engine change needed for my MR

https://github.com/BuzzBumbleBee/flutter/commit/abf0fb84c67be3fb7de7d77cdddd6202a71df125

it adds the logic behind setMessageHandler and adds a 2nd ReceivePort that persistently listens for incoming platform messages (this could potentially be reworked to reuse the same send port as the responses to send)

[stuartmorgan-g]

It's there a design doc for this proposal? That seems like a better structure than a PR for high level discussion/feedback here.

[BuzzBumbleBee]

@stuartmorgan what's the normal forum to share design documentation?

I'm happy to put something together for this implementation

[stuartmorgan-g]

https://github.com/flutter/flutter/wiki/Design-Documents

[BuzzBumbleBee]

@stuartmorgan started a design doc here https://docs.google.com/document/d/1ebFrip3jv9xMAL8VdhIS_88wiI13XeH4QajOBBIeaQM/edit?usp=sharing

[flutter-dashboard[bot]]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

[gaaclarke]

I don't recommend this PR because it expands the API surface for the services package in a way that is complicated. The complication should be bore by the flutter developers not the flutter users.

For example, the usage of this PR looks like this:

  static Future<void> testIsolate(RootIsolateToken tkn) async {
    BackgroundIsolateBinaryMessenger.ensureInitialized(tkn);
    EventChannel eventChannel = const EventChannel('timeHandlerEvent');

    var backgroundIsolateBinaryMessenger = BackgroundIsolateBinaryMessenger.instance as BackgroundIsolateBinaryMessenger;
    backgroundIsolateBinaryMessenger.registerIsolateCallback("timeHandlerEvent");

    eventChannel.receiveBroadcastStream().listen((event) {
      print("evt isolate : $event");
    });

    print("HelloFromIsolate");


    var i = 20;
    while (i > 0) {
      await Future.delayed(const Duration(seconds: 2));
      i = i -1;
    }
    print("CyaFromIsolate");

    backgroundIsolateBinaryMessenger.removeIsolateCallback("timeHandlerEvent");
  }

I don't believe there is a reason why we couldn't make it look like this, other than it is more difficult to implement:

  static Future<void> testIsolate(RootIsolateToken tkn) async {
    BackgroundIsolateBinaryMessenger.ensureInitialized(tkn);
    EventChannel eventChannel = const EventChannel('timeHandlerEvent');

    eventChannel.receiveBroadcastStream().listen((event) {
      print("evt isolate : $event");
    });

    print("HelloFromIsolate");

    var i = 20;
    while (i > 0) {
      await Future.delayed(const Duration(seconds: 2));
      i = i -1;
    }
    print("CyaFromIsolate");
  }

The services API is the most stable and difficult to change library in Flutter and a lot of care must be taken to when modifying it since any breaking change could render plugins inoperable.

[BuzzBumbleBee]

@gaaclarke I can adapt the change for the proposed API, my question would be.

Is there any lifecycle hook on the isolate that would be called just before the thread has stopped. As ideally we would tidy up the registered isolate listeners automatically when its processing.

This was the main reason for having the manual register and remove calls

I'm not really attached to the API as much getting the overall functionality into flutter :)

[gaaclarke]

@gaaclarke I can adapt the change for the proposed API, my question would be.

Is there any lifecycle hook on the isolate that would be called just before the thread has stopped. As ideally we would tidy up the registered isolate listeners automatically when its processing.

This was the main reason for having the manual register and remove calls

There are 2 things you can look at:

1. There is a Dart_IsolateShutdownCallback. I'm not certain you will be able to latch onto it in all cases though.
2. Dart_WeakPersistentHandle - These get set to Dart_Null when the isolate is destroyed. We could do some bookkeeping to clear out old handler entries if they are set to null.

I'm not really attached to the API as much getting the overall functionality into flutter :)

Thanks @BuzzBumbleBee. I, too, am eager to get this feature into flutter.

[BuzzBumbleBee]

@gaaclarke iv re-based this in master to keep the change fresh, Unfortunately so far I have been unable to make a working "auto de-register" process

[johnmccutchan]

I don't recommend this PR because it expands the API surface for the services package in a way that is complicated. The complication should be bore by the flutter developers not the flutter users.

I believe Aaron is correct here.

[BuzzBumbleBee]

I have concerns about the following:

• Naming, API design, and documentation.
• Unclear semantics
• No iOS implementation?

Hi, thanks for the review. I can fix the naming for sure, and I can expand on the documentation.

For iOS i implemented the changes but I do not have an iOS device to validate on.

I'll try get these modifications done over the next week.

Please bare with me, as I'm doing this one in my spare time.

[BuzzBumbleBee]

I don't recommend this PR because it expands the API surface for the services package in a way that is complicated. The complication should be bore by the flutter developers not the flutter users.

I believe Aaron is correct here.

If someone could give me some feedback on how the API could be changed to do this then I'm more than happy to make the changes.

One thing is how do you enable this function? If your saying that it should be transparent to applications Devs how would you register the isolate to receive the channel messages ? (If I spawn 4 isolates I almost definitely do not want all 4 consuming all platform channels)

Wouldn't it be the same complexity for the users be the same as needing to call BackgroundIsolateBinaryMessenger .ensureInitialized(rootIsolateToken); from the spawned isolate (this is the current implementation for one way messages)

[Hixie]

This is a feature we have some interest in implementing, but it's a subtle topic with many implications and we really need to make some clear decisions about the approach before we can land a change. In the interests of being unambiguous I am going to close this PR. To make progress here we will need the following:

1. A clear design doc that makes a detailed analysis of the problem space and the proposed solution.
2. (Technically not required but I would strongly recommend): A discussion of the design doc with the folks who have posted on this thread (in particular @gaaclarke, @johnmccutchan, @stuartmorgan). We have a regular design discussion meeting that we could use for this; normally it's only open to people with commit access but I'm happy to add you to the schedule. Ping me on the Discord if you would like this. Alternatively, we also have this kind of discussion on Discord itself, though that can be harder than just doing a video chat. See the contributing guide for instructions on joining the Discord.
3. A common understanding among the people I listed above and whoever is going to implement and document this feature, as to what the feature should look like.

To help with this I would recommend stepping back from the current proposal and thinking about the space holistically. Specifically, I would write the problem description, goals, requirements, etc, in the design doc without any particular solution in mind. This helps create a more objective way of evaluating proposals. I would also make sure to pay attention to the recommendations in the design doc (and the design doc documentation), and I would also recommend paying attention to the details (e.g. removing boilerplate text in the design doc, etc). Your goal with the design doc is to convince people that the problem exists, get them interested in solving it, and presenting a range of options for how the problem could be approached, as a starting point for discussion. Feedback on the design doc is a gift: people spending time to make suggestions or spot issues. The more you use that gift, the more credibility you will gain with the other participants in the discussion.

I hope this helps. I would encourage you to reach out to me (Hixie) on the Discord if you have any questions or need any help.

[Hixie]

Also I should add, on a personal note, that I am really grateful for your contribution, and I hope you don't take the above as a rejection. The process I describe is the one we use for anything non-trivial, and represents our attempt at finding the best technical decisions as a team. You just happen to have chosen a particularly complicated first thing to contribute. :-)