diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index 322bb768de3..354a8050718 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,4 +1,10 @@ # Unreleased +- [changed] If the SDK's attempt to connect to the Cloud Firestore backend + neither succeeds nor fails within 10 seconds, the SDK will consider itself + "offline", causing get() calls to resolve with cached results, rather than + continuing to wait. + +# 0.3.2 - [fixed] Fixed a regression in Firebase JS release 4.9.0 that could in certain cases result in an "OnlineState should not affect limbo documents." assertion crash when the client loses its network connection. diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index 12e07e48e32..2b4311886c0 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -248,9 +248,9 @@ export class QueryListener { return true; } - // NOTE: We consider OnlineState.Unknown as online (it should become Failed + // NOTE: We consider OnlineState.Unknown as online (it should become Offline // or Online if we wait long enough). - const maybeOnline = onlineState !== OnlineState.Failed; + const maybeOnline = onlineState !== OnlineState.Offline; // Don't raise the event if we're online, aren't synced yet (checked // above) and are waiting for a sync. if (this.options.waitForSyncWhenOnline && maybeOnline) { @@ -262,7 +262,7 @@ export class QueryListener { } // Raise data from cache if we have any documents or we are offline - return !snap.docs.isEmpty() || onlineState === OnlineState.Failed; + return !snap.docs.isEmpty() || onlineState === OnlineState.Offline; } private shouldRaiseEvent(snap: ViewSnapshot): boolean { diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 63e349a5fd1..7829d07eabb 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -288,6 +288,7 @@ export class FirestoreClient { this.remoteStore = new RemoteStore( this.localStore, datastore, + this.asyncQueue, onlineStateChangedHandler ); diff --git a/packages/firestore/src/core/types.ts b/packages/firestore/src/core/types.ts index 49693083a76..c5ef83a76bc 100644 --- a/packages/firestore/src/core/types.ts +++ b/packages/firestore/src/core/types.ts @@ -51,12 +51,12 @@ export enum OnlineState { * reached after a successful connection and there has been at least one * successful message received from the backends. */ - Healthy, + Online, /** * The client is either trying to establish a connection but failing, or it * has been explicitly marked offline via a call to disableNetwork(). * Higher-level components should operate in offline mode. */ - Failed + Offline } diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts index 8da76d74faa..b7d811fa6af 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -276,7 +276,7 @@ export class View { * ViewChange if the view's syncState changes as a result. */ applyOnlineStateChange(onlineState: OnlineState): ViewChange { - if (this.current && onlineState === OnlineState.Failed) { + if (this.current && onlineState === OnlineState.Offline) { // If we're offline, set `current` to false and then call applyChanges() // to refresh our syncState and generate a ViewChange as appropriate. We // are guaranteed to get a new TargetChange that sets `current` back to diff --git a/packages/firestore/src/remote/online_state_tracker.ts b/packages/firestore/src/remote/online_state_tracker.ts new file mode 100644 index 00000000000..147f8d49323 --- /dev/null +++ b/packages/firestore/src/remote/online_state_tracker.ts @@ -0,0 +1,172 @@ +/** + * Copyright 2018 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { OnlineState } from '../core/types'; +import * as log from '../util/log'; +import { assert } from '../util/assert'; +import { AsyncQueue, TimerId } from '../util/async_queue'; +import { CancelablePromise } from '../util/promise'; + +const LOG_TAG = 'OnlineStateTracker'; + +// To deal with transient failures, we allow multiple stream attempts before +// giving up and transitioning from OnlineState.Unknown to Offline. +const MAX_WATCH_STREAM_FAILURES = 2; + +// To deal with stream attempts that don't succeed or fail in a timely manner, +// we have a timeout for OnlineState to reach Online or Offline. +// If the timeout is reached, we transition to Offline rather than waiting +// indefinitely. +const ONLINE_STATE_TIMEOUT_MS = 10 * 1000; + +/** + * A component used by the RemoteStore to track the OnlineState (that is, + * whether or not the client as a whole should be considered to be online or + * offline), implementing the appropriate heuristics. + * + * In particular, when the client is trying to connect to the backend, we + * allow up to MAX_WATCH_STREAM_FAILURES within ONLINE_STATE_TIMEOUT_MS for + * a connection to succeed. If we have too many failures or the timeout elapses, + * then we set the OnlineState to Offline, and the client will behave as if + * it is offline (get()s will return cached data, etc.). + */ +export class OnlineStateTracker { + /** The current OnlineState. */ + private state = OnlineState.Unknown; + + /** + * A count of consecutive failures to open the stream. If it reaches the + * maximum defined by MAX_WATCH_STREAM_FAILURES, we'll set the OnlineState to + * Offline. + */ + private watchStreamFailures = 0; + + /** + * A timer that elapses after ONLINE_STATE_TIMEOUT_MS, at which point we + * transition from OnlineState.Unknown to OnlineState.Offline without waiting + * for the stream to actually fail (MAX_WATCH_STREAM_FAILURES times). + */ + private onlineStateTimer: CancelablePromise | null = null; + + /** + * Whether the client should log a warning message if it fails to connect to + * the backend (initially true, cleared after a successful stream, or if we've + * logged the message already). + */ + private shouldWarnClientIsOffline = true; + + constructor( + private asyncQueue: AsyncQueue, + private onlineStateHandler: (onlineState: OnlineState) => void + ) {} + + /** + * Called by RemoteStore when a watch stream is started. + * + * It sets the OnlineState to Unknown and starts the onlineStateTimer + * if necessary. + */ + handleWatchStreamStart(): void { + this.setAndBroadcast(OnlineState.Unknown); + + if (this.onlineStateTimer === null) { + this.onlineStateTimer = this.asyncQueue.enqueueAfterDelay( + TimerId.OnlineStateTimeout, + ONLINE_STATE_TIMEOUT_MS, + () => { + this.onlineStateTimer = null; + assert( + this.state === OnlineState.Unknown, + 'Timer should be canceled if we transitioned to a different state.' + ); + log.debug( + LOG_TAG, + `Watch stream didn't reach online or offline within ` + + `${ONLINE_STATE_TIMEOUT_MS}ms. Considering client offline.` + ); + this.logClientOfflineWarningIfNecessary(); + this.setAndBroadcast(OnlineState.Offline); + + // NOTE: handleWatchStreamFailure() will continue to increment + // watchStreamFailures even though we are already marked Offline, + // but this is non-harmful. + + return Promise.resolve(); + } + ); + } + } + + /** + * Updates our OnlineState as appropriate after the watch stream reports a + * failure. The first failure moves us to the 'Unknown' state. We then may + * allow multiple failures (based on MAX_WATCH_STREAM_FAILURES) before we + * actually transition to the 'Offline' state. + */ + handleWatchStreamFailure(): void { + if (this.state === OnlineState.Online) { + this.setAndBroadcast(OnlineState.Unknown); + } else { + this.watchStreamFailures++; + if (this.watchStreamFailures >= MAX_WATCH_STREAM_FAILURES) { + this.clearOnlineStateTimer(); + this.logClientOfflineWarningIfNecessary(); + this.setAndBroadcast(OnlineState.Offline); + } + } + } + + /** + * Explicitly sets the OnlineState to the specified state. + * + * Note that this resets our timers / failure counters, etc. used by our + * Offline heuristics, so must not be used in place of + * handleWatchStreamStart() and handleWatchStreamFailure(). + */ + set(newState: OnlineState): void { + this.clearOnlineStateTimer(); + this.watchStreamFailures = 0; + + if (newState === OnlineState.Online) { + // We've connected to watch at least once. Don't warn the developer + // about being offline going forward. + this.shouldWarnClientIsOffline = false; + } + + this.setAndBroadcast(newState); + } + + private setAndBroadcast(newState: OnlineState): void { + if (newState !== this.state) { + this.state = newState; + this.onlineStateHandler(newState); + } + } + + private logClientOfflineWarningIfNecessary(): void { + if (this.shouldWarnClientIsOffline) { + log.error('Could not reach Firestore backend.'); + this.shouldWarnClientIsOffline = false; + } + } + + private clearOnlineStateTimer(): void { + if (this.onlineStateTimer !== null) { + this.onlineStateTimer.cancel(); + this.onlineStateTimer = null; + } + } +} diff --git a/packages/firestore/src/remote/persistent_stream.ts b/packages/firestore/src/remote/persistent_stream.ts index 0fd3c6573fc..df573a763b0 100644 --- a/packages/firestore/src/remote/persistent_stream.ts +++ b/packages/firestore/src/remote/persistent_stream.ts @@ -265,17 +265,6 @@ export abstract class PersistentStream< IDLE_TIMEOUT_MS, () => this.handleIdleCloseTimer() ); - - this.inactivityTimerPromise.catch((err: FirestoreError) => { - // When the AsyncQueue gets drained during testing, pending Promises - // (including these idle checks) will get rejected. We special-case - // these cancelled idle checks to make sure that these specific Promise - // rejections are not considered unhandled. - assert( - err.code === Code.CANCELLED, - `Received unexpected error in idle timeout closure. Expected CANCELLED, but was: ${err}` - ); - }); } } @@ -529,7 +518,7 @@ export class PersistentListenStream extends PersistentStream< ) { super( queue, - TimerId.ListenStreamConnection, + TimerId.ListenStreamConnectionBackoff, TimerId.ListenStreamIdle, connection, credentials @@ -637,7 +626,7 @@ export class PersistentWriteStream extends PersistentStream< ) { super( queue, - TimerId.WriteStreamConnection, + TimerId.WriteStreamConnectionBackoff, TimerId.WriteStreamIdle, connection, credentials diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index e65f11f075f..060edbae56b 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -49,19 +49,14 @@ import { WatchTargetChange, WatchTargetChangeState } from './watch_change'; +import { OnlineStateTracker } from './online_state_tracker'; +import { AsyncQueue } from '../util/async_queue'; const LOG_TAG = 'RemoteStore'; // TODO(b/35853402): Negotiate this with the stream. const MAX_PENDING_WRITES = 10; -// The RemoteStore notifies an onlineStateHandler with OnlineState.Failed 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 onlineStateHandler. -const ONLINE_ATTEMPTS_BEFORE_FAILURE = 2; - /** * RemoteStore - An interface to remotely stored data, basically providing a * wrapper around the Datastore that is more reliable for the rest of the @@ -117,17 +112,7 @@ export class RemoteStore { private watchStream: PersistentListenStream = null; private writeStream: PersistentWriteStream = null; - /** - * The online state of the watch stream. The state is set to healthy if and - * only if there are messages received by the backend. - */ - private watchStreamOnlineState = OnlineState.Unknown; - - /** A count of consecutive failures to open the stream. */ - private watchStreamFailures = 0; - - /** Whether the client should fire offline warning. */ - private shouldWarnOffline = true; + private onlineStateTracker: OnlineStateTracker; constructor( /** @@ -137,8 +122,14 @@ export class RemoteStore { private localStore: LocalStore, /** The client-side proxy for interacting with the backend. */ private datastore: Datastore, - private onlineStateHandler: (onlineState: OnlineState) => void - ) {} + asyncQueue: AsyncQueue, + onlineStateHandler: (onlineState: OnlineState) => void + ) { + this.onlineStateTracker = new OnlineStateTracker( + asyncQueue, + onlineStateHandler + ); + } /** SyncEngine to notify of watch and write events. */ syncEngine: RemoteSyncer; @@ -151,51 +142,6 @@ export class RemoteStore { return this.enableNetwork(); } - /** - * Updates our OnlineState to the new state, updating local state - * and notifying the onlineStateHandler as appropriate. Idempotent. - */ - private updateOnlineState(newState: OnlineState): void { - if (newState !== this.watchStreamOnlineState) { - if (newState === OnlineState.Healthy) { - // We've connected to watch at least once. Don't warn the developer about - // being offline going forward. - this.shouldWarnOffline = false; - } else if (newState === OnlineState.Unknown) { - // 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 ONLINE_ATTEMPTS_BEFORE_FAILURE - // attempts before failing, so we reset the count here. - this.watchStreamFailures = 0; - } - this.watchStreamOnlineState = newState; - this.onlineStateHandler(newState); - } - } - - /** - * Updates our OnlineState as appropriate after the watch stream reports a - * failure. The first failure moves us to the 'Unknown' state. We then may - * allow multiple failures (based on ONLINE_ATTEMPTS_BEFORE_FAILURE) before we - * actually transition to OnlineState.Failed. - */ - private updateOnlineStateAfterFailure(): void { - if (this.watchStreamOnlineState === OnlineState.Healthy) { - this.updateOnlineState(OnlineState.Unknown); - } else { - this.watchStreamFailures++; - if (this.watchStreamFailures >= ONLINE_ATTEMPTS_BEFORE_FAILURE) { - if (this.shouldWarnOffline) { - log.error('Could not reach Firestore backend.'); - this.shouldWarnOffline = false; - } - this.updateOnlineState(OnlineState.Failed); - } - } - } - private isNetworkEnabled(): boolean { assert( (this.watchStream == null) === (this.writeStream == null), @@ -220,10 +166,10 @@ export class RemoteStore { if (this.shouldStartWatchStream()) { this.startWatchStream(); + } else { + this.onlineStateTracker.set(OnlineState.Unknown); } - this.updateOnlineState(OnlineState.Unknown); - return this.fillWritePipeline(); // This may start the writeStream. }); } @@ -234,8 +180,8 @@ export class RemoteStore { */ async disableNetwork(): Promise { this.disableNetworkInternal(); - // Set the OnlineState to failed so get()'s return from cache, etc. - this.updateOnlineState(OnlineState.Failed); + // Set the OnlineState to Offline so get()s return from cache, etc. + this.onlineStateTracker.set(OnlineState.Offline); } /** @@ -259,9 +205,9 @@ export class RemoteStore { shutdown(): Promise { log.debug(LOG_TAG, 'RemoteStore shutting down.'); this.disableNetworkInternal(); - // Set the OnlineState to Unknown (rather than Failed) to avoid potentially + // Set the OnlineState to Unknown (rather than Offline) to avoid potentially // triggering spurious listener events with cached data, etc. - this.updateOnlineState(OnlineState.Unknown); + this.onlineStateTracker.set(OnlineState.Unknown); return Promise.resolve(); } @@ -336,11 +282,13 @@ export class RemoteStore { onClose: this.onWatchStreamClose.bind(this), onWatchChange: this.onWatchStreamChange.bind(this) }); + + this.onlineStateTracker.handleWatchStreamStart(); } /** - * Returns whether the watch stream should be started because there are - * active targets trying to be listened too + * Returns whether the watch stream should be started because it's necessary + * and has not yet been started. */ private shouldStartWatchStream(): boolean { return ( @@ -376,16 +324,16 @@ export class RemoteStore { ); this.cleanUpWatchStreamState(); + this.onlineStateTracker.handleWatchStreamFailure(); // If there was an error, retry the connection. if (this.shouldStartWatchStream()) { - this.updateOnlineStateAfterFailure(); this.startWatchStream(); } else { // No need to restart 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 - this.updateOnlineState(OnlineState.Unknown); + this.onlineStateTracker.set(OnlineState.Unknown); } } @@ -393,8 +341,8 @@ export class RemoteStore { watchChange: WatchChange, snapshotVersion: SnapshotVersion ): Promise { - // Mark the connection as healthy because we got a message from the server - this.updateOnlineState(OnlineState.Healthy); + // Mark the client as online since we got a message from the server + this.onlineStateTracker.set(OnlineState.Online); if ( watchChange instanceof WatchTargetChange && @@ -804,7 +752,7 @@ export class RemoteStore { // for the new user and re-fill the write pipeline with new mutations from the LocalStore // (since mutations are per-user). this.disableNetworkInternal(); - this.updateOnlineState(OnlineState.Unknown); + this.onlineStateTracker.set(OnlineState.Unknown); return this.enableNetwork(); } } diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index 453e720c373..29440d72a50 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -27,12 +27,30 @@ type TimerHandle = any; * Wellknown "timer" IDs used when scheduling delayed operations on the * AsyncQueue. These IDs can then be used from tests to check for the presence * of operations or to run them early. + * + * The string values are used when encoding these timer IDs in JSON spec tests. */ export enum TimerId { - ListenStreamIdle, - ListenStreamConnection, - WriteStreamIdle, - WriteStreamConnection + /** All can be used with runDelayedOperationsEarly() to run all timers. */ + All = 'all', + + /** + * The following 4 timers are used in persistent_stream.ts for the listen and + * write streams. The "Idle" timer is used to close the stream due to + * inactivity. The "ConnectionBackoff" timer is used to restart a stream once + * the appropriate backoff delay has elapsed. + */ + ListenStreamIdle = 'listen_stream_idle', + ListenStreamConnectionBackoff = 'listen_stream_connection_backoff', + WriteStreamIdle = 'write_stream_idle', + WriteStreamConnectionBackoff = 'write_stream_connection_backoff', + + /** + * A timer used in online_state_tracker.ts to transition from + * OnlineState.Unknown to Offline after a set timeout, rather than waiting + * indefinitely for success or failure. + */ + OnlineStateTimeout = 'online_state_timeout' } /** @@ -55,7 +73,12 @@ class DelayedOperation implements CancelablePromise { readonly targetTimeMs: number, private readonly op: () => Promise, private readonly removalCallback: (op: DelayedOperation) => void - ) {} + ) { + // It's normal for the deferred promise to be canceled (due to cancellation) + // and so we attach a dummy catch callback to avoid + // 'UnhandledPromiseRejectionWarning' log spam. + this.deferred.promise.catch(err => {}); + } /** * Creates and returns a DelayedOperation that has been scheduled to be @@ -221,9 +244,7 @@ export class AsyncQueue { // ops with the same timer id in the queue, so defensively reject them. assert( !this.containsDelayedOperation(timerId), - `Attempted to schedule multiple operations with timer id ${ - TimerId[timerId] - }.` + `Attempted to schedule multiple operations with timer id ${timerId}.` ); const delayedOp = DelayedOperation.createAndSchedule( @@ -279,16 +300,17 @@ export class AsyncQueue { /** * For Tests: Runs some or all delayed operations early. * - * @param lastTimerId If specified, only delayed operations up to and - * including this TimerId will be drained. Throws if no such operation - * exists. + * @param lastTimerId Delayed operations up to and including this TimerId will + * be drained. Throws if no such operation exists. Pass TimerId.All to run + * all delayed operations. * @returns a Promise that resolves once all operations have been run. */ - runDelayedOperationsEarly(lastTimerId?: TimerId): Promise { + runDelayedOperationsEarly(lastTimerId: TimerId): Promise { // Note that draining may generate more delayed ops, so we do that first. return this.drain().then(() => { assert( - lastTimerId === undefined || this.containsDelayedOperation(lastTimerId), + lastTimerId === TimerId.All || + this.containsDelayedOperation(lastTimerId), `Attempted to drain to missing operation ${lastTimerId}` ); @@ -297,7 +319,7 @@ export class AsyncQueue { for (const op of this.delayedOperations) { op.skipDelay(); - if (lastTimerId !== undefined && op.timerId === lastTimerId) { + if (lastTimerId !== TimerId.All && op.timerId === lastTimerId) { break; } } diff --git a/packages/firestore/test/integration/remote/stream.test.ts b/packages/firestore/test/integration/remote/stream.test.ts index 0b778e223b5..8faabc26763 100644 --- a/packages/firestore/test/integration/remote/stream.test.ts +++ b/packages/firestore/test/integration/remote/stream.test.ts @@ -259,7 +259,7 @@ describe('Write Stream', () => { writeStream.writeMutations(SINGLE_MUTATION); return streamListener.awaitCallback('mutationResult'); }) - .then(() => queue.runDelayedOperationsEarly()) + .then(() => queue.runDelayedOperationsEarly(TimerId.All)) .then(() => { expect(writeStream.isOpen()).to.be.true; }); diff --git a/packages/firestore/test/unit/core/event_manager.test.ts b/packages/firestore/test/unit/core/event_manager.test.ts index 1e41dd1e778..34d69418d9d 100644 --- a/packages/firestore/test/unit/core/event_manager.test.ts +++ b/packages/firestore/test/unit/core/event_manager.test.ts @@ -139,8 +139,8 @@ describe('EventManager', () => { eventManager.listen(fakeListener1); expect(events).to.deep.equal([OnlineState.Unknown]); - eventManager.applyOnlineStateChange(OnlineState.Healthy); - expect(events).to.deep.equal([OnlineState.Unknown, OnlineState.Healthy]); + eventManager.applyOnlineStateChange(OnlineState.Online); + expect(events).to.deep.equal([OnlineState.Unknown, OnlineState.Online]); }); }); @@ -423,10 +423,10 @@ describe('QueryListener', () => { const changes3 = view.computeDocChanges(documentUpdates()); const snap3 = view.applyChanges(changes3, ackTarget(doc1, doc2)).snapshot!; - listener.applyOnlineStateChange(OnlineState.Healthy); // no event + listener.applyOnlineStateChange(OnlineState.Online); // no event listener.onViewSnapshot(snap1); // no event listener.applyOnlineStateChange(OnlineState.Unknown); // no event - listener.applyOnlineStateChange(OnlineState.Healthy); // no event + listener.applyOnlineStateChange(OnlineState.Online); // no event listener.onViewSnapshot(snap2); // no event listener.onViewSnapshot(snap3); // event because synced @@ -461,11 +461,11 @@ describe('QueryListener', () => { const changes2 = view.computeDocChanges(documentUpdates(doc2)); const snap2 = view.applyChanges(changes2).snapshot!; - listener.applyOnlineStateChange(OnlineState.Healthy); // no event + listener.applyOnlineStateChange(OnlineState.Online); // no event listener.onViewSnapshot(snap1); // no event - listener.applyOnlineStateChange(OnlineState.Failed); // event - listener.applyOnlineStateChange(OnlineState.Healthy); // no event - listener.applyOnlineStateChange(OnlineState.Failed); // no event + listener.applyOnlineStateChange(OnlineState.Offline); // event + listener.applyOnlineStateChange(OnlineState.Online); // no event + listener.applyOnlineStateChange(OnlineState.Offline); // no event listener.onViewSnapshot(snap2); // another event const expectedSnap1 = { @@ -499,9 +499,9 @@ describe('QueryListener', () => { const changes1 = view.computeDocChanges(documentUpdates()); const snap1 = view.applyChanges(changes1).snapshot!; - listener.applyOnlineStateChange(OnlineState.Healthy); // no event + listener.applyOnlineStateChange(OnlineState.Online); // no event listener.onViewSnapshot(snap1); // no event - listener.applyOnlineStateChange(OnlineState.Failed); // event + listener.applyOnlineStateChange(OnlineState.Offline); // event const expectedSnap = { query, @@ -525,7 +525,7 @@ describe('QueryListener', () => { const changes1 = view.computeDocChanges(documentUpdates()); const snap1 = view.applyChanges(changes1).snapshot!; - listener.applyOnlineStateChange(OnlineState.Failed); + listener.applyOnlineStateChange(OnlineState.Offline); listener.onViewSnapshot(snap1); const expectedSnap = { diff --git a/packages/firestore/test/unit/specs/offline_spec.test.ts b/packages/firestore/test/unit/specs/offline_spec.test.ts index d832400e034..af23a2ba614 100644 --- a/packages/firestore/test/unit/specs/offline_spec.test.ts +++ b/packages/firestore/test/unit/specs/offline_spec.test.ts @@ -21,6 +21,7 @@ import { doc, path } from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; +import { TimerId } from '../../../src/util/async_queue'; describeSpec('Offline:', [], () => { specTest('Empty queries are resolved if client goes offline', [], () => { @@ -146,4 +147,38 @@ describeSpec('Offline:', [], () => { .expectLimboDocs() ); }); + + specTest('OnlineState timeout triggers offline behavior', [], () => { + const query = Query.atPath(path('collection')); + const docA = doc('collection/a', 1000, { key: 'a' }); + return ( + spec() + .userListens(query) + + // OnlineState timer should trigger offline behavior (fromCache=true). + .runTimer(TimerId.OnlineStateTimeout) + .expectEvents(query, { + fromCache: true + }) + + // We should get no further events for failed connection attempts. + .watchStreamCloses(Code.UNAVAILABLE) + .watchStreamCloses(Code.UNAVAILABLE) + + // We should get events after a successful connection. + .watchAcksFull(query, 1000, docA) + .expectEvents(query, { added: [docA], fromCache: false }) + + // Running timers should have no effect now. + .runTimer(TimerId.All) + + // After a disconnect, the timer should become active again. + .watchStreamCloses(Code.UNAVAILABLE) + .restoreListen(query, 'resume-token-1000') + .runTimer(TimerId.OnlineStateTimeout) + .expectEvents(query, { + fromCache: true + }) + ); + }); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index c59434b0f4e..b13afa61a33 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -41,6 +41,7 @@ import { SpecStep, SpecWatchFilter } from './spec_test_runner'; +import { TimerId } from '../../../src/util/async_queue'; /** * Provides a high-level language to construct spec tests that can be exported @@ -194,6 +195,12 @@ export class SpecBuilder { return this; } + runTimer(timerId: TimerId) { + this.nextStep(); + this.currentStep = { runTimer: timerId }; + return this; + } + changeUser(uid: string | null): SpecBuilder { this.nextStep(); this.currentStep = { changeUser: uid }; diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 2c202b0c772..b5e5c9f54ec 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -384,6 +384,7 @@ abstract class TestRunner { this.remoteStore = new RemoteStore( this.localStore, this.datastore, + this.queue, onlineStateChangedHandler ); @@ -463,6 +464,8 @@ abstract class TestRunner { return this.doWriteAck(step.writeAck!); } else if ('failWrite' in step) { return this.doFailWrite(step.failWrite!); + } else if ('runTimer' in step) { + return this.doRunTimer(step.runTimer!); } else if ('enableNetwork' in step) { return step.enableNetwork! ? this.doEnableNetwork() @@ -699,7 +702,7 @@ abstract class TestRunner { // The watch stream should re-open if we have active listeners. if (!this.queryListeners.isEmpty()) { await this.queue.runDelayedOperationsEarly( - TimerId.ListenStreamConnection + TimerId.ListenStreamConnectionBackoff ); await this.connection.waitForWatchOpen(); } @@ -759,6 +762,14 @@ abstract class TestRunner { }); } + private async doRunTimer(timer: string): Promise { + // We assume the timer string is a valid TimerID enum value, but if it's + // not, then there won't be a matching item on the queue and + // runDelayedOperationsEarly() will throw. + const timerId = timer as TimerId; + await this.queue.runDelayedOperationsEarly(timerId); + } + private async doDisableNetwork(): Promise { // Make sure to execute all writes that are currently queued. This allows us // to assert on the total number of requests sent before shutdown. @@ -888,11 +899,9 @@ abstract class TestRunner { ) ); expect(actualTarget.query).to.deep.equal(expectedTarget.query); - expect(actualTarget.targetId).to.deep.equal(expectedTarget.targetId); - expect(actualTarget.readTime).to.deep.equal(expectedTarget.readTime); - expect(actualTarget.resumeToken).to.deep.equal( - expectedTarget.resumeToken - ); + expect(actualTarget.targetId).to.equal(expectedTarget.targetId); + expect(actualTarget.readTime).to.equal(expectedTarget.readTime); + expect(actualTarget.resumeToken).to.equal(expectedTarget.resumeToken); delete actualTargets[targetId]; }); expect(obj.size(actualTargets)).to.equal( @@ -1089,6 +1098,12 @@ export interface SpecStep { /** Fail a write */ failWrite?: SpecWriteFailure; + /** + * Run a queued timer task (without waiting for the delay to expire). See + * TimerId enum definition for possible values). + */ + runTimer?: string; + /** Enable or disable RemoteStore's network connection. */ enableNetwork?: boolean; diff --git a/packages/firestore/test/unit/util/async_queue.test.ts b/packages/firestore/test/unit/util/async_queue.test.ts index f1c49f66251..c089a56917c 100644 --- a/packages/firestore/test/unit/util/async_queue.test.ts +++ b/packages/firestore/test/unit/util/async_queue.test.ts @@ -23,9 +23,9 @@ import { Code } from '../../../src/util/error'; describe('AsyncQueue', () => { // We reuse these TimerIds for generic testing. - const timerId1 = TimerId.ListenStreamConnection; + const timerId1 = TimerId.ListenStreamConnectionBackoff; const timerId2 = TimerId.ListenStreamIdle; - const timerId3 = TimerId.WriteStreamConnection; + const timerId3 = TimerId.WriteStreamConnectionBackoff; it('schedules ops in right order', () => { const queue = new AsyncQueue(); @@ -161,7 +161,7 @@ describe('AsyncQueue', () => { err => expect(err.code === Code.CANCELLED) ); - await queue.runDelayedOperationsEarly(); + await queue.runDelayedOperationsEarly(TimerId.All); expect(completedSteps).to.deep.equal([1]); }); @@ -174,7 +174,7 @@ describe('AsyncQueue', () => { queue.enqueueAfterDelay(timerId2, 10000, () => doStep(3)); queue.enqueue(() => doStep(2)); - await queue.runDelayedOperationsEarly(); + await queue.runDelayedOperationsEarly(TimerId.All); expect(completedSteps).to.deep.equal([1, 2, 3, 4]); });