-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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] Fix data race in PersistentFileLogSpec.swift
#28924
[iOS] Fix data race in PersistentFileLogSpec.swift
#28924
Conversation
Synchronize access to bool value with NSLock
Can you elaborate a bit more on what the data race is? I don't doubt that there is one but from my read of the code a data race is okay since there's only one thread writing the value and the other is read polling. If the poll needs one additional loop due to a inconsistent read I think the test still is valid? Definitely okay with moving forward with this solution, just want to clarify a bit before reviewing the code. |
@wschurman Thanks for the feedback. I agree it is not a very severe case. I often use TSan to find potential issues, and I find it useful to eliminate any data races such that one can rather catch the most severe ones if they were to arise. |
Under the "Why" headline, I provided a code snippet with comments denoting the data race. We seem to agree on the nature of it. I can provide some screen shots from Xcode with TSan if that helps clarify the issue. |
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 for the fix! I also looked into using swift actors here but they aren't compatible with our current callback structure.
Don't worry about the test failures. Just one minor set of updates and then I will go ahead and merge this.
packages/expo-modules-core/ios/Tests/PersistentFileLogSpec.swift
Outdated
Show resolved
Hide resolved
packages/expo-updates/ios/Tests/AppLauncherWithDatabaseSpec.swift
Outdated
Show resolved
Hide resolved
Awesome! Thank you |
Co-authored-by: Will Schurman <wschurman@gmail.com>
Fix data race in
PersistentFileLogSpec.swift
by synchronizing concurrent access to a bool value withNSLock
.Why
When running the tests with TSan enabled, I discovered a data race in
PersistentFileLogSpec
:A similar issue was found in
AppLauncherWithDatabaseMock
and fixed accordingly.How
Employ locking mechanism for synchronizing access to the mentioned bool:
Substitute
Swift.Bool
withSynchronized<Bool>
:Test Plan
Piggy back on existing tests as they touch on the relevant code paths.