apiv2: timeout (#3006)

* add timeout option for apiv2 * fix up all tests for project management * add test for timeout * update nock, remove nock types * extend typing to prevent signal and timeout in the same options object * add test and support for providing a signal * update changelog
firebase · Jan 8, 2021 · 706ce19 · 706ce19
1 parent 8082b68
commit 706ce19
Show file tree

Hide file tree

Showing 8 changed files with 277 additions and 220 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,2 +1,3 @@
-- Add warning when a developer is using yarn@2 PnP (#2198)
+- Add warning when a developer is using yarn@2 PnP (#2198).
 - Fixes incorrect URLs reported inside emulated HTTPS functions (#1862).
+- Updates underlying timeout handler when proxying through the Hosting emulator.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -155,7 +155,6 @@
     "@types/marked": "^0.6.5",
     "@types/mocha": "^8.2.0",
     "@types/multer": "^1.4.3",
-    "@types/nock": "^9.3.0",
     "@types/node": "^10.17.50",
     "@types/node-fetch": "^2.5.7",
     "@types/plist": "^3.0.1",
@@ -190,7 +189,7 @@
     "firebase-functions": "^3.11.0",
     "google-discovery-to-swagger": "^2.1.0",
     "mocha": "^8.2.1",
-    "nock": "^9.3.3",
+    "nock": "^13.0.5",
     "nyc": "^15.1.0",
     "openapi-merge": "^1.0.23",
     "prettier": "^1.19.0",

diff --git a/src/apiv2.ts b/src/apiv2.ts
@@ -2,6 +2,7 @@ import { AbortSignal } from "abort-controller";
 import { Readable } from "stream";
 import { URLSearchParams } from "url";
 import * as ProxyAgent from "proxy-agent";
+import AbortController from "abort-controller";
 import fetch, { HeadersInit, Response, RequestInit, Headers } from "node-fetch";
 
 import { FirebaseError } from "./error";
@@ -12,15 +13,28 @@ const CLI_VERSION = require("../package.json").version;
 
 export type HttpMethod = "GET" | "PUT" | "POST" | "DELETE" | "PATCH";
 
-interface RequestOptions<T> extends VerbOptions<T> {
+interface BaseRequestOptions<T> extends VerbOptions<T> {
   method: HttpMethod;
   path: string;
   body?: T | string | NodeJS.ReadableStream;
   responseType?: "json" | "stream";
   redirect?: "error" | "follow" | "manual";
+}
+
+interface RequestOptionsWithSignal<T> extends BaseRequestOptions<T> {
+  // Signal is used to cancel a request. Cannot be used with `timeout`.
   signal?: AbortSignal;
+  timeout?: never;
+}
+
+interface RequestOptionsWithTimeout<T> extends BaseRequestOptions<T> {
+  signal?: never;
+  // Timeout, in ms. 0 is no timeout. Cannot be used with `signal`.
+  timeout?: number;
 }
 
+type RequestOptions<T> = RequestOptionsWithSignal<T> | RequestOptionsWithTimeout<T>;
+
 interface VerbOptions<T> {
   method?: HttpMethod;
   headers?: HeadersInit;
@@ -38,10 +52,13 @@ interface ClientHandlingOptions {
 
 export type ClientRequestOptions<T> = RequestOptions<T> & ClientVerbOptions<T>;
 
-interface InternalClientRequestOptions<T> extends ClientRequestOptions<T> {
+interface BaseInternalClientRequestOptions<T> {
   headers?: Headers;
 }
 
+type InternalClientRequestOptions<T> = BaseInternalClientRequestOptions<T> &
+  ClientRequestOptions<T>;
+
 export type ClientVerbOptions<T> = VerbOptions<T> & ClientHandlingOptions;
 
 export type ClientResponse<T> = {
@@ -272,7 +289,6 @@ export class Client {
       headers: options.headers,
       method: options.method,
       redirect: options.redirect,
-      signal: options.signal,
     };
 
     if (this.opts.proxy) {
@@ -283,6 +299,19 @@ export class Client {
       fetchOptions.agent = new ProxyAgent(envProxy);
     }
 
+    if (options.signal) {
+      fetchOptions.signal = options.signal;
+    }
+
+    let reqTimeout: NodeJS.Timeout | undefined;
+    if (options.timeout) {
+      const controller = new AbortController();
+      reqTimeout = setTimeout(() => {
+        controller.abort();
+      }, options.timeout);
+      fetchOptions.signal = controller.signal;
+    }
+
     if (typeof options.body === "string" || isStream(options.body)) {
       fetchOptions.body = options.body;
     } else if (options.body !== undefined) {
@@ -295,7 +324,16 @@ export class Client {
     try {
       res = await fetch(fetchURL, fetchOptions);
     } catch (err) {
+      const isAbortError = err.name.includes("AbortError");
+      if (isAbortError) {
+        throw new FirebaseError(`Timeout reached making request to ${fetchURL}`, { original: err });
+      }
       throw new FirebaseError(`Failed to make request to ${fetchURL}`, { original: err });
+    } finally {
+      // If we succeed or failed, clear the timeout.
+      if (reqTimeout) {
+        clearTimeout(reqTimeout);
+      }
     }
 
     let body: ResT;

diff --git a/src/hosting/proxy.ts b/src/hosting/proxy.ts
@@ -1,14 +1,13 @@
 import { capitalize, includes } from "lodash";
+import { FetchError, Headers } from "node-fetch";
 import { IncomingMessage, ServerResponse } from "http";
 import { PassThrough } from "stream";
 import { Request, RequestHandler, Response } from "express";
 import { URL } from "url";
-import AbortController from "abort-controller";
 
 import { Client, HttpMethod } from "../apiv2";
 import { FirebaseError } from "../error";
 import * as logger from "../logger";
-import { FetchError, Headers } from "node-fetch";
 
 const REQUIRED_VARY_VALUES = ["Accept-Encoding", "Authorization", "Cookie"];
 
@@ -53,8 +52,6 @@ export function proxyRequestHandler(url: string, rewriteIdentifier: string): Req
     // req.url is just the full path (e.g. /foo?key=value; no origin).
     const u = new URL(url + req.url);
     const c = new Client({ urlPrefix: u.origin, auth: false });
-    const controller = new AbortController();
-    const timer: NodeJS.Timeout = setTimeout(() => controller.abort(), 60000);
 
     let passThrough: PassThrough | undefined;
     if (req.method && !["GET", "HEAD"].includes(req.method)) {
@@ -95,10 +92,9 @@ export function proxyRequestHandler(url: string, rewriteIdentifier: string): Req
         responseType: "stream",
         redirect: "manual",
         body: passThrough,
-        signal: controller.signal,
+        timeout: 60000,
       });
     } catch (err) {
-      clearTimeout(timer);
       const isAbortError =
         err instanceof FirebaseError && err.original?.name.includes("AbortError");
       const isTimeoutError =
@@ -117,7 +113,6 @@ export function proxyRequestHandler(url: string, rewriteIdentifier: string): Req
       return res.end(`An internal error occurred while proxying for ${rewriteIdentifier}\n`);
     }
 
-    clearTimeout(timer);
     if (proxyRes.status === 404) {
       // x-cascade is not a string[].
       const cascade = proxyRes.response.headers.get("x-cascade");

diff --git a/src/management/projects.ts b/src/management/projects.ts
@@ -1,13 +1,14 @@
-import * as api from "../api";
+import * as _ from "lodash";
 import * as clc from "cli-color";
 import * as ora from "ora";
-import * as _ from "lodash";
 
-import * as logger from "../logger";
+import { Client } from "../apiv2";
 import { FirebaseError } from "../error";
 import { pollOperation } from "../operation-poller";
-import { Question } from "inquirer";
 import { promptOnce } from "../prompt";
+import { Question } from "inquirer";
+import * as api from "../api";
+import * as logger from "../logger";
 import * as utils from "../utils";
 
 const TIMEOUT_MILLIS = 30000;
@@ -68,6 +69,12 @@ export const PROJECTS_CREATE_QUESTIONS: Question[] = [
   },
 ];
 
+const firebaseAPIClient = new Client({
+  urlPrefix: api.firebaseApiOrigin,
+  auth: true,
+  apiVersion: "v1beta1",
+});
+
 export async function createFirebaseProjectAndLog(
   projectId: string,
   options: { displayName?: string; parentResource?: ProjectParentResource }
@@ -321,20 +328,24 @@ async function getProjectPage<T>(
     pageToken?: string;
   }
 ): Promise<ProjectPage<T>> {
-  const { responseKey, pageToken, pageSize } = options;
-  const pageTokenQueryString = pageToken ? `&pageToken=${pageToken}` : "";
-  const apiResponse = await api.request(
-    "GET",
-    `${apiResource}?pageSize=${pageSize}${pageTokenQueryString}`,
-    {
-      auth: true,
-      origin: api.firebaseApiOrigin,
-      timeout: TIMEOUT_MILLIS,
-    }
-  );
+  const queryParams: { [key: string]: string } = {
+    pageSize: `${options.pageSize}`,
+  };
+  if (options.pageToken) {
+    queryParams.pageToken = options.pageToken;
+  }
+  const res = await firebaseAPIClient.request<void, { [key: string]: T[] | string | undefined }>({
+    method: "GET",
+    path: apiResource,
+    queryParams,
+    timeout: TIMEOUT_MILLIS,
+    skipLog: { resBody: true },
+  });
+  const projects = res.body[options.responseKey];
+  const token = res.body.nextPageToken;
   return {
-    projects: apiResponse.body[responseKey] || [],
-    nextPageToken: apiResponse.body.nextPageToken,
+    projects: Array.isArray(projects) ? projects : [],
+    nextPageToken: typeof token === "string" ? token : undefined,
   };
 }
 
@@ -349,7 +360,7 @@ export async function getFirebaseProjectPage(
   let projectPage;
 
   try {
-    projectPage = await getProjectPage<FirebaseProjectMetadata>("/v1beta1/projects", {
+    projectPage = await getProjectPage<FirebaseProjectMetadata>("/projects", {
       responseKey: "results",
       pageSize,
       pageToken,
@@ -374,7 +385,7 @@ export async function getAvailableCloudProjectPage(
   pageToken?: string
 ): Promise<ProjectPage<CloudProjectInfo>> {
   try {
-    return await getProjectPage<CloudProjectInfo>("/v1beta1/availableProjects", {
+    return await getProjectPage<CloudProjectInfo>("/availableProjects", {
       responseKey: "projectInfo",
       pageSize,
       pageToken,
@@ -414,12 +425,12 @@ export async function listFirebaseProjects(pageSize?: number): Promise<FirebaseP
  */
 export async function getFirebaseProject(projectId: string): Promise<FirebaseProjectMetadata> {
   try {
-    const response = await api.request("GET", `/v1beta1/projects/${projectId}`, {
-      auth: true,
-      origin: api.firebaseApiOrigin,
+    const res = await firebaseAPIClient.request<void, FirebaseProjectMetadata>({
+      method: "GET",
+      path: `/projects/${projectId}`,
       timeout: TIMEOUT_MILLIS,
     });
-    return response.body;
+    return res.body;
   } catch (err) {
     logger.debug(err.message);
     throw new FirebaseError(