From 39c1e7b640b410709b8285c48c16cda78f29594c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 9 Nov 2021 18:22:53 -0800 Subject: [PATCH] bypass warning by flipping `res.finished` flag --- packages/nextjs/src/utils/withSentry.ts | 67 ++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 3f7a737904e7..53ad6bf54931 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -74,7 +74,57 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = } try { - return await origHandler(req, res); + const handlerResult = await origHandler(req, res); + + // Temporarily mark the response as finished, as a hack to get nextjs to not complain that we're coming back + // from the handler successfully without `res.end()` having completed its work. This is necessary (and we know + // we can do it safely) for a few reasons: + // + // - Normally, `res.end()` is sync and completes before the request handler returns, as part of the handler + // sending data back to the client. As soon as the handler is done, nextjs checks to make sure that the + // response is indeed finished. (`res.end()` signals this by setting `res.finished` to `true`.) If it isn't, + // nextjs complains. ("Warning: API resolved without sending a response for .") + // + // - In order to prevent the lambda running the route handler from shutting down before we can send events to + // Sentry, we monkeypatch `res.end()` so that we can call `flush()`, wait for it to finish, and only then + // allow the response to be marked complete. This turns the normally-sync `res.end()` into an async function, + // which isn't awaited because it's assumed to still be sync. As a result, nextjs runs the aforementioned + // check before the now-async `res.end()` has had a chance to set `res.finished = false`, and therefore thinks + // there's a problem when there's not. + // + // - In order to trick nextjs into not complaining, we can set `res.finished` to `true` before exiting the + // handler. If we do that, though, `res.end()` gets mad because it thinks *it* should be the one to get to + // mark the response complete. We therefore need to flip it back to `false` 1) after nextjs's check but 2) + // before the original `res.end()` is called. + // + // - The second part is easy - we control when the original `res.end()` is called, so we can do the flipping + // right beforehand and `res.end()` will be none the wiser. + // + // - The first part isn't as obvious. How do we know we won't end up with a race condition, such that the + // flipping to `false` might happen before the check, negating the entire purpose of this hack? Fortunately, + // before it's done, our async `res.end()` wrapper has to await a `setImmediate()` callback, guaranteeing its + // run lasts at least until the next event loop. The check, on the other hand, happens synchronously, + // immediately after the request handler (so in the same event loop). So as long as we wait to flip + // `res.finished` back to `false` until after the `setImmediate` callback has run, we know we'll be safely in + // the next event loop when we do so. + // + // And with that, everybody's happy: Nextjs doesn't complain about an unfinished response, `res.end()` doesn’t + // complain about an already-finished response, and we have time to make sure events are flushed to Sentry. + // + // One final note: It might seem like making `res.end()` an awaited async function would run the danger of + // having the lambda close before it's done its thing, meaning we *still* might not get events sent to Sentry. + // Fortunately, even though it's called `res.end()`, and even though it's normally sync, a) it's far from the + // end of the request process, so there's other stuff which needs to happen before the lambda can close in any + // case, and b) that other stuff isn't triggered until `res.end()` emits a `prefinished` event, so even though + // it's not technically awaited, it's still the case that the process can't go on until it's done. + // + // See + // https://github.com/vercel/next.js/blob/e1464ae5a5061ae83ad015018d4afe41f91978b6/packages/next/server/api-utils.ts#L106-L118 + // and + // https://github.com/nodejs/node/blob/d8f1823d5fca5e3c00b19530fb15343fdd3c8bf5/lib/_http_outgoing.js#L833-L911. + res.finished = true; + + return handlerResult; } catch (e) { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced @@ -112,6 +162,16 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = type ResponseEndMethod = AugmentedResponse['end']; type WrappedResponseEndMethod = AugmentedResponse['end']; +/** + * Wrap `res.end()` so that it closes the transaction and flushes events before letting the request finish. + * + * Note: This wraps a sync method with an async method. While in general that's not a great idea in terms of keeping + * things in the right order, in this case it's safe', as explained in detail in the long comment in the main + * `withSentry()` function. + * + * @param origEnd The original `res.end()` method + * @returns The wrapped version + */ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { return async function newEnd(this: AugmentedResponse, ...args: unknown[]) { const transaction = this.__sentryTransaction; @@ -140,6 +200,11 @@ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { logger.log(`Error while flushing events:\n${e}`); } + // If the request didn't error, we will have temporarily marked the response finished to avoid a nextjs warning + // message. (See long note above.) Now we need to flip `finished` back to `false` so that the real `res.end()` + // method doesn't throw `ERR_STREAM_WRITE_AFTER_END` (which it will if presented with an already-finished response). + this.finished = false; + return origEnd.call(this, ...args); }; }