Started handling messages from background isolates.

[gaaclarke]

companion framework pr: https://github.com/flutter/flutter/pull/109005

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]

@jason-simmons Can you please have a look at this draft PR? No need to approve it. I just want to make sure we are on a happy path. This PR follows the work outlined in the Isolate Platform Channel design doc and works for iOS Dart->Host messages (notice there is a framework PR if you want to peek at that too). It probably needs a bit more documentation in the source code. I'm still working on a testing plan, your feedback will help inform that. I'm going to have an integration test in the flutter repo for sure.

Something you might find interesting is the threadsafe_unique_ptr. I came up with that with you in mind since I wanted weak_ptr but you've had some valid concerns about shared_ptr being hard to verify so I think this addresses those in this case where we want a clear single owner.

My plan for this is to get early feedback from you and if everything goes well I'll use this as the template for implementing Dart->Host isolate platform channels across the platforms. I don't have plans on implementing Host->Dart isolate platform channels anytime soon (since it didn't have a clear resolution in the design doc and isn't what most people are asking for).

[jason-simmons]

I'm concerned that allowing access to the root isolate's PlatformConfiguration from multiple threads and isolates will be too confusing and error-prone. Likewise for allowing access to UI thread related Shell services from threads besides the UI thread.

I'm not sure what would be the safest way to implement this. The first potential option that comes to mind would be populating the child isolate's UIDartState::Context with the platform task runner plus a service that can be invoked on the platform thread to dispatch a platform message. The service would need to hold a weak reference to the PlatformView and wrap PlatformView::HandlePlatformMessage. The service could be passed from the parent isolate to the child isolate via the DartIsolateGroupData.

Would be interested in getting consensus among the engine team about how best to do this (@chinmaygarde @dnfield)

[gaaclarke]

Thanks @jason-simmons for taking a look.

I'm concerned that allowing access to the root isolate's PlatformConfiguration from multiple threads and isolates will be too confusing and error-prone. Likewise for allowing access to UI thread related Shell services from threads besides the UI thread.

Yep, I shared that concern that's why I've limited access to PlatformConfiguration to only the method that I've deemed threadsafe, HandlePlatformMessage. If you look at UIDartState you can see that while you can set PlatformConfiguration on it, you can never query it unless you are the RootIsolate (matching old behavior). The only way a background isolate can access it is via UIDartState::HandlePlatformMessage. That's why I added that method, to provide protected access to the PlatformConfiguration. At some point I had an alternative interface for PlatformConfiguration that was just the methods that can be called safely on background threads, but I think that confuses memory management and the code more. It was better to let UIDartState protect access imo. We could go down the route of just giving a limited interface on PlatformConfiguration to background isolates if you want. Maybe it wouldn't be so bad now that we have threadsafe_unique_ptr to make memory management easier.

I'm not sure what would be the safest way to implement this.

If you describe potential things that could go wrong it will be easier to guide you on how to make things safer.

The first potential option that comes to mind would be populating the child isolate's UIDartState::Context with the platform task runner plus a service that can be invoked on the platform thread to dispatch a platform message. The service would need to hold a weak reference to the PlatformView and wrap PlatformView::HandlePlatformMessage. The service could be passed from the parent isolate to the child isolate via the DartIsolateGroupData.

I know this isn't very clear in the code but I wanted to make sure you saw that the PlatformView is guaranteed to be alive when receiving messages from background isolates right now because of threadsafe_unique_ptr. It looks like this, which means the PlatformConfiguration becomes unavailable to background isolates before the PlatformView is deallocated.

Would be interested in getting consensus among the engine team about how best to do this (@chinmaygarde @dnfield)

While this is a draft I would prefer to work with just one person. We've worked with similar code and situations in the past so I thought you'd be a good person to get early feedback from. It became clear halfway through implementing this that it will require careful consideration. Until I can make one person happy with the code there is no point in adding more voices which confuses the progress.

[jason-simmons]

Generally the concern is about potential races and deadlocks due to changing what were previously invariants of the engine's architecture.

Before this the PlatformConfiguration was only accessed from the UI thread and from the root isolate. With this PR, other threads and isolates will be invoking methods on the root isolate's PlatformConfiguration instance. Anyone working with PlatformConfiguration will now need to keep that special case in mind.

PlatformConfiguration is a gateway to the APIs exported by the Shell. The Shell already has a complex threading model that coordinates components that are associated with the platform, UI, IO, and raster threads. This PR adds another special case - a part of the Shell that used to be exclusive to the UI thread is now accessed from background isolate threads.

This in turn leads to further complexity such as making DartIsolate/PlatformConfiguration destruction potentially block waiting for completion of some other thread's call to HandlePlatformMessage.

I'd prefer a solution that makes it obvious which objects can be used across all Dart threads and isolates and keeps them entirely separate from objects that are tied to a specific thread and isolate. That will make it easier to reason about the interactions between components and avoid bugs.

[gaaclarke]

Thanks, some clarifications and potential alternatives:

Before this the PlatformConfiguration was only accessed from the UI thread and from the root isolate. With this PR, other threads and isolates will be invoking methods on the root isolate's PlatformConfiguration instance. Anyone working with PlatformConfiguration will now need to keep that special case in mind.

The only method being called on PlatformConfiguration from a new thread is client() which returns a const value which is set at PlatformConfiguration construction. That value of client() is guaranteed to be alive by the threadsafe_unique_ptr. A background isolate can't arbitrarily call into PlatformConfiguration since UIDartState restricts access to it.

This in turn leads to further complexity such as making DartIsolate/PlatformConfiguration destruction potentially block waiting for completion of some other thread's call to HandlePlatformMessage.

I just want to point out, since it has a confusing name, HandlePlatformMessage is the code that passes off execution to the thread that will execute the user defined handler. So, that isn't as bad as it may sound.

I'd prefer a solution that makes it obvious which objects can be used across all Dart threads and isolates and keeps them entirely separate from objects that are tied to a specific thread and isolate. That will make it easier to reason about the interactions between components and avoid bugs.

Here's an idea. Platform Channels have 2 halves: the sending to the target thread and the execution on the target thread. In order to implement this feature we only need to change the first half. Traditionally that flow looks like this: Dart->DartIsolate->PlatformConfiguration->RuntimeController->Engine->Shell->PlatformMessageHandler->(hop to platform thread)->PlatformView. For the purposes of messages from background isolates most of that is a waste and that seems your biggest concern. So theoretically we could have a sequence like this: Dart->DartIsolate->PlatformMessageHandler->(hop to platform thread)->PlatformView. Would prefer this? It might be a difficult bookkeeping task but I can see.

[gaaclarke]

@dnfield I've responded to all the early feedback (thanks everyone) and have added tests and documentation, let's do this for real now. I also cut the scope to iOS in the name of trying to make the PR smaller and easier to review. If someone uses this feature outside of iOS it will throw an UnimplementedError. I've added some integration tests here and ultimately the full flow is tested in integration tests in the framework PR.

[gaaclarke]

This in its current state actually breaks android platform channels of size 1 byte in the framework integration test here: https://github.com/gaaclarke/flutter/blob/2c22cd79a206503396dd31dc02074ccb66604259/dev/integration_tests/channels/lib/main.dart#L114:L114

That same test passes in iOS after the patch and passes in Android before this patch. I'll look into it.

[gaaclarke]

This in its current state actually breaks android platform channels of size 1 byte in the framework integration test here: https://github.com/gaaclarke/flutter/blob/2c22cd79a206503396dd31dc02074ccb66604259/dev/integration_tests/channels/lib/main.dart#L114:L114

That same test passes in iOS after the patch and passes in Android before this patch. I'll look into it.

I've looked into this further. I can reproduce it on the patch before this PR so it wasn't introduced here. I'll investigate it separately since it is blocking me from implementing the Android side of this PR. I'll have to figure out why CI isn't picking it up either.

[gaaclarke]

Update:

• https://github.com/flutter/engine/pull/35804 implements Android
• https://github.com/flutter/engine/pull/35803 fixes the crash we were seeing in android builds before this PR

[gaaclarke]

Update:

• https://github.com/flutter/engine/pull/35893 Implements support for all the desktop embedders

[gaaclarke]

Thanks everyone, I'll get to work cleaning up and putting and the dependent PRs into review today.