Remove ConnectionPool

Author: schmidt-sebastian

common/api-review/storage.api.md

@@ -58,7 +58,7 @@ export class _FirebaseStorageImpl implements FirebaseStorage {
     constructor(
     app: FirebaseApp, _authProvider: Provider<FirebaseAuthInternalName>,
     _appCheckProvider: Provider<AppCheckInternalComponentName>,
-    _pool: ConnectionPool, _url?: string | undefined, _firebaseVersion?: string | undefined);
+    _url?: string | undefined, _firebaseVersion?: string | undefined);
     readonly app: FirebaseApp;
     // (undocumented)
     readonly _appCheckProvider: Provider<AppCheckInternalComponentName>;
@@ -78,23 +78,20 @@ export class _FirebaseStorageImpl implements FirebaseStorage {
     get host(): string;
     set host(host: string);
     // Warning: (ae-forgotten-export) The symbol "RequestInfo" needs to be exported by the entry point index.d.ts
+    // Warning: (ae-forgotten-export) The symbol "Connection" needs to be exported by the entry point index.d.ts
     // Warning: (ae-forgotten-export) The symbol "Request" needs to be exported by the entry point index.d.ts
     //
     // (undocumented)
-    _makeRequest<T>(requestInfo: RequestInfo_2<T>, authToken: string | null, appCheckToken: string | null): Request_2<T>;
+    _makeRequest<T>(requestInfo: RequestInfo_2<T>, requestFactory: () => Connection, authToken: string | null, appCheckToken: string | null): Request_2<T>;
     // (undocumented)
-    makeRequestWithTokens<T>(requestInfo: RequestInfo_2<T>): Promise<Request_2<T>>;
+    makeRequestWithTokens<T>(requestInfo: RequestInfo_2<T>, requestFactory: () => Connection): Promise<Request_2<T>>;
     _makeStorageReference(loc: _Location): _Reference;
     get maxOperationRetryTime(): number;
     set maxOperationRetryTime(time: number);
     get maxUploadRetryTime(): number;
     set maxUploadRetryTime(time: number);
     // (undocumented)
     _overrideAuthToken?: string;
-    // Warning: (ae-forgotten-export) The symbol "ConnectionPool" needs to be exported by the entry point index.d.ts
-    //
-    // (undocumented)
-    readonly _pool: ConnectionPool;
     // (undocumented)
     _protocol: string;
     // (undocumented)

packages/storage/src/implementation/connectionPool.ts

@@ -32,7 +32,6 @@ import { RequestInfo } from './requestinfo';
 import { isJustDef } from './type';
 import { makeQueryString } from './url';
 import { Headers, Connection, ErrorCode } from './connection';
-import { ConnectionPool } from './connectionPool';
 
 export interface Request<T> {
   getPromise(): Promise<T>;
@@ -67,7 +66,6 @@ class NetworkRequest<T> implements Request<T> {
     | null;
   private progressCallback_: ((p1: number, p2: number) => void) | null;
   private timeout_: number;
-  private pool_: ConnectionPool;
   promise_: Promise<T>;
 
   constructor(
@@ -78,12 +76,10 @@ class NetworkRequest<T> implements Request<T> {
     successCodes: number[],
     additionalRetryCodes: number[],
     callback: (p1: Connection, p2: string) => T,
-    errorCallback:
-      | ((p1: Connection, p2: StorageError) => StorageError)
-      | null,
+    errorCallback: ((p1: Connection, p2: StorageError) => StorageError) | null,
     timeout: number,
     progressCallback: ((p1: number, p2: number) => void) | null,
-    pool: ConnectionPool
+    private connectionFactory_: () => Connection
   ) {
     this.url_ = url;
     this.method_ = method;
@@ -95,7 +91,6 @@ class NetworkRequest<T> implements Request<T> {
     this.errorCallback_ = errorCallback;
     this.progressCallback_ = progressCallback;
     this.timeout_ = timeout;
-    this.pool_ = pool;
     this.promise_ = new Promise((resolve, reject) => {
       this.resolve_ = resolve as (value?: T | PromiseLike<T>) => void;
       this.reject_ = reject;
@@ -117,7 +112,7 @@ class NetworkRequest<T> implements Request<T> {
         backoffCallback(false, new RequestEndStatus(false, null, true));
         return;
       }
-      const connection = self.pool_.createConnection();
+      const connection = self.connectionFactory_();
       self.pendingConnection_ = connection;
 
       function progressListener(progressEvent: ProgressEvent): void {
@@ -296,7 +291,7 @@ export function makeRequest<T>(
   appId: string | null,
   authToken: string | null,
   appCheckToken: string | null,
-  pool: ConnectionPool,
+  requestFactory: () => Connection,
   firebaseVersion?: string
 ): Request<T> {
   const queryPart = makeQueryString(requestInfo.urlParams);
@@ -317,6 +312,6 @@ export function makeRequest<T>(
     requestInfo.errorHandler,
     requestInfo.timeout,
     requestInfo.progressCallback,
-    pool
+    requestFactory
   );
 }

packages/storage/src/implementation/request.ts

@@ -27,7 +27,6 @@ import {
   SDK_VERSION
 } from '@firebase/app';
 
-import { ConnectionPool } from '../src/implementation/connectionPool';
 import { FirebaseStorageImpl } from '../src/service';
 import {
   Component,
@@ -56,7 +55,6 @@ function factory(
     app,
     authProvider,
     appCheckProvider,
-    new ConnectionPool(),
     url,
     SDK_VERSION
   );

packages/storage/src/implementation/requestmaker.ts

@@ -22,6 +22,9 @@ import {
 } from '../../implementation/connection';
 import { internalError } from '../../implementation/error';
 
+/** An override for the text-based Connection. Used in tests. */
+let connectionFactoryOverride: (() => Connection) | null = null;
+
 /**
  * Network layer for browsers. We use this instead of goog.net.XhrIo because
  * goog.net.XhrIo is hyuuuuge and doesn't work in React Native on Android.
@@ -148,5 +151,11 @@ export class XhrConnection implements Connection {
 }
 
 export function newConnection(): Connection {
-  return new XhrConnection();
+  return connectionFactoryOverride
+    ? connectionFactoryOverride()
+    : new XhrConnection();
+}
+
+export function injectTestConnection(factory: (() => Connection) | null): void {
+  connectionFactoryOverride = factory;
 }

packages/storage/src/index.ts

@@ -15,9 +15,17 @@
  * limitations under the License.
  */
 import { Connection } from '../implementation/connection';
-import { newConnection as nodeNewConnection } from './node/connection';
+import {
+  newConnection as nodeNewConnection,
+  injectTestConnection as nodeInjectTestConnection
+} from './node/connection';
 
 export function newConnection(): Connection {
   // This file is only used under ts-node.
   return nodeNewConnection();
 }
+
+export function injectTestConnection(factory: (() => Connection) | null): void {
+  // This file is only used under ts-node.
+  nodeInjectTestConnection(factory);
+}

packages/storage/src/platform/browser/connection.ts

@@ -22,6 +22,9 @@ import nodeFetch from 'node-fetch';
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const fetch: typeof window.fetch = nodeFetch as any;
 
+/** An override for the text-based Connection. Used in tests. */
+let connectionFactoryOverride: (() => Connection) | null = null;
+
 /**
  * Network layer that works in Node.
  *
@@ -114,5 +117,11 @@ export class FetchConnection implements Connection {
 }
 
 export function newConnection(): Connection {
-  return new FetchConnection();
+  return connectionFactoryOverride
+    ? connectionFactoryOverride()
+    : new FetchConnection();
+}
+
+export function injectTestConnection(factory: (() => Connection) | null): void {
+  connectionFactoryOverride = factory;
 }

packages/storage/src/platform/connection.ts

@@ -39,6 +39,7 @@ import { ListResult } from './list';
 import { UploadTask } from './task';
 import { invalidRootOperation, noDownloadURL } from './implementation/error';
 import { validateNumber } from './implementation/type';
+import { newConnection } from './platform/connection';
 
 /**
  * Provides methods to interact with a bucket in the Firebase Storage service.
@@ -165,7 +166,7 @@ export function uploadBytes(
     metadata
   );
   return ref.storage
-    .makeRequestWithTokens(requestInfo)
+    .makeRequestWithTokens(requestInfo, newConnection)
     .then(request => request.getPromise())
     .then(finalMetadata => {
       return {
@@ -312,7 +313,9 @@ export async function list(
     op.pageToken,
     op.maxResults
   );
-  return (await ref.storage.makeRequestWithTokens(requestInfo)).getPromise();
+  return (
+    await ref.storage.makeRequestWithTokens(requestInfo, newConnection)
+  ).getPromise();
 }
 
 /**
@@ -329,7 +332,9 @@ export async function getMetadata(ref: Reference): Promise<Metadata> {
     ref._location,
     getMappings()
   );
-  return (await ref.storage.makeRequestWithTokens(requestInfo)).getPromise();
+  return (
+    await ref.storage.makeRequestWithTokens(requestInfo, newConnection)
+  ).getPromise();
 }
 
 /**
@@ -354,7 +359,9 @@ export async function updateMetadata(
     metadata,
     getMappings()
   );
-  return (await ref.storage.makeRequestWithTokens(requestInfo)).getPromise();
+  return (
+    await ref.storage.makeRequestWithTokens(requestInfo, newConnection)
+  ).getPromise();
 }
 
 /**
@@ -370,7 +377,7 @@ export async function getDownloadURL(ref: Reference): Promise<string> {
     ref._location,
     getMappings()
   );
-  return (await ref.storage.makeRequestWithTokens(requestInfo))
+  return (await ref.storage.makeRequestWithTokens(requestInfo, newConnection))
     .getPromise()
     .then(url => {
       if (url === null) {
@@ -389,7 +396,9 @@ export async function getDownloadURL(ref: Reference): Promise<string> {
 export async function deleteObject(ref: Reference): Promise<void> {
   ref._throwIfRoot('deleteObject');
   const requestInfo = requestsDeleteObject(ref.storage, ref._location);
-  return (await ref.storage.makeRequestWithTokens(requestInfo)).getPromise();
+  return (
+    await ref.storage.makeRequestWithTokens(requestInfo, newConnection)
+  ).getPromise();
 }
 
 /**

packages/storage/src/platform/node/connection.ts

@@ -19,7 +19,6 @@ import { Location } from './implementation/location';
 import { FailRequest } from './implementation/failrequest';
 import { Request, makeRequest } from './implementation/request';
 import { RequestInfo } from './implementation/requestinfo';
-import { ConnectionPool } from './implementation/connectionPool';
 import { Reference, _getChild } from './reference';
 import { Provider } from '@firebase/component';
 import { FirebaseAuthInternalName } from '@firebase/auth-interop-types';
@@ -31,7 +30,7 @@ import {
   DEFAULT_HOST,
   DEFAULT_MAX_OPERATION_RETRY_TIME,
   DEFAULT_MAX_UPLOAD_RETRY_TIME
-} from '../src/implementation/constants';
+} from './implementation/constants';
 import {
   invalidArgument,
   appDeleted,
@@ -40,6 +39,7 @@ import {
 import { validateNumber } from './implementation/type';
 import { FirebaseStorage } from './public-types';
 import { createMockUserToken, EmulatorMockTokenOptions } from '@firebase/util';
+import { Connection } from './implementation/connection';
 
 export function isUrl(path?: string): boolean {
   return /^[A-Za-z]+:\/\//.test(path as string);
@@ -182,7 +182,6 @@ export class FirebaseStorageImpl implements FirebaseStorage {
     /**
      * @internal
      */
-    readonly _pool: ConnectionPool,
     readonly _url?: string,
     readonly _firebaseVersion?: string
   ) {
@@ -301,6 +300,7 @@ export class FirebaseStorageImpl implements FirebaseStorage {
    */
   _makeRequest<T>(
     requestInfo: RequestInfo<T>,
+    requestFactory: () => Connection,
     authToken: string | null,
     appCheckToken: string | null
   ): Request<T> {
@@ -310,7 +310,7 @@ export class FirebaseStorageImpl implements FirebaseStorage {
         this._appId,
         authToken,
         appCheckToken,
-        this._pool,
+        requestFactory,
         this._firebaseVersion
       );
       this._requests.add(request);
@@ -326,13 +326,19 @@ export class FirebaseStorageImpl implements FirebaseStorage {
   }
 
   async makeRequestWithTokens<T>(
-    requestInfo: RequestInfo<T>
+    requestInfo: RequestInfo<T>,
+    requestFactory: () => Connection
   ): Promise<Request<T>> {
     const [authToken, appCheckToken] = await Promise.all([
       this._getAuthToken(),
       this._getAppCheckToken()
     ]);
 
-    return this._makeRequest(requestInfo, authToken, appCheckToken);
+    return this._makeRequest(
+      requestInfo,
+      requestFactory,
+      authToken,
+      appCheckToken
+    );
   }
 }