From 7672e98d4d219dbdcaa3952cfc64af506d7d0bc3 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 7 Feb 2022 16:28:37 -0800 Subject: [PATCH 1/2] log message rather than changing finished status on response --- packages/nextjs/src/utils/withSentry.ts | 65 +++++-------------------- 1 file changed, 11 insertions(+), 54 deletions(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 6c0f1d26a764..3e6512b2c40f 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -79,53 +79,12 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = try { 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; + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.warn( + '\n[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which we\'re working to rectify.\n', + ); + } return handlerResult; } catch (e) { @@ -183,8 +142,11 @@ type WrappedResponseEndMethod = AugmentedNextApiResponse['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. + * things in the right order, in this case it's safe, because the native `.end()` actually *is* async, and its run + * actually *is* awaited, just manually so (which reflects the fact that the core of the request/response code in Node + * by far predates the introduction of `async`/`await`). When `.end()` is done, it emits the `prefinish` event, and + * only once that fires does request processing continue. See + * https://github.com/nodejs/node/commit/7c9b607048f13741173d397795bac37707405ba7. * * @param origEnd The original `res.end()` method * @returns The wrapped version @@ -193,11 +155,6 @@ function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { return async function newEnd(this: AugmentedNextApiResponse, ...args: unknown[]) { await finishSentryProcessing(this); - // 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); }; } From fdb798044c9db63d57c9a03712e75b32519eccfb Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 8 Feb 2022 10:12:17 -0800 Subject: [PATCH 2/2] remove newlines --- packages/nextjs/src/utils/withSentry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 3e6512b2c40f..94d14db4dd41 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -82,7 +82,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = if (process.env.NODE_ENV === 'development') { // eslint-disable-next-line no-console console.warn( - '\n[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which we\'re working to rectify.\n', + '[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which we\'re working to rectify.', ); }