Switch the MacOS Desktop embedder to using a thread configuration where the platform and render task runners are the same. #13300
Conversation
|
@franciscojma86, @stuartmorgan When you are back, can we figure out how to make the test harness more thorough? There also needs to be some guidance on managing the lifecycle of the FlutterEngine. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Haven't had time for a full review, but flagged a few things in a quick skim.
| * Instead of looking up the assets and ICU data path in the application bundle, this initializer | ||
| * allows callers to create a Dart project with custom locations specified for the both. | ||
| */ | ||
| - (_Nonnull instancetype)initWithAssetsPath:(NSString* _Nonnull)assets |
There was a problem hiding this comment.
Prefer nonnull to _Nonnull when possible (such as here) for consistency with the other macOS framework code.
| @implementation FlutterDartProject { | ||
| NSBundle* _dartBundle; | ||
| NSString* _assetsPath; | ||
| NSString* _icuDataPath; |
| } | ||
|
|
||
| // Balancing release for the retain in the task runner dispatch table. | ||
| CFRelease((CFTypeRef)self); |
There was a problem hiding this comment.
The declaration comment above can't possibly be correct with this change, because dealloc won't be called as long as this reference exists. So it seems like this makes it impossible to clean up engine instances.
There was a problem hiding this comment.
My expectation was that when the view controller goes away, it will reset its association with the FlutterEngine via engine.viewController = nil; which would have shutdown the engine explicitly. But you're right that this is a leak now as that doesn't seem to be happening. The expectation is that the engine needs to be shut down explicitly. I'll rework this.
|
|
||
| #pragma mark - Task runner integration | ||
|
|
||
| - (void)postMainThreadTask:(FlutterTask)task targetTimeNanos:(uint64_t)target_time { |
There was a problem hiding this comment.
macOS framework style is to pre-declare at the top of the file, with a declaration comment, all private methods.
|
|
||
| #pragma mark - Task runner integration | ||
|
|
||
| - (void)postMainThreadTask:(FlutterTask)task targetTimeNanos:(uint64_t)target_time { |
|
|
||
| #pragma mark - Task runner integration | ||
|
|
||
| - (void)postMainThreadTask:(FlutterTask)task targetTimeNanos:(uint64_t)target_time { |
There was a problem hiding this comment.
targetTimeInNanoseconds: (abbreviation is strongly discouraged in ObjC)
| if (result != kSuccess) { | ||
| NSLog(@"Could not post a task to the Flutter engine."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this be also logged as a failure?
There was a problem hiding this comment.
This should not happen at all because timed tasks are not posted to the platform or render task runners and untimed tasks are awaited upon during shell shutdown. But yeah, not giving all the tasks back to the engine is a small leak of the task handle.
…re the platform and render task runners are the same. Also creates a new test harness for the desktop embedder framework target and adds a test that launches a headless engine in this new thread configuration. Fixes flutter/flutter#17579
|
I cleared @franciscojma86 review. This isn't good to go yet with the explicit shutdown being now necessary. |
|
@keithbrianptomo Can you elaborate? |
chinmaygarde
force-pushed
the
no_resize_crash
branch
from
fdc69c6 to
862c0ad
Compare
|
Hmm, I tried really hard to figure out a way to safely shut down the embedder instance of the Flutter engine automatically on The next bit is tricky because both the Obj-C & C instances of the engine object are called The FlutterEngine Obj-C instance needs to be accessed from other threads while it is being deallocated. This is because the FlutterEngine C instance may post tasks during FlutterEngine Obj-C instance shutdown. Accessing the Obj-C instance from other thread during deallocation is not safe (specifically the weak handle generation). The runtime correctly warns of this error. Now, my patch doesn't actually introduce a leak because deallocating the view controller will set the engines view controller property to nil. This, in turn, will shutdown the engine. Thereby releasing the strong reference. While using the view controller directly wont result in a leak, I understand that folks using the FlutterEngine instance directly now need a way to shutdown the engine explicitly lest they introduce a leak. To address this, I have moved I believe with these changes and requirements on the use of the FlutterEngine, the lifecycle management is thread safe and leak free. PTAL. |
…tion where the platform and render task runners are the same. (flutter/engine#13300)
…tion where the platform and render task runners are the same. (flutter/engine#13300)
[email protected]:flutter/engine.git/compare/c3b63d610b07...49971e2 git log c3b63d6..49971e2 --no-merges --oneline 2019-10-28 [email protected] Implement basic Picture.toImage via BitmapCanvas (flutter/engine#13391) 2019-10-28 [email protected] Switch the MacOS Desktop embedder to using a thread configuration where the platform and render task runners are the same. (flutter/engine#13300) 2019-10-28 [email protected] Roll src/third_party/skia 18f5b1a6dd77..1beb8ae9a2f5 (9 commits) (flutter/engine#13392) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
[email protected]:flutter/engine.git/compare/c3b63d610b07...49971e2 git log c3b63d6..49971e2 --no-merges --oneline 2019-10-28 [email protected] Implement basic Picture.toImage via BitmapCanvas (flutter/engine#13391) 2019-10-28 [email protected] Switch the MacOS Desktop embedder to using a thread configuration where the platform and render task runners are the same. (flutter/engine#13300) 2019-10-28 [email protected] Roll src/third_party/skia 18f5b1a6dd77..1beb8ae9a2f5 (9 commits) (flutter/engine#13392) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Adds a `destruction_callback` member to `FlutterTaskRunnerDescription` that provides embedder developers a means of performing any cleanup of resources tied to the lifetime of the task runner. In the case of the macOS embedder, the task runner holds a reference to the engine itself that is used in the `post_task_callback` and whose lifetime needs to match/exceed that of the task runner. This refactoring allows us to centralise all related retain count manipulation around task runner setup. This is a refactor of the lifecycle bits of #13300. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…#56445) Adds a `destruction_callback` member to `FlutterTaskRunnerDescription` that provides embedder developers a means of performing any cleanup of resources tied to the lifetime of the task runner. In the case of the macOS embedder, the task runner holds a reference to the engine itself that is used in the `post_task_callback` and whose lifetime needs to match/exceed that of the task runner. This refactoring allows us to centralise all related retain count manipulation around task runner setup. This is a refactor of the lifecycle bits of flutter/engine#13300. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
4 participants
Also creates a new test harness for the desktop embedder framework target and adds a test that launches a headless engine in this new thread configuration.
Fixes flutter/flutter#17579