From e837136fb580950b8bed1d20fdbaa5f19b9b0033 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Thu, 4 Jan 2018 09:19:21 -0800 Subject: [PATCH 1/8] Add timeout to OnlineState tracking. --- packages/firestore/src/core/event_manager.ts | 6 +- packages/firestore/src/core/types.ts | 4 +- packages/firestore/src/core/view.ts | 2 +- .../src/remote/online_state_tracker.ts | 157 ++++++++++++++++++ packages/firestore/src/remote/remote_store.ts | 114 ++++--------- .../test/unit/core/event_manager.test.ts | 22 +-- 6 files changed, 205 insertions(+), 100 deletions(-) create mode 100644 packages/firestore/src/remote/online_state_tracker.ts diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts index 2dd1d3d34e9..a4a4857b8ef 100644 --- a/packages/firestore/src/core/event_manager.ts +++ b/packages/firestore/src/core/event_manager.ts @@ -251,9 +251,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) { @@ -265,7 +265,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/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 5bb06e85bcf..0a723a62c75 100644 --- a/packages/firestore/src/core/view.ts +++ b/packages/firestore/src/core/view.ts @@ -274,7 +274,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..d1efe10b94d --- /dev/null +++ b/packages/firestore/src/remote/online_state_tracker.ts @@ -0,0 +1,157 @@ +/** + * Copyright 2017 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'; + +const LOG_TAG = 'OnlineStateTracker'; + +// To deal with transient failures, we allow multiple stream attempts before +// giving up and transitioning 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 maximum timeout we'll wait for the stream to either succeed or fail +// MAX_WATCH_STREAM_FAILURES times, else we revert to OnlineState.Offline. +const MAX_WATCH_STREAM_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 MAX_WATCH_STREAM_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 MAX_WATCH_STREAM_TIMEOUT_MS, at which point we + * revert to OnlineState.Offline without waiting for the stream to actually + * fail (MAX_WATCH_STREAM_FAILURES times). + */ + // tslint:disable-next-line:no-any setTimeout() type differs on browser / node + private watchStreamTimer: any = 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 onlineStateHandler: (onlineState: OnlineState) => void) {} + + /** + * Called by RemoteStore when a watch stream is started. + * + * It sets the OnlineState to Unknown and starts a MAX_WATCH_STREAM_TIMEOUT_MS + * timer if necessary. + */ + handleWatchStreamStart(): void { + this.setAndBroadcast(OnlineState.Unknown); + + if (this.watchStreamTimer === null) { + this.watchStreamTimer = setTimeout(() => { + this.watchStreamTimer = 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 ` + + `${MAX_WATCH_STREAM_TIMEOUT_MS}ms. Considering client offline.` + ); + this.logClientOfflineWarningIfNecessary(); + this.setAndBroadcast(OnlineState.Offline); + }, MAX_WATCH_STREAM_TIMEOUT_MS); + } + } + + /** + * 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.clearWatchStreamTimer(); + 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.clearWatchStreamTimer(); + 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 clearWatchStreamTimer(): void { + if (this.watchStreamTimer !== null) { + clearTimeout(this.watchStreamTimer); + this.watchStreamTimer = null; + } + } +} diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index b0801209c3f..182e0e13871 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -51,19 +51,14 @@ import { WatchTargetChange, WatchTargetChangeState } from './watch_change'; +import { clearTimeout } from 'timers'; +import { OnlineStateTracker } from './online_state_tracker'; 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 @@ -119,17 +114,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( /** @@ -139,8 +124,10 @@ export class RemoteStore { private localStore: LocalStore, /** The client-side proxy for interacting with the backend. */ private datastore: Datastore, - private onlineStateHandler: (onlineState: OnlineState) => void - ) {} + onlineStateHandler: (onlineState: OnlineState) => void + ) { + this.onlineStateTracker = new OnlineStateTracker(onlineStateHandler); + } /** SyncEngine to notify of watch and write events. */ public syncEngine: RemoteSyncer; @@ -153,51 +140,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), @@ -222,10 +164,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. }); } @@ -236,8 +178,8 @@ export class RemoteStore { */ 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); return Promise.resolve(); } @@ -262,9 +204,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(); } @@ -340,18 +282,24 @@ 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 a watch stream is currently necessary (network is enabled + * and we have active watch targets). + */ + private isWatchStreamNecessary(): boolean { + return this.isNetworkEnabled() && !objUtils.isEmpty(this.listenTargets); + } + + /** + * Returns whether the watch stream should be started because it's necessary + * and has not yet been started. */ private shouldStartWatchStream(): boolean { - return ( - this.isNetworkEnabled() && - !this.watchStream.isStarted() && - !objUtils.isEmpty(this.listenTargets) - ); + return this.isWatchStreamNecessary() && !this.watchStream.isStarted(); } private cleanUpWatchStreamState(): void { @@ -379,16 +327,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); } return Promise.resolve(); } @@ -397,8 +345,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 && @@ -821,7 +769,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/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 = { From 46588e629cbe1a4332888e803b072953b81f976e Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Mon, 8 Jan 2018 09:21:24 -0800 Subject: [PATCH 2/8] Add reminder to dispatch timeout onto AsyncQueue. --- packages/firestore/src/remote/online_state_tracker.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/firestore/src/remote/online_state_tracker.ts b/packages/firestore/src/remote/online_state_tracker.ts index d1efe10b94d..9f48c800273 100644 --- a/packages/firestore/src/remote/online_state_tracker.ts +++ b/packages/firestore/src/remote/online_state_tracker.ts @@ -79,6 +79,7 @@ export class OnlineStateTracker { if (this.watchStreamTimer === null) { this.watchStreamTimer = setTimeout(() => { + // TODO(mikelehen): DO NOT SUBMIT: Need to dispatch onto async queue. this.watchStreamTimer = null; assert( this.state === OnlineState.Unknown, From 18c838ae6a7bc977952053f28124a8dd61b77d71 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Fri, 23 Feb 2018 08:40:17 -0800 Subject: [PATCH 3/8] Addressed review Feedback and added a spec test. * Addressed mcg@'s review feedback. * Moved OnlineState timeout to AsyncQueue using new cancelable delayed operation capabilities. * Added a dummy .catch() handler to the CancelablePromises returned by AsyncQueue.enqueueAfterDelay() to avoid UnhandledPromiseRejection log spam due to canceled timers. * Added a SpecTest to verify OnlineState timeout behavior. * Added ability to run timers from spec tests (including assigning string names to TimerId enum values) * Added TimerId.All to match iOS and make it easier to run all timers from spec tests. --- .../firestore/src/core/firestore_client.ts | 1 + .../src/remote/online_state_tracker.ts | 54 +++++++++++-------- .../firestore/src/remote/persistent_stream.ts | 11 ---- packages/firestore/src/remote/remote_store.ts | 24 ++++----- packages/firestore/src/util/async_queue.ts | 37 ++++++++----- .../test/integration/remote/stream.test.ts | 2 +- .../test/unit/specs/offline_spec.test.ts | 35 ++++++++++++ .../firestore/test/unit/specs/spec_builder.ts | 7 +++ .../test/unit/specs/spec_test_runner.ts | 25 +++++++-- .../test/unit/util/async_queue.test.ts | 4 +- 10 files changed, 132 insertions(+), 68 deletions(-) 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/remote/online_state_tracker.ts b/packages/firestore/src/remote/online_state_tracker.ts index 9f48c800273..4b16a821904 100644 --- a/packages/firestore/src/remote/online_state_tracker.ts +++ b/packages/firestore/src/remote/online_state_tracker.ts @@ -1,5 +1,5 @@ /** - * Copyright 2017 Google Inc. + * 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. @@ -17,6 +17,8 @@ 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'; @@ -56,8 +58,7 @@ export class OnlineStateTracker { * revert to OnlineState.Offline without waiting for the stream to actually * fail (MAX_WATCH_STREAM_FAILURES times). */ - // tslint:disable-next-line:no-any setTimeout() type differs on browser / node - private watchStreamTimer: any = null; + private watchStreamTimerPromise: CancelablePromise | null = null; /** * Whether the client should log a warning message if it fails to connect to @@ -66,7 +67,10 @@ export class OnlineStateTracker { */ private shouldWarnClientIsOffline = true; - constructor(private onlineStateHandler: (onlineState: OnlineState) => void) {} + constructor( + private asyncQueue: AsyncQueue, + private onlineStateHandler: (onlineState: OnlineState) => void + ) {} /** * Called by RemoteStore when a watch stream is started. @@ -77,22 +81,26 @@ export class OnlineStateTracker { handleWatchStreamStart(): void { this.setAndBroadcast(OnlineState.Unknown); - if (this.watchStreamTimer === null) { - this.watchStreamTimer = setTimeout(() => { - // TODO(mikelehen): DO NOT SUBMIT: Need to dispatch onto async queue. - this.watchStreamTimer = 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 ` + - `${MAX_WATCH_STREAM_TIMEOUT_MS}ms. Considering client offline.` - ); - this.logClientOfflineWarningIfNecessary(); - this.setAndBroadcast(OnlineState.Offline); - }, MAX_WATCH_STREAM_TIMEOUT_MS); + if (this.watchStreamTimerPromise === null) { + this.watchStreamTimerPromise = this.asyncQueue.enqueueAfterDelay( + TimerId.OnlineStateTimeout, + MAX_WATCH_STREAM_TIMEOUT_MS, + () => { + this.watchStreamTimerPromise = 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 ` + + `${MAX_WATCH_STREAM_TIMEOUT_MS}ms. Considering client offline.` + ); + this.logClientOfflineWarningIfNecessary(); + this.setAndBroadcast(OnlineState.Offline); + return Promise.resolve(); + } + ); } } @@ -150,9 +158,9 @@ export class OnlineStateTracker { } private clearWatchStreamTimer(): void { - if (this.watchStreamTimer !== null) { - clearTimeout(this.watchStreamTimer); - this.watchStreamTimer = null; + if (this.watchStreamTimerPromise !== null) { + this.watchStreamTimerPromise.cancel(); + this.watchStreamTimerPromise = null; } } } diff --git a/packages/firestore/src/remote/persistent_stream.ts b/packages/firestore/src/remote/persistent_stream.ts index 0fd3c6573fc..4c5899b8877 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}` - ); - }); } } diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index b128ecce950..060edbae56b 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -49,8 +49,8 @@ import { WatchTargetChange, WatchTargetChangeState } from './watch_change'; -import { clearTimeout } from 'timers'; import { OnlineStateTracker } from './online_state_tracker'; +import { AsyncQueue } from '../util/async_queue'; const LOG_TAG = 'RemoteStore'; @@ -122,9 +122,13 @@ export class RemoteStore { private localStore: LocalStore, /** The client-side proxy for interacting with the backend. */ private datastore: Datastore, + asyncQueue: AsyncQueue, onlineStateHandler: (onlineState: OnlineState) => void ) { - this.onlineStateTracker = new OnlineStateTracker(onlineStateHandler); + this.onlineStateTracker = new OnlineStateTracker( + asyncQueue, + onlineStateHandler + ); } /** SyncEngine to notify of watch and write events. */ @@ -176,7 +180,7 @@ export class RemoteStore { */ async disableNetwork(): Promise { this.disableNetworkInternal(); - // Set the OnlineState to Offline so get()'s return from cache, etc. + // Set the OnlineState to Offline so get()s return from cache, etc. this.onlineStateTracker.set(OnlineState.Offline); } @@ -282,20 +286,16 @@ export class RemoteStore { this.onlineStateTracker.handleWatchStreamStart(); } - /** - * Returns whether a watch stream is currently necessary (network is enabled - * and we have active watch targets). - */ - private isWatchStreamNecessary(): boolean { - return this.isNetworkEnabled() && !objUtils.isEmpty(this.listenTargets); - } - /** * Returns whether the watch stream should be started because it's necessary * and has not yet been started. */ private shouldStartWatchStream(): boolean { - return this.isWatchStreamNecessary() && !this.watchStream.isStarted(); + return ( + this.isNetworkEnabled() && + !this.watchStream.isStarted() && + !objUtils.isEmpty(this.listenTargets) + ); } private cleanUpWatchStreamState(): void { diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index 453e720c373..b3fd595876a 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -27,12 +27,17 @@ 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', + ListenStreamIdle = 'listen_stream_idle', + ListenStreamConnection = 'listen_stream_connection', + WriteStreamIdle = 'write_stream_idle', + WriteStreamConnection = 'write_stream_connection', + OnlineStateTimeout = 'online_state_timeout' } /** @@ -55,7 +60,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 +231,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 +287,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 +306,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/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..2d643ea065e 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() @@ -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..e5bb3543b7e 100644 --- a/packages/firestore/test/unit/util/async_queue.test.ts +++ b/packages/firestore/test/unit/util/async_queue.test.ts @@ -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]); }); From e4819fef983624ea237e0ea4dfc148e14f608b00 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Mon, 26 Feb 2018 13:39:08 -0800 Subject: [PATCH 4/8] Update changelog. --- packages/firestore/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) 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. From b5d412f1f3b9795818c045f24f474fdfc19b5989 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Wed, 28 Feb 2018 18:11:43 -0800 Subject: [PATCH 5/8] Tweak naming / comments based on CR feedback. --- .../src/remote/online_state_tracker.ts | 50 +++++++++++-------- packages/firestore/src/util/async_queue.ts | 15 +++++- 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/packages/firestore/src/remote/online_state_tracker.ts b/packages/firestore/src/remote/online_state_tracker.ts index 4b16a821904..147f8d49323 100644 --- a/packages/firestore/src/remote/online_state_tracker.ts +++ b/packages/firestore/src/remote/online_state_tracker.ts @@ -23,13 +23,14 @@ import { CancelablePromise } from '../util/promise'; const LOG_TAG = 'OnlineStateTracker'; // To deal with transient failures, we allow multiple stream attempts before -// giving up and transitioning to Offline. +// 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 maximum timeout we'll wait for the stream to either succeed or fail -// MAX_WATCH_STREAM_FAILURES times, else we revert to OnlineState.Offline. -const MAX_WATCH_STREAM_TIMEOUT_MS = 10 * 1000; +// 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, @@ -37,7 +38,7 @@ const MAX_WATCH_STREAM_TIMEOUT_MS = 10 * 1000; * 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 MAX_WATCH_STREAM_TIMEOUT_MS for + * 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.). @@ -54,11 +55,11 @@ export class OnlineStateTracker { private watchStreamFailures = 0; /** - * A timer that elapses after MAX_WATCH_STREAM_TIMEOUT_MS, at which point we - * revert to OnlineState.Offline without waiting for the stream to actually - * fail (MAX_WATCH_STREAM_FAILURES times). + * 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 watchStreamTimerPromise: CancelablePromise | null = null; + private onlineStateTimer: CancelablePromise | null = null; /** * Whether the client should log a warning message if it fails to connect to @@ -75,18 +76,18 @@ export class OnlineStateTracker { /** * Called by RemoteStore when a watch stream is started. * - * It sets the OnlineState to Unknown and starts a MAX_WATCH_STREAM_TIMEOUT_MS - * timer if necessary. + * It sets the OnlineState to Unknown and starts the onlineStateTimer + * if necessary. */ handleWatchStreamStart(): void { this.setAndBroadcast(OnlineState.Unknown); - if (this.watchStreamTimerPromise === null) { - this.watchStreamTimerPromise = this.asyncQueue.enqueueAfterDelay( + if (this.onlineStateTimer === null) { + this.onlineStateTimer = this.asyncQueue.enqueueAfterDelay( TimerId.OnlineStateTimeout, - MAX_WATCH_STREAM_TIMEOUT_MS, + ONLINE_STATE_TIMEOUT_MS, () => { - this.watchStreamTimerPromise = null; + this.onlineStateTimer = null; assert( this.state === OnlineState.Unknown, 'Timer should be canceled if we transitioned to a different state.' @@ -94,10 +95,15 @@ export class OnlineStateTracker { log.debug( LOG_TAG, `Watch stream didn't reach online or offline within ` + - `${MAX_WATCH_STREAM_TIMEOUT_MS}ms. Considering client offline.` + `${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(); } ); @@ -116,7 +122,7 @@ export class OnlineStateTracker { } else { this.watchStreamFailures++; if (this.watchStreamFailures >= MAX_WATCH_STREAM_FAILURES) { - this.clearWatchStreamTimer(); + this.clearOnlineStateTimer(); this.logClientOfflineWarningIfNecessary(); this.setAndBroadcast(OnlineState.Offline); } @@ -131,7 +137,7 @@ export class OnlineStateTracker { * handleWatchStreamStart() and handleWatchStreamFailure(). */ set(newState: OnlineState): void { - this.clearWatchStreamTimer(); + this.clearOnlineStateTimer(); this.watchStreamFailures = 0; if (newState === OnlineState.Online) { @@ -157,10 +163,10 @@ export class OnlineStateTracker { } } - private clearWatchStreamTimer(): void { - if (this.watchStreamTimerPromise !== null) { - this.watchStreamTimerPromise.cancel(); - this.watchStreamTimerPromise = null; + private clearOnlineStateTimer(): void { + if (this.onlineStateTimer !== null) { + this.onlineStateTimer.cancel(); + this.onlineStateTimer = null; } } } diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index b3fd595876a..04e7912c0a5 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -31,12 +31,25 @@ type TimerHandle = any; * The string values are used when encoding these timer IDs in JSON spec tests. */ export enum TimerId { - // All can be used with runDelayedOperationsEarly() to run all timers. + /** 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 "Connection" timer is used to restart a stream once the + * appropriate backoff delay has elapsed. + */ ListenStreamIdle = 'listen_stream_idle', ListenStreamConnection = 'listen_stream_connection', WriteStreamIdle = 'write_stream_idle', WriteStreamConnection = 'write_stream_connection', + + /** + * 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' } From cd7e71ea9196a8eb67a48f9dfd4a2a608afcfdb2 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Fri, 2 Mar 2018 12:06:01 -0800 Subject: [PATCH 6/8] Tweak timer names: Connection => ConnectionBackoff. --- packages/firestore/src/remote/persistent_stream.ts | 4 ++-- packages/firestore/src/util/async_queue.ts | 4 ++-- packages/firestore/test/unit/specs/spec_test_runner.ts | 2 +- packages/firestore/test/unit/util/async_queue.test.ts | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/remote/persistent_stream.ts b/packages/firestore/src/remote/persistent_stream.ts index 4c5899b8877..df573a763b0 100644 --- a/packages/firestore/src/remote/persistent_stream.ts +++ b/packages/firestore/src/remote/persistent_stream.ts @@ -518,7 +518,7 @@ export class PersistentListenStream extends PersistentStream< ) { super( queue, - TimerId.ListenStreamConnection, + TimerId.ListenStreamConnectionBackoff, TimerId.ListenStreamIdle, connection, credentials @@ -626,7 +626,7 @@ export class PersistentWriteStream extends PersistentStream< ) { super( queue, - TimerId.WriteStreamConnection, + TimerId.WriteStreamConnectionBackoff, TimerId.WriteStreamIdle, connection, credentials diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index 04e7912c0a5..47c6969650b 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -41,9 +41,9 @@ export enum TimerId { * appropriate backoff delay has elapsed. */ ListenStreamIdle = 'listen_stream_idle', - ListenStreamConnection = 'listen_stream_connection', + ListenStreamConnectionBackoff = 'listen_stream_connection_backoff', WriteStreamIdle = 'write_stream_idle', - WriteStreamConnection = 'write_stream_connection', + WriteStreamConnectionBackoff = 'write_stream_connection_backoff', /** * A timer used in online_state_tracker.ts to transition from diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 2d643ea065e..b5e5c9f54ec 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -702,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(); } diff --git a/packages/firestore/test/unit/util/async_queue.test.ts b/packages/firestore/test/unit/util/async_queue.test.ts index e5bb3543b7e..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(); From 8925ae6ebb9d18b17051e1686b8c2c65d3d1f286 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Fri, 2 Mar 2018 12:08:38 -0800 Subject: [PATCH 7/8] [AUTOMATED]: Prettier Code Styling --- packages/database/test/order_by.test.ts | 4 +- packages/database/test/query.test.ts | 28 +++-------- packages/database/test/transaction.test.ts | 48 +++++-------------- .../src/platform_node/grpc_connection.ts | 6 +-- .../firestore/src/util/input_validation.ts | 8 ++-- .../test/integration/api/validation.test.ts | 4 +- 6 files changed, 27 insertions(+), 71 deletions(-) diff --git a/packages/database/test/order_by.test.ts b/packages/database/test/order_by.test.ts index 70495f2fd60..b821de62138 100644 --- a/packages/database/test/order_by.test.ts +++ b/packages/database/test/order_by.test.ts @@ -363,9 +363,7 @@ describe('.orderBy tests', function() { expect(addedPrevNames).to.deep.equal(expectedPrevNames); }); - it('Removing default listener removes non-default listener that loads all data', function( - done - ) { + it('Removing default listener removes non-default listener that loads all data', function(done) { const ref = getRandomNode() as Reference; const initial = { key: 'value' }; diff --git a/packages/database/test/query.test.ts b/packages/database/test/query.test.ts index 499748d3edd..bda21cdbb0b 100644 --- a/packages/database/test/query.test.ts +++ b/packages/database/test/query.test.ts @@ -1971,9 +1971,7 @@ describe('Query Tests', function() { expect(val).to.equal(2); }); - it('.startAt() with two arguments works properly (case 1169).', function( - done - ) { + it('.startAt() with two arguments works properly (case 1169).', function(done) { const ref = getRandomNode() as Reference; const data = { Walker: { @@ -2110,9 +2108,7 @@ describe('Query Tests', function() { }); }); - it(".endAt(null, 'f').limitToLast(5) returns the right set of children.", function( - done - ) { + it(".endAt(null, 'f').limitToLast(5) returns the right set of children.", function(done) { const ref = getRandomNode() as Reference; ref.set( { a: 'a', b: 'b', c: 'c', d: 'd', e: 'e', f: 'f', g: 'g', h: 'h' }, @@ -2134,9 +2130,7 @@ describe('Query Tests', function() { ); }); - it('complex update() at query root raises correct value event', function( - done - ) { + it('complex update() at query root raises correct value event', function(done) { const nodePair = getRandomNode(2); const writer = nodePair[0]; const reader = nodePair[1]; @@ -2241,9 +2235,7 @@ describe('Query Tests', function() { }); }); - it('listen for child_added events with limit and different types fires properly', function( - done - ) { + it('listen for child_added events with limit and different types fires properly', function(done) { const nodePair = getRandomNode(2); const writer = nodePair[0]; const reader = nodePair[1]; @@ -2285,9 +2277,7 @@ describe('Query Tests', function() { }); }); - it('listen for child_changed events with limit and different types fires properly', function( - done - ) { + it('listen for child_changed events with limit and different types fires properly', function(done) { const nodePair = getRandomNode(2); const writer = nodePair[0]; const reader = nodePair[1]; @@ -2338,9 +2328,7 @@ describe('Query Tests', function() { }); }); - it('listen for child_remove events with limit and different types fires properly', function( - done - ) { + it('listen for child_remove events with limit and different types fires properly', function(done) { const nodePair = getRandomNode(2); const writer = nodePair[0]; const reader = nodePair[1]; @@ -2442,9 +2430,7 @@ describe('Query Tests', function() { ); }); - it('listen for child_remove events when parent set to scalar', function( - done - ) { + it('listen for child_remove events when parent set to scalar', function(done) { const nodePair = getRandomNode(2); const writer = nodePair[0]; const reader = nodePair[1]; diff --git a/packages/database/test/transaction.test.ts b/packages/database/test/transaction.test.ts index e603105ebc0..d0f44f5accc 100644 --- a/packages/database/test/transaction.test.ts +++ b/packages/database/test/transaction.test.ts @@ -86,9 +86,7 @@ describe('Transaction Tests', function() { }); }); - it('Non-aborted transaction sets committed to true in callback.', function( - done - ) { + it('Non-aborted transaction sets committed to true in callback.', function(done) { const node = getRandomNode() as Reference; node.transaction( @@ -104,9 +102,7 @@ describe('Transaction Tests', function() { ); }); - it('Aborted transaction sets committed to false in callback.', function( - done - ) { + it('Aborted transaction sets committed to false in callback.', function(done) { const node = getRandomNode() as Reference; node.transaction( @@ -236,9 +232,7 @@ describe('Transaction Tests', function() { return ea.promise; }); - it('Second transaction gets run immediately on previous output and only runs once.', function( - done - ) { + it('Second transaction gets run immediately on previous output and only runs once.', function(done) { const nodePair = getRandomNode(2) as Reference[]; let firstRun = false, firstDone = false, @@ -512,9 +506,7 @@ describe('Transaction Tests', function() { ); }); - it('Set should cancel already sent transactions that come back as datastale.', function( - done - ) { + it('Set should cancel already sent transactions that come back as datastale.', function(done) { const nodePair = getRandomNode(2) as Reference[]; let transactionCalls = 0; nodePair[0].set(5, function() { @@ -688,9 +680,7 @@ describe('Transaction Tests', function() { return Promise.all([tx1, tx2]); }); - it('Doing set() in successful transaction callback works. Case 870.', function( - done - ) { + it('Doing set() in successful transaction callback works. Case 870.', function(done) { const node = getRandomNode() as Reference; let transactionCalled = false; let callbackCalled = false; @@ -710,9 +700,7 @@ describe('Transaction Tests', function() { ); }); - it('Doing set() in aborted transaction callback works. Case 870.', function( - done - ) { + it('Doing set() in aborted transaction callback works. Case 870.', function(done) { const nodePair = getRandomNode(2) as Reference[], node1 = nodePair[0], node2 = nodePair[1]; @@ -1028,9 +1016,7 @@ describe('Transaction Tests', function() { ); }); - it('Transaction properly reverts data when you add a deeper listen.', function( - done - ) { + it('Transaction properly reverts data when you add a deeper listen.', function(done) { const refPair = getRandomNode(2) as Reference[], ref1 = refPair[0], ref2 = refPair[1]; @@ -1200,9 +1186,7 @@ describe('Transaction Tests', function() { }); }); - it("transaction() doesn't pick up cached data from previous once().", function( - done - ) { + it("transaction() doesn't pick up cached data from previous once().", function(done) { const refPair = getRandomNode(2) as Reference[]; const me = refPair[0], other = refPair[1]; @@ -1229,9 +1213,7 @@ describe('Transaction Tests', function() { }); }); - it("transaction() doesn't pick up cached data from previous transaction.", function( - done - ) { + it("transaction() doesn't pick up cached data from previous transaction.", function(done) { const refPair = getRandomNode(2) as Reference[]; const me = refPair[0], other = refPair[1]; @@ -1263,9 +1245,7 @@ describe('Transaction Tests', function() { ); }); - it('server values: local timestamp should eventually (but not immediately) match the server with txns', function( - done - ) { + it('server values: local timestamp should eventually (but not immediately) match the server with txns', function(done) { const refPair = getRandomNode(2) as Reference[], writer = refPair[0], reader = refPair[1], @@ -1357,9 +1337,7 @@ describe('Transaction Tests', function() { ); }); - it("transaction() on queried location doesn't run initially on null (firebase-worker-queue depends on this).", function( - done - ) { + it("transaction() on queried location doesn't run initially on null (firebase-worker-queue depends on this).", function(done) { const ref = getRandomNode() as Reference; ref.push({ a: 1, b: 2 }, function() { ref @@ -1437,9 +1415,7 @@ describe('Transaction Tests', function() { ); }); - it('transactions works with merges without the transaction path', function( - done - ) { + it('transactions works with merges without the transaction path', function(done) { const ref = getRandomNode() as Reference; ref.update({ foo: 'bar' }); diff --git a/packages/firestore/src/platform_node/grpc_connection.ts b/packages/firestore/src/platform_node/grpc_connection.ts index f6780ead440..639c8c3b831 100644 --- a/packages/firestore/src/platform_node/grpc_connection.ts +++ b/packages/firestore/src/platform_node/grpc_connection.ts @@ -37,9 +37,9 @@ const LOG_TAG = 'Connection'; // TODO(b/38203344): The SDK_VERSION is set independently from Firebase because // we are doing out-of-band releases. Once we release as part of Firebase, we // should use the Firebase version instead. -const X_GOOG_API_CLIENT_VALUE = `gl-node/${process.versions.node} fire/${ - SDK_VERSION -} grpc/${grpcVersion}`; +const X_GOOG_API_CLIENT_VALUE = `gl-node/${ + process.versions.node +} fire/${SDK_VERSION} grpc/${grpcVersion}`; type DuplexRpc = () => grpc.ClientDuplexStream; type ReadableRpc = (req: Req) => grpc.ClientReadableStream; diff --git a/packages/firestore/src/util/input_validation.ts b/packages/firestore/src/util/input_validation.ts index 6d7701c760c..7ad47b8b078 100644 --- a/packages/firestore/src/util/input_validation.ts +++ b/packages/firestore/src/util/input_validation.ts @@ -191,11 +191,9 @@ export function validateNamedPropertyEquals( const actualDescription = valueDescription(input); throw new FirestoreError( Code.INVALID_ARGUMENT, - `Invalid value ${actualDescription} provided to function ${ - functionName - }() for option "${ - optionName - }". Acceptable values: ${expectedDescription.join(', ')}` + `Invalid value ${actualDescription} provided to function ${functionName}() for option "${optionName}". Acceptable values: ${expectedDescription.join( + ', ' + )}` ); } diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index 8bab7ff59fa..a9820691496 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -170,9 +170,7 @@ apiDescribe('Validation:', persistence => { const collection = db.collection('test-collection'); const doc = collection.doc('test-document'); for (const path of badPaths) { - const reason = `Invalid path (${ - path - }). Paths must not contain // in them.`; + const reason = `Invalid path (${path}). Paths must not contain // in them.`; expect(() => db.collection(path)).to.throw(reason); expect(() => db.doc(path)).to.throw(reason); expect(() => collection.doc(path)).to.throw(reason); From de41073424e163115c4410893ed310621c588677 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Fri, 2 Mar 2018 16:55:35 -0800 Subject: [PATCH 8/8] comment tweak. --- packages/firestore/src/util/async_queue.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index 47c6969650b..29440d72a50 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -37,8 +37,8 @@ export enum TimerId { /** * 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 "Connection" timer is used to restart a stream once the - * appropriate backoff delay has elapsed. + * 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',