feat: add async error handling to onRequest (#1755)

BREAKING CHANGE: Unhandled errors in async onRequest handlers now immediately return a 500 Internal Server Error instead of hanging until timeout. This may affect clients relying on 504 Gateway Timeout for retry logic.

Author: taeold

spec/helper.ts

@@ -40,7 +40,11 @@ export interface RunHandlerResult {
  * data populated into the response.
  */
 export function runHandler(
-  handler: express.Handler,
+  handler: (
+    req: https.Request,
+    res: express.Response,
+    next?: express.NextFunction
+  ) => void | Promise<void>,
   request: https.Request
 ): Promise<RunHandlerResult> {
   return new Promise((resolve) => {
@@ -119,7 +123,7 @@ export function runHandler(
       }
     }
     const response = new MockResponse();
-    handler(request, response as any, () => undefined);
+    return void handler(request, response as any, () => undefined);
   });
 }
 

spec/v1/providers/httpsAsync.spec.ts

@@ -0,0 +1,49 @@
+import { expect } from "chai";
+import * as sinon from "sinon";
+import * as https from "../../../src/v1/providers/https";
+import * as logger from "../../../src/logger";
+import { MockRequest } from "../../fixtures/mockrequest";
+import { runHandler } from "../../helper";
+
+describe("CloudHttpsBuilder async onRequest", () => {
+  let loggerSpy: sinon.SinonSpy;
+
+  beforeEach(() => {
+    loggerSpy = sinon.spy(logger, "error");
+  });
+
+  afterEach(() => {
+    loggerSpy.restore();
+  });
+
+  it("should catch and log unhandled rejections in async onRequest handlers", async () => {
+    const err = new Error("boom");
+    const fn = https.onRequest(async (_req, _res) => {
+      await Promise.resolve();
+      throw err;
+    });
+
+    const req = new MockRequest({}, {});
+    req.method = "GET";
+
+    const result = await runHandler(fn, req as any);
+
+    expect(loggerSpy.calledWith("Unhandled error", err)).to.be.true;
+    expect(result.status).to.equal(500);
+    expect(result.body).to.equal("Internal Server Error");
+  });
+
+  it("should not log if handler completes successfully", async () => {
+    const fn = https.onRequest(async (_req, res) => {
+      await Promise.resolve();
+      res.send(200);
+    });
+
+    const req = new MockRequest({}, {});
+    req.method = "GET";
+
+    await runHandler(fn, req as any);
+
+    expect(loggerSpy.called).to.be.false;
+  });
+});

spec/v2/providers/httpsAsync.spec.ts

@@ -0,0 +1,49 @@
+import { expect } from "chai";
+import * as sinon from "sinon";
+import * as https from "../../../src/v2/providers/https";
+import * as logger from "../../../src/logger";
+import { MockRequest } from "../../fixtures/mockrequest";
+import { runHandler } from "../../helper";
+
+describe("v2.https.onRequest async", () => {
+  let loggerSpy: sinon.SinonSpy;
+
+  beforeEach(() => {
+    loggerSpy = sinon.spy(logger, "error");
+  });
+
+  afterEach(() => {
+    loggerSpy.restore();
+  });
+
+  it("should catch and log unhandled rejections in async onRequest handlers", async () => {
+    const err = new Error("boom");
+    const fn = https.onRequest(async (_req, _res) => {
+      await Promise.resolve();
+      throw err;
+    });
+
+    const req = new MockRequest({}, {});
+    req.method = "GET";
+
+    const result = await runHandler(fn, req as any);
+
+    expect(loggerSpy.calledWith("Unhandled error", err)).to.be.true;
+    expect(result.status).to.equal(500);
+    expect(result.body).to.equal("Internal Server Error");
+  });
+
+  it("should not log if handler completes successfully", async () => {
+    const fn = https.onRequest(async (_req, res) => {
+      await Promise.resolve();
+      res.send(200);
+    });
+
+    const req = new MockRequest({}, {});
+    req.method = "GET";
+
+    await runHandler(fn, req as any);
+
+    expect(loggerSpy.called).to.be.false;
+  });
+});

src/common/providers/https.ts

@@ -952,3 +952,30 @@ function wrapOnCallHandler<Req = any, Res = any, Stream = unknown>(
     }
   };
 }
+
+/**
+ * Wraps an HTTP handler with a safety net for unhandled errors.
+ *
+ * This wrapper catches both synchronous errors and rejected Promises from `async` handlers.
+ * Without this, an unhandled error in an `async` handler would cause the request to hang
+ * until the platform timeout, as Express (v4) does not await handlers.
+ *
+ * It logs the error and returns a 500 Internal Server Error to the client if the response
+ * headers have not yet been sent.
+ *
+ * @internal
+ */
+export function withErrorHandler(
+  handler: (req: Request, res: express.Response) => void | Promise<void>
+): (req: Request, res: express.Response) => Promise<void> {
+  return async (req: Request, res: express.Response) => {
+    try {
+      await handler(req, res);
+    } catch (err) {
+      logger.error("Unhandled error", err);
+      if (!res.headersSent) {
+        res.status(500).send("Internal Server Error");
+      }
+    }
+  };
+}

src/v1/providers/https.ts

@@ -29,6 +29,7 @@ import {
   HttpsError,
   onCallHandler,
   Request,
+  withErrorHandler,
 } from "../../common/providers/https";
 import { HttpsFunction, optionsToEndpoint, optionsToTrigger, Runnable } from "../cloud-functions";
 import { DeploymentOptions } from "../function-configuration";
@@ -66,7 +67,7 @@ export function _onRequestWithOptions(
 ): HttpsFunction {
   // lets us add __endpoint without altering handler:
   const cloudFunction: any = (req: Request, res: express.Response) => {
-    return wrapTraceContext(withInit(handler))(req, res);
+    return wrapTraceContext(withInit(withErrorHandler(handler)))(req, res);
   };
   cloudFunction.__trigger = {
     ...optionsToTrigger(options),

src/v2/providers/https.ts

@@ -39,6 +39,7 @@ import {
   onCallHandler,
   Request,
   AuthData,
+  withErrorHandler,
 } from "../../common/providers/https";
 import { initV2Endpoint, ManifestEndpoint } from "../../runtime/manifest";
 import { GlobalOptions, SupportedRegion } from "../options";
@@ -319,6 +320,8 @@ export function onRequest(
     opts = optsOrHandler as HttpsOptions;
   }
 
+  handler = withErrorHandler(handler);
+
   if (isDebugFeatureEnabled("enableCors") || "cors" in opts) {
     let origin = opts.cors instanceof Expression ? opts.cors.value() : opts.cors;
     if (isDebugFeatureEnabled("enableCors")) {