-
Notifications
You must be signed in to change notification settings - Fork 200
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
fix: Bug where subscription connections happen at the same time #389
Conversation
self.modelReconciliationQueueSinks = [:] | ||
self.eventReconciliationQueueTopic = PassthroughSubject<IncomingEventReconciliationQueueEvent, DataStoreError>() | ||
self.reconciliationQueues = [:] | ||
self.reconciliationQueueConnectionStatus = [:] | ||
self.modelReconciliationQueueFactory = modelReconciliationQueueFactory ?? | ||
AWSModelReconciliationQueue.init(modelType:storageAdapter:api:incomingSubscriptionEvents:) | ||
self.connectionStatusOperationQueue = DispatchQueue(label: "com.amazonaws.DataStore.AWSIncomingEventReconciliationQueue") |
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.
- This is a dispatch queue, not an operation queue
- Add a
target
of a serial queue for the whole SyncEngine system, to help prevent thread explosions and increase performance. (I don't know whether we already have a top-level SyncEngine subsystem queue, so if you want to follow up on this with a separate PR, feel free.)
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.
- updated naming
- Added to do, to be followed up soon
reconciliationQueueConnectionStatus[modelName] = true | ||
if reconciliationQueueConnectionStatus.count == reconciliationQueues.count { | ||
eventReconciliationQueueTopic.send(.initialized) | ||
connectionStatusOperationQueue.async { |
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.
Should protect the assignment in onReceiveCompletion
, as well as cancel
and any other assignments of instance variables that I missed in my scan.
943451c
to
cb1b724
Compare
cb1b724
to
a3901b3
Compare
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.
Consider adding a link to a backlog item for your TODO, otherwise LGTM
...e/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/AWSIncomingEventReconciliationQueue.swift
Outdated
Show resolved
Hide resolved
a3901b3
to
86d24aa
Compare
Prior to this fix, the crash looked something like this:
Seems like a race condition, where both subscription connections are coming in very quick succession, and somehow we are getting into a read/write race condition with
reconciliationQueueConnectionStatus
inside ofAWSIncomingEventReconciliationQueue
.To reproduce this issue, you can remove the
connectionStatusOperationQueue.async
and run the unit test inAWSIncomingEventReconciliationQueueTests
to see a very similar segfault:By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.