diff --git a/spec/helper.ts b/spec/helper.ts index 9df32156b..c3f0f38ff 100644 --- a/spec/helper.ts +++ b/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, request: https.Request ): Promise { 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); }); } diff --git a/spec/v1/providers/httpsAsync.spec.ts b/spec/v1/providers/httpsAsync.spec.ts new file mode 100644 index 000000000..84fcf3a59 --- /dev/null +++ b/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; + }); +}); diff --git a/spec/v2/providers/httpsAsync.spec.ts b/spec/v2/providers/httpsAsync.spec.ts new file mode 100644 index 000000000..78ff12bf1 --- /dev/null +++ b/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; + }); +}); diff --git a/src/common/providers/https.ts b/src/common/providers/https.ts index 46b25e948..e6d69cc5b 100644 --- a/src/common/providers/https.ts +++ b/src/common/providers/https.ts @@ -952,3 +952,30 @@ function wrapOnCallHandler( } }; } + +/** + * 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 +): (req: Request, res: express.Response) => Promise { + 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"); + } + } + }; +} diff --git a/src/v1/providers/https.ts b/src/v1/providers/https.ts index 8d079bfa1..da3f2fdb0 100644 --- a/src/v1/providers/https.ts +++ b/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), diff --git a/src/v2/providers/https.ts b/src/v2/providers/https.ts index eaf56cb94..0a510e62f 100644 --- a/src/v2/providers/https.ts +++ b/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")) {