Repository navigation
RCTScreenScale may deadlock #18096
Description
RCTScreenScale will deadlock if it called for the first time on a non-main thread and the main thread is waiting synchronously for the thread on which RCTScreenScale is called, because the dispatch_sync(dispatch_get_main_queue(), ...) in RCTUnsafeExecuteOnMainQueueOnceSync will deadlock in that scenario. I don't know whether this can actually happen in a React Native app, but if not it should probably be commented on in the code (e.g. to prevent people from copying the code without taking this issue into account).
Because of this issue, having a general helper function like RCTUnsafeExecuteOnMainQueueOnceSync is problematic. For use cases like RCTScreenScale it's usually better to do the initialization work eagerly at app startup or library load time (e.g. in an Objective-C category load method).
Apart from this general issue, RCTUnsafeExecuteOnMainQueueOnceSync also misses a compiler barrier in the second if statement, see the dispatch_once inline function implementation in more recent libdispatch versions, e.g. https://github.com/apple/swift-corelibs-libdispatch/blob/b5ec5d8dfa59426912fbb4884fab642804b99827/dispatch/once.h#L86
Also, if you copy the internals of dispatch_once, I'd use exactly the same predicate test against ~0L instead of 0, just in case that it may matter for some obscure reason.
Finally, I'd like to point out that RCTScreenScale currently doesn't handle the case where the scale of the mainScreen changes, which I think could happen on the Apple TV.
Activity
react-native-bot commentedon Feb 26, 2018
Thanks for posting this! It looks like your issue is missing some required information. Can you please add all the details specified in the Issue Template? This is necessary for people to be able to understand and reproduce your issue.
I am going to close this, but please feel free to open a new issue with the additional information provided. Thanks!
How to Contribute • What to Expect from Maintainers
sophiebits commentedon Feb 26, 2018
@shergin Care to comment? I believe that we never dispatch synchronously from the main thread to this RN thread, precisely to avoid deadlock, but I am not sure.
It’s true that screen size can change and I don’t think our APIs are really set up to accommodate that at the moment. :(
mmmulani commentedon Feb 26, 2018
weird, the bot closed this when it shouldn't have.
we could call the API inside +initilaize to ensure that it's on the main thread.
And yea, the caching isn't ideal either.
[-]RCTScreenSize may lead to deadlock[/-][+]RCTScreenScale may deadlock[/+]hramos commentedon Feb 26, 2018
It does look like the bot worked as expected here: the issue as of this writing still does not conform to the issue template outlined in https://raw.githubusercontent.com/facebook/react-native/master/.github/ISSUE_TEMPLATE.md.
shergin commentedon Feb 27, 2018
@stephan-tolksdorf Thank you for reporting that... and for bunch of advices!
Yes, I am aware of that. Theoretically, this might happen only if RCTSurface was used for instantiating a RootView. This (Surface) is very new and experimental API, so this should not affect real production cases. I plan to fix it soon anyways.
The golden rule of RN is: Never ever block UIManager queue waiting for the main queue. And there is two places where we still does not follow this rule, one of them is
RCTScreenScale. 😞Well, it's pretty low-level system helper, it's not supposed to be used outside the RN Core. It even has "Unsafe" in the name which kinda implied that... if you are using this, you know what you are going.
Yeah, it is also known issue. It will work perfectly with AppleTV, but it will probably not work for attached/connected external displays.
stephan-tolksdorf commentedon Feb 27, 2018
Thanks for replying!
RCTUtils.h seems to be part of the public API and in an embedding scenario someone might be tempted to call
RCTScreenScaleorRCTScreenSizebefore using other parts of React Native. Even if you don't want to support this use case, it might still be worth documenting the current thread-safety limitation of these functions.Since React Native has a huge audience and many developers are looking for inspiration in its sources, it might be worth explaining the sharp edges of tools like
RCTUnsafeExecuteOnMainQueueOnceSyncin short code comments, even if these are obvious to the core developers.(And if you want to continue to use
RCTUnsafeExecuteOnMainQueueOnceSyncinternally, it would probably be prudent to add the missing compiler barrier I mentioned above.)hramos commentedon Jan 31, 2019
What's the status of this issue, can it be closed?
Titozzz commentedon Mar 19, 2019
@shergin Do you think we can close this issue?
React Native sync for revisions 241c446...b5c6dd2