Fix cancel to ensure events do not continue firing #355
Conversation
| @@ -520,6 +528,9 @@ final class AppSyncSubscriptionWithSync<Subscription: GraphQLSubscription, BaseQ | |||
| /// Cancels and releases the subscription watcher, cancels active timers, and unregisters for system notifications. After | |||
| /// invoking this method, the instance will be eligible for release. | |||
| private func internalCancel() { | |||
| isCancelled = true | |||
| internalStateSyncQueue.cancelAllOperations() | |||
| internalStateSyncQueue.isSuspended = true | |||
There was a problem hiding this comment.
do you think we can just check internalStateSyncQueue.isSuspended to be true to not continue firing, instead of having the isCancelled?
There was a problem hiding this comment.
What do you think of changing isCancelled definition to:
private var isCancelled: Bool {
set {
internalStateSyncQueue.isSuspended = newValue
}
get {
return internalStateSyncQueue.isSuspended
}
}Just so it is intentful of what is going on, and is also similar to AWSAppSyncSubscriptionWatcher. Otherwise, I don't mind using internalStateSyncQueue.isSuspended 😄
There was a problem hiding this comment.
I think that might confusing if the internalStateSyncQueue is suspended for other reasons and isCancelled represents that state. Then we should just name it isOperationsSuspended or something. My original thought was that if we already have a property that we can check then why not just use it and reduce introducing a new variable. Thanks for calling out AWSAppSyncSubscriptionWatcher, i'd say i think we should stick with the same pattern and keep what you currently have in the PR then
Issue #342, if available:
Description of changes:
calling cancel and holding strong retain cycle will not actually cancel as there are still operations that might exist on the internal operation queue.