Repository navigation
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
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. |
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the thread creation logic for the Flutter engine on iOS to ensure a separate UI thread is always created when allowHeadlessExecution is enabled. This prevents the engine from hanging in environments like notification service extensions where the platform's run loop is not actively pumped. The changes involve updating MakeThreadHost to consider allowHeadlessExecution and adjusting the logic for selecting the UI task runner in createShell. The changes are logical and address the issue, though there is an opportunity to unify the decision-making logic to improve maintainability.
| bool useMergedThread = | ||
| settings.enable_impeller && | ||
| settings.merged_platform_ui_thread == flutter::Settings::MergedPlatformUIThread::kEnabled && | ||
| !self.allowHeadlessExecution; |
There was a problem hiding this comment.
While this logic is correct, the decision to use a merged thread is now present in two places: here and in MakeThreadHost. This duplication could lead to inconsistencies in the future.
To improve clarity and maintainability, consider consolidating this logic. You could define useMergedThread before calling MakeThreadHost and then pass !useMergedThread to it to decide whether to create the UI thread. This would unify the decision-making into a single place.
For example:
// In -[FlutterEngine createShell:...]
// ... before calling MakeThreadHost
const bool useMergedThread =
settings.enable_impeller &&
settings.merged_platform_ui_thread == flutter::Settings::MergedPlatformUIThread::kEnabled &&
!self.allowHeadlessExecution;
NSString* threadLabel = [FlutterEngine generateThreadLabel:self.labelPrefix];
_threadHost = std::make_shared<flutter::ThreadHost>();
// MakeThreadHost would need to be adjusted to take a boolean indicating if a UI thread should be created.
*_threadHost = MakeThreadHost(threadLabel, settings, !useMergedThread);
// ...
if (useMergedThread) {
ui_runner = fml::MessageLoop::GetCurrent().GetTaskRunner();
} else {
ui_runner = _threadHost->ui_thread->GetTaskRunner();
}This would also resolve a potential pre-existing issue where a crash could occur if enable_impeller is false while merged_platform_ui_thread is enabled.
References
- Code should avoid duplicating state to maintain a single source of truth. The logic for deciding whether to use a merged thread is duplicated, which could be consolidated. (link)
- Code should be optimized for readability. Consolidating the logic for thread creation would make the code easier to understand and maintain. (link)
There was a problem hiding this comment.
I have implemented this change
for iOS service extensions
401a53b to
4d9d673
Compare
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
This sounds a bit dicy dealing with threading. This PR has no tests, we'll need that as a minimum.
Absolutely. Probably out of my depth a bit here. However, I've added one test there which I think gives full coverage? |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I'll let the iOS team review this. Thanks for the test. There is always a tension when increasing the FlutterEngine API but I think you have a good case to be made here.
| * | ||
| * Defaults to NO. | ||
| */ | ||
| @property(nonatomic, assign) BOOL isRunningInExtension; |
There was a problem hiding this comment.
This should be a parameter in init, not a property since setting it after run is meaningless.
There was a problem hiding this comment.
Happy to shift it to a parameter if that is preferred.
The use is like this:
let engine = FlutterEngine(
name: "notification_service",
project: flutterProject,
allowHeadlessExecution: true
)
engine.isRunningInExtension = true
self.flutterEngine = engine
// Run the dispatcher entrypoint
let success = engine.run(withEntrypoint: "dispatcher",
libraryURI: nil,
initialRoute: nil,
entrypointArgs: ["notification_service_extension"])
In FlutterEngine.h there is a kind of similar existing example of a settable BOOL property:
isGpuDisabled (line 441-446):
/**
* Whether or not GPU calls are allowed.
*
* Typically this is set when the app is backgrounded and foregrounded.
*/
@property(nonatomic, assign) BOOL isGpuDisabled;
| * | ||
| * When set to YES, the engine will use a separate UI thread instead of merging with the platform | ||
| * thread. This is necessary in extension contexts where the platform's CFRunLoop is not being | ||
| * actively pumped. |
There was a problem hiding this comment.
what does "pump" mean.
Could you also add details on why this is necessary?
There was a problem hiding this comment.
Sure, this is the first time I've worked on this so it's a lot for me to wrap my head around.
In a Service Extension the Platform Thread is not being looped through. In a normal iOS app UIApplicationMain loops indefinitely.
In the engine there is a check (which I have modified) which sees if it should merge the UI Thread into the Platform thread. If they are merged the dart code is never run because it is not looped through.
We need some way of forcing the two threads to be seperate if we want it to ever be able to run dart code without the Platform Thread looping. Alternatively we could potentially start manually looping the Platform Thread.
| // - Impeller is disabled (merged thread only works with Impeller) | ||
| // - merged_platform_ui_thread setting is not enabled | ||
| // - Running in an extension (e.g., notification service extension) where the platform's | ||
| // CFRunLoop is not being actively pumped |
There was a problem hiding this comment.
is there any apple documentation on this? does this apply to all extensions (e.g. share extension) or is it just for notification service extension
|
@Gibbo97 from triage: are you able to address the comments? thanks |
|
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. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
From triage: @Gibbo97 we're converting this to a draft for now to remove it from our triage queue. Please request review again when you're ready! |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
I have decided to close this PR as I came up with a better solution that does not need the engine changes. Writeup and sample project here: https://medium.com/top-rail/flutter-and-notification-service-extensions-d88c35e4bfb9 |
This PR aims to allow the flutter engine to run from iOS service extensions. We have an "offline first" app which has extensive dart code to manage state in an SQLite database among other things. We want to reuse this code and run it from a Notification Service Extension before a notification is delivered, to ensure the app has the data the user is being notified about. Service extensions like this are a common feature.
When allowHeadlessExecution: true is set, the Flutter engine was still trying to use the merged platform/UI thread optimization. In notification service extensions, the platform's CFRunLoop isn't being pumped, causing the engine to hang.
Existing Issue: #165904
Example project https://github.com/Gibbo97/flutter_headless_engine_example
In the example when a notification is sent to the device, the method "dispatcher" in main.dart is called from NotificationService.swift, and the print statement runs successfully.
Without the engine changes this statement is never run.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on [Discord].