Add network timeout cancellation to the core network code (#3771)

* Add network timeout cancellation to core network code

* Formatting

* Fix broken test on CI

* Formatting

Author: sam-gc

packages-exp/auth-exp/src/api/index.test.ts

@@ -17,7 +17,9 @@
 
 import { assert, expect, use } from 'chai';
 import * as chaiAsPromised from 'chai-as-promised';
+import * as sinon from 'sinon';
 import { useFakeTimers } from 'sinon';
+import * as sinonChai from 'sinon-chai';
 
 import { FirebaseError } from '@firebase/util';
 
@@ -36,6 +38,7 @@ import {
 } from './';
 import { ServerError } from './errors';
 
+use(sinonChai);
 use(chaiAsPromised);
 
 describe('api/_performApiRequest', () => {
@@ -227,6 +230,22 @@ describe('api/_performApiRequest', () => {
       clock.restore();
     });
 
+    it('should clear the network timeout on success', async () => {
+      const clock = useFakeTimers();
+      sinon.spy(clock, 'clearTimeout');
+      mockFetch.setUp();
+      mockEndpoint(Endpoint.SIGN_UP, {});
+      const promise = _performApiRequest(
+        auth,
+        HttpMethod.POST,
+        Endpoint.SIGN_UP,
+        request
+      );
+      await promise;
+      expect(clock.clearTimeout).to.have.been.called;
+      clock.restore();
+    });
+
     it('should handle network failure', async () => {
       mockFetch.setUpWithOverride(() => {
         return new Promise<never>((_, reject) =>

packages-exp/auth-exp/src/api/index.ts

@@ -120,11 +120,16 @@ export async function _performFetchWithErrorHandling<V>(
   (auth as Auth)._canInitEmulator = false;
   const errorMap = { ...SERVER_ERROR_MAP, ...customErrorMap };
   try {
+    const networkTimeout = new NetworkTimeout<Response>(auth.name);
     const response: Response = await Promise.race<Promise<Response>>([
       fetchFn(),
-      makeNetworkTimeout(auth.name)
+      networkTimeout.promise
     ]);
 
+    // If we've reached this point, the fetch succeeded and the networkTimeout
+    // didn't throw; clear the network timeout delay so that Node won't hang
+    networkTimeout.clearNetworkTimeout();
+
     const json = await response.json();
     if ('needConfirmation' in json) {
       throw makeTaggedError(auth, AuthErrorCode.NEED_CONFIRMATION, json);
@@ -201,16 +206,26 @@ export function _getFinalTarget(
   return _emulatorUrl(auth.config, base);
 }
 
-function makeNetworkTimeout<T>(appName: string): Promise<T> {
-  return new Promise((_, reject) =>
-    setTimeout(() => {
+class NetworkTimeout<T> {
+  // Node timers and browser timers are fundamentally incompatible, but we
+  // don't care about the value here
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  private timer: any | null = null;
+  readonly promise = new Promise<T>((_, reject) => {
+    this.timer = setTimeout(() => {
       return reject(
         AUTH_ERROR_FACTORY.create(AuthErrorCode.TIMEOUT, {
-          appName
+          appName: this.appName
         })
       );
-    }, DEFAULT_API_TIMEOUT_MS.get())
-  );
+    }, DEFAULT_API_TIMEOUT_MS.get());
+  });
+
+  clearNetworkTimeout(): void {
+    clearTimeout(this.timer);
+  }
+
+  constructor(private readonly appName: string) {}
 }
 
 interface PotentialResponse extends IdTokenResponse {

packages-exp/auth-exp/src/core/strategies/email.test.ts

@@ -24,7 +24,7 @@ import { Operation, ProviderId } from '@firebase/auth-types-exp';
 import { FirebaseError, isNode } from '@firebase/util';
 
 import { mockEndpoint } from '../../../test/helpers/api/helper';
-import { testAuth, testUser, TestAuth } from '../../../test/helpers/mock_auth';
+import { testAuth, TestAuth, testUser } from '../../../test/helpers/mock_auth';
 import * as mockFetch from '../../../test/helpers/mock_fetch';
 import { Endpoint } from '../../api';
 import { ServerError } from '../../api/errors';
@@ -72,10 +72,12 @@ describe('core/strategies/fetchSignInMethodsForEmail', () => {
       });
       const response = await fetchSignInMethodsForEmail(auth, email);
       expect(response).to.eql(expectedSignInMethods);
-      expect(mock.calls[0].request).to.eql({
-        identifier: email,
-        continueUri: 'http://localhost:8089/context.html'
-      });
+      const request = mock.calls[0].request as Record<string, string>;
+      expect(request['identifier']).to.eq(email);
+      // We can't rely on a fixed port number
+      expect(request['continueUri']).to.match(
+        /http:\/\/localhost:[0-9]+\/context\.html/
+      );
     });
   }