Synchronize to fix crashes in the user module #1554
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nan-li
force-pushed
the
fix/various_user_module_crashes
branch
2 times, most recently
from
1d54216 to
4adc282
Compare
* Add test to start user manager from multiple threads
* The models dictionary could be mutated from multiple threads
* Don't update the subscription ID to the server. Locally, we may set it to null if we get 404s and later we will get a value when user create is called. * We were sending an Update Subscription request with "subscriptionId" = null which isn't even a valid property anyway and no-ops thus far
* The OneSignalUserManagerImpl `start()` method sets up initializers and local state. * Now prevent any race conditions that could occur with multiple threads trying to start up the manager simultaneously by making it thread-safe, atomic, and properly handle concurrent access. * We must now be careful of producing a deadlock situation within the start() method.
* Some code paths use _login to create an anonymous user but this is overly complicated when they can just directly call createNewUser. * createNewUser will set the _user instance so there is no need to set it after the method returns * Now, _login is only used by the public login method, so remove this helper _login method.
* Start the user manager before making updates for session time, purchases
nan-li
force-pushed
the
fix/various_user_module_crashes
branch
from
4adc282 to
735e755
Compare
nan-li
changed the title
|
The test |
jkasten2
approved these changes
May 6, 2025
This was referenced May 7, 2025
Merged
This was referenced May 27, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Make changes to synchronize and prevent crashes that have been reported.
Details
Motivation
Address synchronization, race conditions, and user-submitted crash reports.
Scope
purchasesandsession_timewill be dropped if the user manager has not started up by that point, which is a scenario that should not be happening anyway.Testing
Unit testing
Added one test to reproduce a crash via multiple concurrent threads.
Manual testing
Tested on iPhone 13 running iOS 18.4.1
start()method essentially has 3 paths: has cached user, has legacy player to migrate, and new install. Test all code paths to ensure correctness and no deadlocking.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is