Protect against race conditions accessing the same RCTDevSettings object #743
Conversation
Merge upstream into fork
|
React/CoreModules/RCTDevSettings.mm
Outdated
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| [self->_userDefaults setObject:self->_settings forKey:kRCTDevSettingsUserDefaultsKey]; | ||
| }); |
There was a problem hiding this comment.
Could this cause an issue because this method is called on init, and someone might potentially use this object before all the default values are set because it's asynchronous?
There was a problem hiding this comment.
nit: maybe we should only dispatch async to main thread when the current running thread isn't main thread.... or do you know already it will always be in background thread?
There was a problem hiding this comment.
There's a utility in React Native that does this exact check too (https://github.com/facebook/react-native/blob/1163599989aa6d641f127f3100a01c45f04d2edc/React/Base/RCTUtils.m#L259)
There was a problem hiding this comment.
Note that the utility next to it that does a dispatch_sync is explicitly named unsafe because of deadlocks exactly like this one :D
There was a problem hiding this comment.
Nice yea I think it makes sense to use RN's explicit method to check the thread. I don't think we can guarantee this is ever called on a given thread
There was a problem hiding this comment.
Could this cause an issue because this method is called on init, and someone might potentially use this object before all the default values are set because it's asynchronous?
Don't think we need to worry about that. The _settings var is determined synchronously before we call this and the _userDefaults value is set up synchronously before this in init.
…ect (microsoft#743) * Update RCTCxxBridge.mm * Update RCTCxxBridge.mm * protect against race conditions * use built in main queue execution
Please select one of the following
Summary
It's possible to get into a scenario where the main thread and a background thread are trying to edit the same object at the same time, causing a deadlock on initialization. The general deadlock happens like this:
It's not totally clear to me why the background thread is stopping the main thread's access to RCTDevSettings, but from searching around it appears that we aren't the first ones to hit multi-threading issues using Apple's NSUserDefaults object.
The simplest fix with the smallest churn between our fork and upstream that also stops the deadlocking from happening is to kick off the main thread's call to happen async so we don't block on setting user defaults.
An alternative fix here can be to move all the NSUserDefaults props to be ivars on the class, but that has a couple issues.
Changelog
[Apple] [Bug] - Protect against race conditions deadlocking on init
Test Plan
Ran this in a downstream test app and we consistently deadlock here with another thread holding a mutex on the same object. When we dispatch async we don't block the thread on data manipulation and won't deadlock anymore.
Microsoft Reviewers: Open in CodeFlow