fix(nextjs): Revert #4139 - remove manipulation of res.finished value (#4516)

In #4139, a change was introduced in order to suppress a false positive warning thrown by nextjs, by setting `res.finished` to `true` just long enough for nextjs to check it and decide no warning was needed. In simple cases, it worked just fine, but in some set-ups the "just long enough for nextjs to check it and calm down" turned out to also be "just long enough for other things to check it and get mad."

This backs out that change, as it seems it's doing more harm than good. In order to address the original problem (documented in [1] and [2]), a new warning is added, explaining that the false positive is just that. Not as elegant as suppressing the message altogether, but it should tide us over until such time as we're able to try again with a different approach.

Fixes #4151.

[1] #3852
[2] #4007

Author: lobsterkatie

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 <url>.")
-        //
-        // - 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(
+            '[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which we\'re working to rectify.',
+          );
+        }
 
         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);
   };
 }