Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix crashes when encoding user models #1412

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Fix crashes when encoding user models #1412

merged 7 commits into from
Apr 29, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Apr 18, 2024 •
edited
Loading

Description

One Line Summary

Fix crashes during model encoding due to concurrent data access.

Details

  • Models are encoded often: any change encodes them in the model store, when deltas are cached in the operation repo, when deltas are flushed in the operation repo, when deltas are enqueued to executors, when deltas are turned into requests, when requests are sent and removed, etc. These all have references to models.

  • Identity Model and Properties Model have access to their aliases and tags dictionary wrapped in a lock. However, this lock was missing from the encode method, and is necessary.

  • The changes made in this PR already avoid recursive locking, but let's also replace the lock that is used to a NSRecursiveLock for future-proofing. During development, I ran into this before trying to avoid recursive locking as models are encoded so often. This occurred when tags change on a model (in a lock), driving the model store to encode it's models, which eventually calls the model's own encode method (which is also in a lock).

  • Another change in this PR is to reduce extra caching of the Identity Model.

Motivation

Fixes any stack trace with this signature.

0  libobjc.A.dylib                0x1814 objc_msgSend + 20
1  Foundation                     0x12284 _encodeObject + 172
2  OneSignalUser                  0x12a14 __swift_destroy_boxed_opaque_existential_0 + 6928
3  OneSignalUser                  0x12a74 __swift_destroy_boxed_opaque_existential_0 + 7024
...
8  Foundation                     0x547f4 -[NSKeyedArchiver _encodeArrayOfObjects:forKey:] + 344
...
11 OneSignalCore                  0xd69c -[OneSignalUserDefaults saveCodeableDataForKey:withValue:] + 88

Scope

Locking in models

Alternative to Consider

  • Many objects have a model reference, and encodes the model when itself is being cached.
  • However, most of these objects don't need all the data in the model.
  • For example, an OSDelta may references an Identity Model, but only for the purpose of identifying the user and retrieving the OSID.
  • An alternative would be models are cached in one place and objects with references would not cache the whole model.

Testing

Unit testing

Added tests reproducing crash when encoding models with concurrency

  • Adds two tests that encode Identity Model and Properties Model while their aliases and tags dictionary are written to.
  • The tests were run manually in Xcode with "Run "testName" Repeatedly..." 100 times.
  • The crash would be reproduced before the changes in this PR.

Manual testing

Device: iPhone 13 with iOS 17.4

  • reproduced occasionally following similar steps to the tests, before changes in the PR

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@emawby emawby mentioned this pull request Apr 19, 2024
Closed
1 task
@nan-li nan-li force-pushed the fix/user_module_user_defaults_crashes branch from 4adea7b to 600a5db Compare April 19, 2024 23:03
@nan-li
add test reproducing crash when encoding models with concurrency
35f6ed8 
* Adds two tests that encode Identity Model and Properties Model while their aliases and tags dictionary are written to.
* The tests try to add data, clear data, and encode these models concurrently
@nan-li
lock access to model dictionaries during encode
8cdf446 
* Identity Model and Properties Model have access to their aliases and tags dictionary wrapped in a lock. However, this lock was missing from the encode method, and is necessary.
@nan-li
No need to hydrate identity model twice
8c28fba 
* I noticed we were always hydrating the same identity model twice.
* This is because we are parsing a fetch user response, we end up hydrating the identity model twice, unnecessarily, which drives caching it twice as well.
* We already hydrate the identity model attached to the request.
* Here, we were hydrating it again if the current user is the same. However, if the current user is the same, the model is the same and has already been hydrated.
@nan-li
Update Identity Model to cache itself less frequently
ff4705a 
* Removing multiple aliases via `removeAliases(array)` will now be done in one go:
  - Creates one OSDelta that will be parsed by the executor into individual Remove Alias Requests
  - Caches the new aliases once instead of in a loop for each label to remove
* Hydrating aliases will now be done in one go as well:
  - Now, cache the new aliases once at the end. Prior, we cache the aliases after each alias as we hydrate.
* In addition, put the call to `self.set` outside of the tagsLock, since the call to `self.set` will drive encoding itself, which also requires the tagsLock. The UnfairLock does not handle nested locking.
@nan-li
Use a recursive lock
ac60efc 
* Drop UnfairLock and use NSRecursiveLock
* UnfairLock is fast, but it doesn’t support recursive locking. So, it crashes whenever you lock it twice from the same thread.
* So far, we haven't encountered this, but this is possible, especially if we are not careful about how changes are propagated up.
@nan-li
Move properties model's change propagation outside of the lock
db031ff 
* Keep the lock minimal, just around write and read access to the tags dictionary
* Move the events-driving outside of the lock, that create Deltas and caches the model, etc.
@nan-li
[dev app] add remove tag
98908ee 
* Adding tags with blank value will remove the tag
@nan-li nan-li force-pushed the fix/user_module_user_defaults_crashes branch from 48e6983 to 98908ee Compare April 19, 2024 23:09
@nan-li nan-li force-pushed the fix/user_module_user_defaults_crashes branch from 21661e5 to 98908ee Compare April 22, 2024 18:07
@nan-li nan-li merged commit 4bf6826 into main Apr 29, 2024
7 checks passed
@nan-li nan-li deleted the fix/user_module_user_defaults_crashes branch April 29, 2024 17:23
This was referenced Apr 30, 2024
Merged
Merged
@emawby emawby mentioned this pull request May 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emawby emawby emawby approved these changes

@jkasten2 jkasten2 Awaiting requested review from jkasten2

@jinliu9508 jinliu9508 Awaiting requested review from jinliu9508

@shepherd-l shepherd-l Awaiting requested review from shepherd-l

@jennantilla jennantilla Awaiting requested review from jennantilla

Assignees

@emawby emawby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nan-li @emawby