-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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] Static counters lead to data races #44715
Labels
Comments
facebook-github-bot
pushed a commit
that referenced
this issue
Jun 28, 2024
…tomic counters (#45114) Summary: In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein `std::atomic` is the backing for the integer values. ## Changelog: [iOS] [Fixed] - Implement shared atomic counters and replace static integers in `RCTImageLoader` and `RCTNetworkTask` that were accessed concurrently, which in some cases lead to data races. Pull Request resolved: #45114 Test Plan: Added unit tests for the counters in `RCTSharedCounterTests`. Reviewed By: cipolleschi Differential Revision: D59155076 Pulled By: javache fbshipit-source-id: f73afce6a816ad3226ed8c123cb2ccf4183549a0
cortinico
added
Resolution: Fixed
A PR that fixes this issue has been merged.
and removed
Needs: Triage 🔍
labels
Jul 4, 2024
blakef
pushed a commit
that referenced
this issue
Jul 15, 2024
…tomic counters (#45114) Summary: In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein `std::atomic` is the backing for the integer values. ## Changelog: [iOS] [Fixed] - Implement shared atomic counters and replace static integers in `RCTImageLoader` and `RCTNetworkTask` that were accessed concurrently, which in some cases lead to data races. Pull Request resolved: #45114 Test Plan: Added unit tests for the counters in `RCTSharedCounterTests`. Reviewed By: cipolleschi Differential Revision: D59155076 Pulled By: javache fbshipit-source-id: f73afce6a816ad3226ed8c123cb2ccf4183549a0
Titozzz
pushed a commit
that referenced
this issue
Jul 22, 2024
…tomic counters (#45114) Summary: In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein `std::atomic` is the backing for the integer values. ## Changelog: [iOS] [Fixed] - Implement shared atomic counters and replace static integers in `RCTImageLoader` and `RCTNetworkTask` that were accessed concurrently, which in some cases lead to data races. Pull Request resolved: #45114 Test Plan: Added unit tests for the counters in `RCTSharedCounterTests`. Reviewed By: cipolleschi Differential Revision: D59155076 Pulled By: javache fbshipit-source-id: f73afce6a816ad3226ed8c123cb2ccf4183549a0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Description
Some places there are static counter variables in the iOS code base. This can be problematic when different threads increment the counters. One such example is
getNextImageRequestCount
ofRCTImageLoader
:Another example is the initializer of
RCTNetworkTask
:Other places in the code, this issue seems to have been addressed, e.g.
RCTModuleData
:As shared counters seem to be employed several places in the code base, perhaps a common implementation should reside in the utils? C++ is not my forte, but perhaps one could do something like this:
This can be employed everywhere a shared counter is needed, and since it is generic, one can preserve the specific primitive types needed for each counter case.
Steps to reproduce
NB! Reproduction is tricky as other data races may occur:
cd /Users/user/Documents/Dev/react-native/packages/rn-tester && npm i && cd ios && bundle exec pod install
React Native Version
0.74.1
Affected Platforms
Runtime - iOS
Output of
npx react-native info
Stacktrace or Logs
Reproducer
https://github.com/hakonk/react-native
Screenshots and Videos
No response
The text was updated successfully, but these errors were encountered: