Added retry/restart logic to RemoteSyncEngine without cancel downstream components #322
Conversation
...AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine+IncomingEventReconciliationQueueEvent.swift
Show resolved
Hide resolved
AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine+Action.swift
Show resolved
Hide resolved
AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine+Retryable.swift
Show resolved
Hide resolved
| @@ -17,6 +17,7 @@ enum RemoteSyncEngineEvent { | |||
| case subscriptionsActivated | |||
| case mutationQueueStarted | |||
| case syncStarted | |||
| case cleanedUp | |||
There was a problem hiding this comment.
BTW, while I'm thinking about it, I like the idea you're promoting (intentionally or not) of normalizing our naming: State can represent a point-in-time view of a system, and could be thought of as something managed internally. Event can represent an external report of a state transition, even if it doesn't literally represent a change from one State to another.
| self.mutationRetryNotifier = nil | ||
| self.stateMachine.notify(action: .receivedStart) |
There was a problem hiding this comment.
nit: indentation?
I just realized our indentation rule is not enforced by SwiftFormat... it's currently disabled. I wonder why...
There was a problem hiding this comment.
Hmm indentation seems fine to me? i'm selecting everything and hitting "Ctrl + i", and there doesn't seem to be any updates to the formatting of the code...
| do { | ||
| networkReachabilityPublisher = try reachability.reachabilityPublisher() | ||
| } catch { | ||
| log.error("\(#function): Unable to listen on reachability: \(error)") |
There was a problem hiding this comment.
is this unexpected? In case it fails what is the side-effect? I'm guessing in case it disconnects it won't attempt to reconnect, is that the case?
There was a problem hiding this comment.
I believe this code is correct. If the networkReachabilityPublisher does not initialized due to some exception being thrown, then our retry logic will be based purely on the exponential backoff (versus exponential backoff w/ using the status of reachability).
I looked into the reachability code, and it seems that it is possible for reachability to fail during initialization, so, we kept the interface to return an optional instance.
AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/RemoteSyncEngine.swift
Show resolved
Hide resolved
wooj2
force-pushed
the
master-RSEState-Restart
branch
from
2a49d3a to
782de9c
Compare
This handles logic to restart the remote sync engine, without actually canceling downstream components.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.