Tweak naming / comments based on CR feedback.

Author: Michael Lehenbauer

packages/firestore/src/remote/online_state_tracker.ts

@@ -23,21 +23,22 @@ 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,
  * 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
+ * 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<void> | null = null;
+  private onlineStateTimer: CancelablePromise<void> | null = null;
 
   /**
    * Whether the client should log a warning message if it fails to connect to
@@ -75,29 +76,34 @@ 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.'
           );
           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;
     }
   }
 }

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'
 }