Fix crashes when encoding user models #1412
Merged
Commits on Apr 19, 2024
-
add test 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 try to add data, clear data, and encode these models concurrently
-
lock access to model dictionaries during encode
* 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.
-
No need to hydrate identity model twice
* 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.
-
Update Identity Model to cache itself less frequently
* 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.
-
* 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.
-
Move properties model's change propagation outside of the lock
* 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.
-
* Adding tags with blank value will remove the tag
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.