From 64cb87005a98f530701e98128673861bdb73121d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 6 Jan 2023 14:09:07 +0000 Subject: [PATCH 1/2] fix(nextjs): Don't wrap `res.json` and `res.send` --- .../src/config/wrappers/withSentryAPI.ts | 24 ++----------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/withSentryAPI.ts b/packages/nextjs/src/config/wrappers/withSentryAPI.ts index e047be166ed4..605284b1cd86 100644 --- a/packages/nextjs/src/config/wrappers/withSentryAPI.ts +++ b/packages/nextjs/src/config/wrappers/withSentryAPI.ts @@ -133,28 +133,8 @@ export function withSentry(origHandler: NextApiHandler, parameterizedRoute?: str if (platformSupportsStreaming()) { autoEndTransactionOnResponseEnd(transaction, res); } else { - // If we're not on a platform that supports streaming, we're blocking all response-ending methods until the - // queue is flushed. - - const origResSend = res.send; - res.send = async function (this: unknown, ...args: unknown[]) { - if (transaction) { - await finishTransaction(transaction, res); - await flushQueue(); - } - - origResSend.apply(this, args); - }; - - const origResJson = res.json; - res.json = async function (this: unknown, ...args: unknown[]) { - if (transaction) { - await finishTransaction(transaction, res); - await flushQueue(); - } - - origResJson.apply(this, args); - }; + // If we're not on a platform that supports streaming, we're blocking res.end() until the queue is flushed. + // res.json() and res.send() will implicitly call res.end(), so it is enough to wrap res.end(). // eslint-disable-next-line @typescript-eslint/unbound-method const origResEnd = res.end; From 9fb99d54e89660b8d465c063fa0f53b054c9ec48 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 9 Jan 2023 09:47:25 +0000 Subject: [PATCH 2/2] Add test --- packages/nextjs/src/config/wrappers/types.ts | 5 +++++ packages/nextjs/src/config/wrappers/withSentryAPI.ts | 4 ++-- .../integration/pages/api/doubleEndMethodOnVercel.ts | 11 +++++++++++ .../test/server/doubleEndMethodOnVercel.js | 10 ++++++++++ 4 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 packages/nextjs/test/integration/pages/api/doubleEndMethodOnVercel.ts create mode 100644 packages/nextjs/test/integration/test/server/doubleEndMethodOnVercel.js diff --git a/packages/nextjs/src/config/wrappers/types.ts b/packages/nextjs/src/config/wrappers/types.ts index 7ad042cc2474..a411c4ea62cf 100644 --- a/packages/nextjs/src/config/wrappers/types.ts +++ b/packages/nextjs/src/config/wrappers/types.ts @@ -25,6 +25,11 @@ import type { NextApiRequest, NextApiResponse } from 'next'; export type NextApiHandler = { (req: NextApiRequest, res: NextApiResponse): void | Promise | unknown | Promise; __sentry_route__?: string; + + /** + * A property we set in our integration tests to simulate running an API route on platforms that don't support streaming. + */ + __sentry_test_doesnt_support_streaming__?: true; }; export type WrappedNextApiHandler = { diff --git a/packages/nextjs/src/config/wrappers/withSentryAPI.ts b/packages/nextjs/src/config/wrappers/withSentryAPI.ts index 605284b1cd86..561939685aa0 100644 --- a/packages/nextjs/src/config/wrappers/withSentryAPI.ts +++ b/packages/nextjs/src/config/wrappers/withSentryAPI.ts @@ -130,7 +130,7 @@ export function withSentry(origHandler: NextApiHandler, parameterizedRoute?: str ); currentScope.setSpan(transaction); - if (platformSupportsStreaming()) { + if (platformSupportsStreaming() && !origHandler.__sentry_test_doesnt_support_streaming__) { autoEndTransactionOnResponseEnd(transaction, res); } else { // If we're not on a platform that supports streaming, we're blocking res.end() until the queue is flushed. @@ -203,7 +203,7 @@ export function withSentry(origHandler: NextApiHandler, parameterizedRoute?: str // moment they detect an error, so it's important to get this done before rethrowing the error. Apps not // deployed serverlessly will run into this cleanup code again in `res.end(), but the transaction will already // be finished and the queue will already be empty, so effectively it'll just no-op.) - if (platformSupportsStreaming()) { + if (platformSupportsStreaming() && !origHandler.__sentry_test_doesnt_support_streaming__) { void finishTransaction(transaction, res); } else { await finishTransaction(transaction, res); diff --git a/packages/nextjs/test/integration/pages/api/doubleEndMethodOnVercel.ts b/packages/nextjs/test/integration/pages/api/doubleEndMethodOnVercel.ts new file mode 100644 index 000000000000..f32fcf55fafd --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/doubleEndMethodOnVercel.ts @@ -0,0 +1,11 @@ +import { NextApiRequest, NextApiResponse } from 'next'; + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + // This handler calls .end twice. We test this to verify that this still doesn't throw because we're wrapping `.end`. + res.status(200).json({ success: true }); + res.end(); +}; + +handler.__sentry_test_doesnt_support_streaming__ = true; + +export default handler; diff --git a/packages/nextjs/test/integration/test/server/doubleEndMethodOnVercel.js b/packages/nextjs/test/integration/test/server/doubleEndMethodOnVercel.js new file mode 100644 index 000000000000..fa2b0e7cbeb4 --- /dev/null +++ b/packages/nextjs/test/integration/test/server/doubleEndMethodOnVercel.js @@ -0,0 +1,10 @@ +const assert = require('assert'); +const { getAsync } = require('../utils/server'); + +// This test asserts that our wrapping of `res.end` doesn't break API routes on Vercel if people call `res.json` or +// `res.send` multiple times in one request handler. +// https://github.com/getsentry/sentry-javascript/issues/6670 +module.exports = async ({ url: urlBase }) => { + const response = await getAsync(`${urlBase}/api/doubleEndMethodOnVercel`); + assert.equal(response, '{"success":true}'); +};