-
Notifications
You must be signed in to change notification settings - Fork 24.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[iOS] Fixes main thread stuck when reload in bridgeless mode #45486
Conversation
Base commit: fcd526d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zhongwuzw, thanks for the fix. I left a question/suggestion as the change is slightly different from the original code.
packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm
Outdated
Show resolved
Hide resolved
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Hi @zhongwuzw! A colleague suggested that we might just remove the callback. Given this todo, we should be able to just start the surface after it is created. Do you mind implementing the changes, please? |
@cipolleschi Hi, do you mean we can call the start surface on |
Yes, probably yes! :D |
@cipolleschi I changed to bufferedruntime. |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@cipolleschi merged this pull request in a778979. |
This pull request was successfully merged by @zhongwuzw in a778979 When will my fix make it into a release? | How to file a pick request? |
moduleRegistry:_moduleRegistry | ||
parentInspectorTarget:_inspectorTarget.get() | ||
launchOptions:_launchOptions]; | ||
[_hostDelegate hostDidStart:self]; | ||
|
||
for (RCTFabricSurface *surface in [self _getAttachedSurfaces]) { | ||
[surface resetWithSurfacePresenter:self.surfacePresenter]; | ||
[_instance callFunctionOnBufferedRumtimeExecutor:[surface](facebook::jsi::Runtime &_) { [surface start]; }]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small typo "rumtime"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch, I would fix it in my next PR. :)
Summary: I introduced a typo in #45486 . Thanks migueldaipre for the catch-up. cc cipolleschi ## Changelog: [IOS] [FIXED] - Fixes typo of function callFunctionOnBufferedRumtimeExecutor Pull Request resolved: #45902 Test Plan: Just a typo. Reviewed By: cipolleschi Differential Revision: D60775511 Pulled By: arushikesarwani94 fbshipit-source-id: da781ea5ecf2e0a15e5419430240e10194043b1b
Summary:
In fabric bridgeless mode, when we reload, main thread may block because of dead lock. the backtrace example as below:
Changelog:
[IOS] [FIXED] - Fixes main thread stuck when reload in bridgeless mode
Test Plan:
RNTester, enables fabric, which is very easy to repro by tapping
r
command multiple times quickly to trigger reload.