[Bug] Add Dispatch Queues to all executors #1454
Merged
Conversation
This file contains 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/add_dispatch_queues_to_all_executors
branch
from
df63d6f
to
c6b8bc6
Compare
* When testing concurrency, the Mock Client actually was synchronizing the requests, so crashes were not reproducible * By changing to a concurrent DispatchQueue, this is more reflective of the real Client's behavior
* Add a flag called `fireSuccessForAllRequests` that will fire the success callback for all requests so that they don't need to be manually set up using mock responses beforehand. * This is useful for tests that want the client to return success but don't need the details of particular requests.
* Synchronize access to the delta queue and request queues * Add test `testSubscriptionExecutorConcurrency` that reproduced crash that is then fixed by the changes here.
* Synchronize access to the delta queue and request queues * Add test `testIdentityExecutorConcurrency` that reproduced crash that is then fixed by the changes here.
* Synchronize access to the request queues * Add test `testUserExecutorConcurrency` that reproduced crash that is then fixed by the changes here. * Note that concurrent access in this executor should be mitigated already by the executor only sending one request at a time.
* Add test `testPropertyExecutorConcurrency`
nan-li
force-pushed
the
fix/add_dispatch_queues_to_all_executors
branch
from
36b0075
to
86335fa
Compare
nan-li
changed the base branch from
main
to
fix/clearing_notifs_when_swiping_notif_center
nan-li
force-pushed
the
fix/add_dispatch_queues_to_all_executors
branch
16 times, most recently
from
a6b5ed5
to
6dfdfd8
Compare
|
The core changes are ready for review... I am trying to fix flaky tests here but those can be moved to a different PR. |
* swiftlint error: Type Body Length Violation: Type body should span 350 lines or less excluding comments and whitespace: currently spans 354 lines (type_body_length) * No logic changes in this commit, only split up executing requests code into an extension
nan-li
force-pushed
the
fix/add_dispatch_queues_to_all_executors
branch
from
6dfdfd8
to
8248762
Compare
Base automatically changed from
fix/clearing_notifs_when_swiping_notif_center
to
main
June 27, 2024 18:04
This was referenced Jul 1, 2024
Merged
This was referenced Jul 3, 2024
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
Improve synchronization in the remaining executors (Identity, Subscription, User) to prevent concurrent access crashes.
Details
In #1376, a
DispatchQueuewas added to the Operation Repo and the Property Executor to serialize access to shared data. These two components encounter the most updates and frequent mutations by different threads. Though rare, the other executors can also encounter concurrent access.The changes for the Operation Repo and Property Executor appear to be mitigating crashes, so we are adding the same to all executors in this PR:
Motivation
We received a report of a crash that I believe is due to one of these remaining executors experiencing a concurrent state access / mutation.
Scope
Executors synchronize array access, mutation, and caching through a Dispatch Queue.
Testing
Unit testing
Manual testing
Unable to reproduce
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is