From 679b7bb8fafc1464d5eb1c48989d477157be2330 Mon Sep 17 00:00:00 2001
From: Matt Mastracci <matthew@mastracci.com>
Date: Sat, 18 Nov 2023 13:16:53 -0700
Subject: [PATCH] fix(ext/http): fix crash in dropped Deno.serve requests
 (#21252)

Fixes #21250

We were attempting to recycle dropped resource responses too early.
---
 cli/tests/unit/serve_test.ts | 130 +++++++++++++++++++----------------
 ext/http/http_next.rs        |   3 +-
 2 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/cli/tests/unit/serve_test.ts b/cli/tests/unit/serve_test.ts
index 69bcbc7403..20ad7316dc 100644
--- a/cli/tests/unit/serve_test.ts
+++ b/cli/tests/unit/serve_test.ts
@@ -2727,73 +2727,81 @@ Deno.test(
   },
 );
 
-for (const url of ["text", "file", "stream"]) {
-  // Ensure that we don't panic when the incoming TCP request was dropped
-  // https://github.com/denoland/deno/issues/20315 and that we correctly
-  // close/cancel the response
-  Deno.test({
-    permissions: { read: true, write: true, net: true },
-    name: `httpServerTcpCancellation_${url}`,
-    fn: async function () {
-      const ac = new AbortController();
-      const streamCancelled = url == "stream" ? deferred() : undefined;
-      const listeningPromise = deferred();
-      const waitForAbort = deferred();
-      const waitForRequest = deferred();
-      const server = Deno.serve({
-        port: servePort,
-        signal: ac.signal,
-        onListen: onListen(listeningPromise),
-        handler: async (req: Request) => {
-          let respBody = null;
-          if (req.url.includes("/text")) {
-            respBody = "text";
-          } else if (req.url.includes("/file")) {
-            respBody = (await makeTempFile(1024)).readable;
-          } else if (req.url.includes("/stream")) {
-            respBody = new ReadableStream({
-              start(controller) {
-                controller.enqueue(new Uint8Array([1]));
-              },
-              cancel(reason) {
-                streamCancelled!.resolve(reason);
-              },
-            });
-          } else {
-            fail();
-          }
-          waitForRequest.resolve();
-          await waitForAbort;
-          // Allocate the request body
-          req.body;
-          return new Response(respBody);
-        },
-      });
+for (const delay of ["delay", "nodelay"]) {
+  for (const url of ["text", "file", "stream"]) {
+    // Ensure that we don't panic when the incoming TCP request was dropped
+    // https://github.com/denoland/deno/issues/20315 and that we correctly
+    // close/cancel the response
+    Deno.test({
+      permissions: { read: true, write: true, net: true },
+      name: `httpServerTcpCancellation_${url}_${delay}`,
+      fn: async function () {
+        const ac = new AbortController();
+        const streamCancelled = url == "stream" ? deferred() : undefined;
+        const listeningPromise = deferred();
+        const waitForAbort = deferred();
+        const waitForRequest = deferred();
+        const server = Deno.serve({
+          port: servePort,
+          signal: ac.signal,
+          onListen: onListen(listeningPromise),
+          handler: async (req: Request) => {
+            let respBody = null;
+            if (req.url.includes("/text")) {
+              respBody = "text";
+            } else if (req.url.includes("/file")) {
+              respBody = (await makeTempFile(1024)).readable;
+            } else if (req.url.includes("/stream")) {
+              respBody = new ReadableStream({
+                start(controller) {
+                  controller.enqueue(new Uint8Array([1]));
+                },
+                cancel(reason) {
+                  streamCancelled!.resolve(reason);
+                },
+              });
+            } else {
+              fail();
+            }
+            waitForRequest.resolve();
+            await waitForAbort;
 
-      await listeningPromise;
+            if (delay == "delay") {
+              await new Promise((r) => setTimeout(r, 1000));
+            }
+            // Allocate the request body
+            req.body;
+            return new Response(respBody);
+          },
+        });
 
-      // Create a POST request and drop it once the server has received it
-      const conn = await Deno.connect({ port: servePort });
-      const writer = conn.writable.getWriter();
-      await writer.write(new TextEncoder().encode(`POST /${url} HTTP/1.0\n\n`));
-      await waitForRequest;
-      await writer.close();
+        await listeningPromise;
 
-      waitForAbort.resolve();
+        // Create a POST request and drop it once the server has received it
+        const conn = await Deno.connect({ port: servePort });
+        const writer = conn.writable.getWriter();
+        await writer.write(
+          new TextEncoder().encode(`POST /${url} HTTP/1.0\n\n`),
+        );
+        await waitForRequest;
+        await writer.close();
 
-      // Wait for cancellation before we shut the server down
-      if (streamCancelled !== undefined) {
-        await streamCancelled;
-      }
+        waitForAbort.resolve();
 
-      // Since the handler has a chance of creating resources or running async
-      // ops, we need to use a graceful shutdown here to ensure they have fully
-      // drained.
-      await server.shutdown();
+        // Wait for cancellation before we shut the server down
+        if (streamCancelled !== undefined) {
+          await streamCancelled;
+        }
 
-      await server.finished;
-    },
-  });
+        // Since the handler has a chance of creating resources or running async
+        // ops, we need to use a graceful shutdown here to ensure they have fully
+        // drained.
+        await server.shutdown();
+
+        await server.finished;
+      },
+    });
+  }
 }
 
 Deno.test(
diff --git a/ext/http/http_next.rs b/ext/http/http_next.rs
index 98fbd1f8bc..9504a6fa48 100644
--- a/ext/http/http_next.rs
+++ b/ext/http/http_next.rs
@@ -716,6 +716,8 @@ pub async fn op_http_set_response_body_resource(
     }
   };
 
+  *http.needs_close_after_finish() = true;
+
   set_response(
     http.clone(),
     resource.size_hint().1.map(|s| s as usize),
@@ -726,7 +728,6 @@ pub async fn op_http_set_response_body_resource(
     },
   );
 
-  *http.needs_close_after_finish() = true;
   http.response_body_finished().await;
   Ok(())
 }