diff options
5 files changed, 208 insertions, 97 deletions
diff --git a/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m b/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m index b4c0b02..b83942b 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m +++ b/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m @@ -263,8 +263,10 @@ NS_ASSUME_NONNULL_BEGIN [NSError errorWithDomain:FIRFirestoreErrorDomain code:errorCode userInfo:userInfo]; [self.datastore failWatchStreamWithError:error]; - // Unlike web, stream should re-open synchronously - FSTAssert(self.datastore.isWatchStreamOpen, @"Watch stream is open"); + // Unlike web, stream should re-open synchronously (if we have any listeners) + if (self.queryListeners.count > 0) { + FSTAssert(self.datastore.isWatchStreamOpen, @"Watch stream is open"); + } } - (NSDictionary<FSTDocumentKey *, FSTBoxedTargetID *> *)currentLimboDocuments { diff --git a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json index e58bae1..f542a6e 100644 --- a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json @@ -35,6 +35,14 @@ "code": 14, "message": "Simulated Backend Error" } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } }, "expect": [ { @@ -116,6 +124,14 @@ "code": 14, "message": "Simulated Backend Error" } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } }, "expect": [ { @@ -147,5 +163,136 @@ } } ] + }, + "Removing all listeners delays \"Offline\" status on next listen": { + "describeName": "Offline:", + "itName": "Removing all listeners delays \"Offline\" status on next listen", + "tags": [], + "config": { + "useGarbageCollection": true + }, + "steps": [ + { + "userListen": [ + 2, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } + }, + "expect": [ + { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + }, + { + "userUnlisten": [ + 2, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": {} + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } + } + }, + { + "userListen": [ + 4, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "4": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + } + }, + "expect": [ + { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + } + ] } } diff --git a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json index edb0751..26bb520 100644 --- a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json @@ -489,19 +489,7 @@ "code": 14, "message": "Simulated Backend Error" } - }, - "expect": [ - { - "query": { - "path": "collection", - "filters": [], - "orderBys": [] - }, - "errorCode": 0, - "fromCache": true, - "hasPendingWrites": false - } - ] + } }, { "watchAck": [ diff --git a/Firestore/Example/Tests/SpecTests/json/write_spec_test.json b/Firestore/Example/Tests/SpecTests/json/write_spec_test.json index 60ed107..51d1aee 100644 --- a/Firestore/Example/Tests/SpecTests/json/write_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/write_spec_test.json @@ -4372,9 +4372,9 @@ } ] }, - "Writes that fail with code cancelled are retried": { + "Writes that fail with code resource_exhausted are not rejected": { "describeName": "Writes:", - "itName": "Writes that fail with code cancelled are retried", + "itName": "Writes that fail with code resource_exhausted are not rejected", "tags": [], "config": { "useGarbageCollection": true @@ -4435,73 +4435,16 @@ { "failWrite": { "error": { - "code": 1 + "code": 8 }, "expectUserCallback": false } - }, - { - "writeAck": { - "version": 1000, - "expectUserCallback": true - } - }, - { - "watchAck": [ - 2 - ] - }, - { - "watchEntity": { - "docs": [ - [ - "collection/key", - 0, - { - "foo": "bar" - } - ] - ], - "targets": [ - 2 - ] - } - }, - { - "watchCurrent": [ - [ - 2 - ], - "resume-token-1000" - ], - "watchSnapshot": 1000, - "expect": [ - { - "query": { - "path": "collection/key", - "filters": [], - "orderBys": [] - }, - "metadata": [ - [ - "collection/key", - 0, - { - "foo": "bar" - } - ] - ], - "errorCode": 0, - "fromCache": false, - "hasPendingWrites": false - } - ] } ] }, - "Writes that fail with code unknown are retried": { + "Writes that fail with code cancelled are retried": { "describeName": "Writes:", - "itName": "Writes that fail with code unknown are retried", + "itName": "Writes that fail with code cancelled are retried", "tags": [], "config": { "useGarbageCollection": true @@ -4562,7 +4505,7 @@ { "failWrite": { "error": { - "code": 2 + "code": 1 }, "expectUserCallback": false } @@ -4626,9 +4569,9 @@ } ] }, - "Writes that fail with code deadline-exceeded are retried": { + "Writes that fail with code unknown are retried": { "describeName": "Writes:", - "itName": "Writes that fail with code deadline-exceeded are retried", + "itName": "Writes that fail with code unknown are retried", "tags": [], "config": { "useGarbageCollection": true @@ -4689,7 +4632,7 @@ { "failWrite": { "error": { - "code": 4 + "code": 2 }, "expectUserCallback": false } @@ -4753,9 +4696,9 @@ } ] }, - "Writes that fail with code resource-exhausted are retried": { + "Writes that fail with code deadline-exceeded are retried": { "describeName": "Writes:", - "itName": "Writes that fail with code resource-exhausted are retried", + "itName": "Writes that fail with code deadline-exceeded are retried", "tags": [], "config": { "useGarbageCollection": true @@ -4816,7 +4759,7 @@ { "failWrite": { "error": { - "code": 8 + "code": 4 }, "expectUserCallback": false } diff --git a/Firestore/Source/Remote/FSTRemoteStore.m b/Firestore/Source/Remote/FSTRemoteStore.m index 85608e7..b4a6449 100644 --- a/Firestore/Source/Remote/FSTRemoteStore.m +++ b/Firestore/Source/Remote/FSTRemoteStore.m @@ -38,7 +38,15 @@ NS_ASSUME_NONNULL_BEGIN * The maximum number of pending writes to allow. * TODO(bjornick): Negotiate this value with the backend. */ -static const NSUInteger kMaxPendingWrites = 10; +static const int kMaxPendingWrites = 10; + +/** + * The FSTRemoteStore notifies an onlineStateDelegate with FSTOnlineStateFailed if we fail to + * connect to the backend. This subsequently triggers get() requests to fail or use cached data, + * etc. Unfortunately, our connections have historically been subject to various transient failures. + * So we wait for multiple failures before notifying the onlineStateDelegate. + */ +static const int kOnlineAttemptsBeforeFailure = 2; #pragma mark - FSTRemoteStore @@ -98,6 +106,9 @@ static const NSUInteger kMaxPendingWrites = 10; */ @property(nonatomic, assign) FSTOnlineState watchStreamOnlineState; +/** A count of consecutive failures to open the stream. */ +@property(nonatomic, assign) int watchStreamFailures; + #pragma mark Write Stream // The writeStream is null when the network is disabled. The non-null check is performed by // isNetworkEnabled. @@ -144,6 +155,34 @@ static const NSUInteger kMaxPendingWrites = 10; [self enableNetwork]; } +- (void)setOnlineStateToHealthy { + [self updateAndNotifyAboutOnlineState:FSTOnlineStateHealthy]; +} + +- (void)setOnlineStateToUnknown { + // The state is set to unknown when a healthy stream is closed (e.g. due to a token timeout) or + // when we have no active listens and therefore there's no need to start the stream. Assuming + // there is (possibly in the future) an active listen, then we will eventually move to state + // Online or Failed, but we always want to make at least kOnlineAttemptsBeforeFailure attempts + // before failing, so we reset the count here. + self.watchStreamFailures = 0; + [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown]; +} + +- (void)updateOnlineStateAfterFailure { + // The first failure after we are successfully connected moves us to the 'Unknown' state. We + // then may make multiple attempts (based on kOnlineAttemptsBeforeFailure) before we actually + // report failure. + if (self.watchStreamOnlineState == FSTOnlineStateHealthy) { + [self setOnlineStateToUnknown]; + } else { + self.watchStreamFailures++; + if (self.watchStreamFailures >= kOnlineAttemptsBeforeFailure) { + [self updateAndNotifyAboutOnlineState:FSTOnlineStateFailed]; + } + } +} + - (void)updateAndNotifyAboutOnlineState:(FSTOnlineState)watchStreamOnlineState { BOOL didChange = (watchStreamOnlineState != self.watchStreamOnlineState); self.watchStreamOnlineState = watchStreamOnlineState; @@ -178,7 +217,7 @@ static const NSUInteger kMaxPendingWrites = 10; [self fillWritePipeline]; // This may start the writeStream. // We move back to the unknown state because we might not want to re-open the stream - [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown]; + [self setOnlineStateToUnknown]; } - (void)disableNetwork { @@ -296,7 +335,7 @@ static const NSUInteger kMaxPendingWrites = 10; - (void)watchStreamDidChange:(FSTWatchChange *)change snapshotVersion:(FSTSnapshotVersion *)snapshotVersion { // Mark the connection as healthy because we got a message from the server. - [self updateAndNotifyAboutOnlineState:FSTOnlineStateHealthy]; + [self setOnlineStateToHealthy]; FSTWatchTargetChange *watchTargetChange = [change isKindOfClass:[FSTWatchTargetChange class]] ? (FSTWatchTargetChange *)change : nil; @@ -334,20 +373,12 @@ static const NSUInteger kMaxPendingWrites = 10; // If the watch stream closed due to an error, retry the connection if there are any active // watch targets. if ([self shouldStartWatchStream]) { - // If the connection fails before the stream has become healthy, consider the online state - // failed. Otherwise consider the online state unknown and the next connection attempt will - // resolve the online state. For example, if a healthy stream is closed due to an expired token - // we want to have one more try at reconnecting before we consider the connection unhealthy. - if (self.watchStreamOnlineState == FSTOnlineStateHealthy) { - [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown]; - } else { - [self updateAndNotifyAboutOnlineState:FSTOnlineStateFailed]; - } + [self updateOnlineStateAfterFailure]; [self.watchStream start]; } else { // We don't need to restart the watch stream because there are no active targets. The online // state is set to unknown because there is no active attempt at establishing a connection. - [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown]; + [self setOnlineStateToUnknown]; } } |