[camera]revert the weakSelf in FLTThreadSafeEventChannel #6303
Conversation
|
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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
Hmm... |
|
@jmagman oh I initially pushed without tests. test was added later. so we should be good |
Oh okay good, thanks! |
| XCTestExpectation *expectation = | ||
| [self expectationWithDescription:@"Completion should be called."]; | ||
| dispatch_async(dispatch_queue_create("test", NULL), ^{ | ||
| FLTThreadSafeEventChannel *channel = [[FLTThreadSafeEventChannel alloc] |
There was a problem hiding this comment.
@autoreleasepool to release it?
There was a problem hiding this comment.
No need because the channel defined within the scope of the dispatch_async.
I tested it out and this test fails the previous code and passes the current code. So we should be fine.
hellohuanlin
added
the
autosubmit
2 participants
Why this bug?
introduced in flutter/flutter#109910
This is the same reason why we can't use weakSelf in FLTThreadSafeFlutterResult - it's only retained within a call stack, not in the heap. So when dispatched back to main, the wrapper has been gone.
Workaround
As a temp workaround, I will simply revert the change in
FLTThreadSafeEventChannelto fix the bug. An alternative workaround is to makeFLTThreadSafeEventChannelan ivar and keep it around during streaming. I have verified that both works.In this particular example, we don't have to worry about strongSelf making engine API being called after engine is torn down. This is because engine would have to be dispatched to the main thread to be torn down, which must happen after the current block that we just enqueued.
Long term solution
However, I dislike this approach, because all our thread safe wrappers cannot be re-used as general utilitys - they can only be used in this particular context of camera plugin.
In the long term, I think a more general solution is to always use strong reference to the wrapper itself (where the retain cycle will be broken after the work is done dispatching), but pass in weak reference of the
plugin, and use thepluginreference to tell if the engine is torn down or not (instead of using wrapper reference). This approach will make the wrapper working for both ivar and local variable/argument scenarios, while still avoiding calling engine API after engine is torn down.List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#109910
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.