Forced get to wait until db is online (#6340)

Fixes #6036

Author: maneesht

.changeset/metal-spies-shave.md

@@ -0,0 +1,6 @@
+---
+"@firebase/database": patch
+"@firebase/util": patch
+---
+
+Forced `get()` to wait until db is online to resolve.

common/api-review/util.api.md

@@ -346,6 +346,11 @@ export function ordinal(i: number): string;
 // @public (undocumented)
 export type PartialObserver<T> = Partial<Observer<T>>;
 
+// Warning: (ae-internal-missing-underscore) The name "promiseWithTimeout" should be prefixed with an underscore because the declaration is marked as @internal
+//
+// @internal
+export function promiseWithTimeout<T>(promise: Promise<T>, timeInMS?: number): Promise<T>;
+
 // Warning: (ae-missing-release-tag) "querystring" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public

packages/database-compat/test/query.test.ts

@@ -15,6 +15,7 @@
  * limitations under the License.
  */
 
+import { promiseWithTimeout } from '@firebase/util';
 import { expect, use } from 'chai';
 import chaiAsPromised from 'chai-as-promised';
 import * as _ from 'lodash';
@@ -3226,7 +3227,8 @@ describe('Query Tests', () => {
     const node = getRandomNode() as Reference;
     node.database.goOffline();
     try {
-      await expect(node.get()).to.eventually.be.rejected;
+      const getPromise = promiseWithTimeout(node.get());
+      await expect(getPromise).to.eventually.be.rejected;
     } finally {
       node.database.goOnline();
     }
@@ -3392,8 +3394,8 @@ describe('Query Tests', () => {
     expect(snapshot.val()).to.deep.equal({ data: '1' });
     reader.database.goOffline();
     try {
-      await expect(reader.child('foo/notCached').get()).to.eventually.be
-        .rejected;
+      await expect(promiseWithTimeout(reader.child('foo/notCached').get())).to
+        .eventually.be.rejected;
     } finally {
       reader.database.goOnline();
     }

packages/database/src/core/PersistentConnection.ts

@@ -44,7 +44,6 @@ import { QueryContext } from './view/EventRegistration';
 
 const RECONNECT_MIN_DELAY = 1000;
 const RECONNECT_MAX_DELAY_DEFAULT = 60 * 5 * 1000; // 5 minutes in milliseconds (Case: 1858)
-const GET_CONNECT_TIMEOUT = 3 * 1000;
 const RECONNECT_MAX_DELAY_FOR_ADMINS = 30 * 1000; // 30 seconds for admin clients (likely to be a backend server)
 const RECONNECT_DELAY_MULTIPLIER = 1.3;
 const RECONNECT_DELAY_RESET_TIMEOUT = 30000; // Reset delay back to MIN_DELAY after being connected for 30sec.
@@ -216,22 +215,6 @@ export class PersistentConnection extends ServerActions {
     this.outstandingGetCount_++;
     const index = this.outstandingGets_.length - 1;
 
-    if (!this.connected_) {
-      setTimeout(() => {
-        const get = this.outstandingGets_[index];
-        if (get === undefined || outstandingGet !== get) {
-          return;
-        }
-        delete this.outstandingGets_[index];
-        this.outstandingGetCount_--;
-        if (this.outstandingGetCount_ === 0) {
-          this.outstandingGets_ = [];
-        }
-        this.log_('get ' + index + ' timed out on connection');
-        deferred.reject(new Error('Client is offline.'));
-      }, GET_CONNECT_TIMEOUT);
-    }
-
     if (this.connected_) {
       this.sendGet_(index);
     }

packages/database/test/exp/integration.test.ts

@@ -17,7 +17,8 @@
 
 import { initializeApp, deleteApp } from '@firebase/app';
 import { Deferred } from '@firebase/util';
-import { expect } from 'chai';
+import { expect, use } from 'chai';
+import chaiAsPromised from 'chai-as-promised';
 
 import {
   get,
@@ -44,6 +45,8 @@ import {
   waitFor
 } from '../helpers/util';
 
+use(chaiAsPromised);
+
 export function createTestApp() {
   return initializeApp({ databaseURL: DATABASE_URL });
 }
@@ -257,26 +260,38 @@ describe('Database@exp Tests', () => {
     expect(events[0]).to.equal('a');
   });
 
-  it('Can goOffline/goOnline', async () => {
-    const db = getDatabase(defaultApp);
-    goOffline(db);
-    try {
-      await get(ref(db, 'foo/bar'));
-      expect.fail('Should have failed since we are offline');
-    } catch (e) {
-      expect(e.message).to.equal('Error: Client is offline.');
-    }
-    goOnline(db);
-    await get(ref(db, 'foo/bar'));
-  });
-
   it('Can delete app', async () => {
     const db = getDatabase(defaultApp);
     await deleteApp(defaultApp);
     expect(() => ref(db)).to.throw('Cannot call ref on a deleted database.');
     defaultApp = undefined;
   });
 
+  it('blocks get requests until the database is online', async () => {
+    const db = getDatabase(defaultApp);
+    const r = ref(db, 'foo3');
+    const initial = {
+      test: 1
+    };
+    await set(r, initial);
+    goOffline(db);
+    const pendingGet = get(r);
+    let resolvedData: any = null;
+    pendingGet.then(
+      data => {
+        resolvedData = data;
+      },
+      () => {
+        resolvedData = new Error('rejected');
+      }
+    );
+    await waitFor(2000);
+    expect(resolvedData).to.equal(null);
+    goOnline(db);
+    await waitFor(2000);
+    expect(resolvedData.val()).to.deep.equal(initial);
+  });
+
   it('Can listen to transaction changes', async () => {
     // Repro for https://github.com/firebase/firebase-js-sdk/issues/5195
     let latestValue = 0;

packages/util/index.node.ts

@@ -31,6 +31,7 @@ export * from './src/errors';
 export * from './src/json';
 export * from './src/jwt';
 export * from './src/obj';
+export * from './src/promise';
 export * from './src/query';
 export * from './src/sha1';
 export * from './src/subscribe';

packages/util/index.ts

@@ -26,6 +26,7 @@ export * from './src/errors';
 export * from './src/json';
 export * from './src/jwt';
 export * from './src/obj';
+export * from './src/promise';
 export * from './src/query';
 export * from './src/sha1';
 export * from './src/subscribe';

packages/util/src/promise.ts

@@ -0,0 +1,32 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ *
+ * 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 { Deferred } from './deferred';
+
+/**
+ * Rejects if the given promise doesn't resolve in timeInMS milliseconds.
+ * @internal
+ */
+export function promiseWithTimeout<T>(
+  promise: Promise<T>,
+  timeInMS = 2000
+): Promise<T> {
+  const deferredPromise = new Deferred<T>();
+  setTimeout(() => deferredPromise.reject('timeout!'), timeInMS);
+  promise.then(deferredPromise.resolve, deferredPromise.reject);
+  return deferredPromise.promise;
+}