Repository navigation
Fix: avoid deadlock by removing requiresMainQueueSetup = true from Mapbox view managers - #3954
Conversation
60a996d to
6cff75f
Compare
ff820a5 to
1a982c5
Compare
requiresMainQueueSetup, which forces React Native to lazily initialize the TurboModule on the main thread. When this initialization happened while Fabric was performing a synchronous UI update (e.g. from native-driven animations), the JS thread and main thread could deadlock: - Main thread held a shared lock in ComponentDescriptorRegistry during synchronouslyUpdateViewOnUIThread. - JS thread attempted to resolve the view manager, triggering RCTUnsafeExecuteOnMainQueueSync. - Both threads waited on each other → freeze. Since these view managers do not require UIKit access during init, they can safely return false. This removes the synchronous main-thread call and eliminates the deadlock condition.
1a982c5 to
82e0e2f
Compare
|
@whiteHatCip good catch. I happen to reproduce the issue, I think the bug is in ReactNative locking mechanism for ComponentDescriptorRegistry. It's holding a lock while calling code that can wait on main thread. That's asking for deadlock. Not using requiresMainQueueSetup is a good workaround, I don't think we need that either. For me main thread was waiting in While the js thread was trying to to create a view and first tried to init the manager, which required the main queue, but it hold a lock inside ComponentDescriptorRegistry::add Also this is somewhat documented in react-native source: And see also: com.facebook.react.runtime.JavaScript (10) |
Well I didn't know that it was - like you said - somewhat documented, and it's an absolute coincidence that I found the solution to our deadlock issue. anyway, I am glad that this issue is no more 😃 Gotta say, tho, if the one I proposed is to be considered a workaround, do you think that there would be a "more recommended" way to do the same things? what do you think? |
|
@whiteHatCip one thing I'm wandering is why is new arch using |
Is new arch using it? why do you ask? |
The deadlock happened in new architecture. The files you changed was in old architecture |
Circle layer was just an example. Yes it could be any component. And yes the deadlock is by changing the requiresMainQueueSetup to false. I agree that this change will not cause any issue, I also agree that this change fixes the deadlock.
Again this is good change, and fixes the deadlock. BUT in new architecture react-native should be using our Fabric components |
Description
Fixes #3886
Checklist
CONTRIBUTING.mdyarn generatein the root folder/exampleapp./example)There's no need for a component that reproduces the bug because the bug happens also in the example app.
Since it's happening apparently randomly, I cannot tell the exact steps to reproduce the bug, and to verify whether my proposed solution gets rid of the bug 100% sure, but before I made these changes I was experiencing the app freeze every time I ran the app. After applying these changes, the bug was no more, despite I have to admit that I've been launching, playing with and killing and starting again the app for about 30-40 minutes, hence this is just manual testing and has intrinsic limitations.
Bug Root Cause
Several Mapbox view managers declared:
This forces React Native to lazily initialize the TurboModule / view manager on the main thread, synchronously, the first time JS code accesses it.
When this lazy initialization happened while Fabric was already performing a synchronous UI update (e.g. from a native-driven animation), both the JS thread and the main thread ended up waiting on each other:
• The main thread held a shared lock in ComponentDescriptorRegistry while updating props.
• The JS thread tried to resolve the view manager, which called into RCTUnsafeExecuteOnMainQueueSync to initialize the module.
• Result: main waits for JS, JS waits for main → deadlock.
This manifested as the app “freezing” with no crash logs, sometimes seemingly at random.
Probably the reason why @McFly78 could experience less frequent freezes when he reduced the amount children of the Map: less children were racing to be initialized and updated. At least that's my guess.
Fix
All Mapbox view managers that previously returned YES from requiresMainQueueSetup now return NO.
None of these managers actually require synchronous main-thread setup (they don’t touch UIKit at initialization time), so they can be safely initialized off the main thread. This eliminates the main-thread synchronous call and removes the possibility of deadlock.
Outcome
truefromrequiresMainQueueSetupif the module truly depends onUIKitduring initialization.