From 56fec538e1aa7558dc4a7adea7134394f76251f6 Mon Sep 17 00:00:00 2001
From: Matt Mastracci <matthew@mastracci.com>
Date: Mon, 29 Apr 2024 09:40:02 -0600
Subject: [PATCH] fix(ext/http): ensure signal is created iff requested
 (#23601)

This correctly creates the `AbortSignal` regardless of when we request
it. If the signal is requested after the request has completed, the
signal is created in the aborted state.

Using GC counts, we can see a reduction in object creation:

This PR: 440
deno 1.42.4: 1650
deno 1.43.0+b02ffec: 874
---
 ext/fetch/23_request.js  | 27 +++++++++++++++++++++++----
 tests/unit/serve_test.ts | 29 +++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/ext/fetch/23_request.js b/ext/fetch/23_request.js
index 70e00a874eda8..873d05a2b5a11 100644
--- a/ext/fetch/23_request.js
+++ b/ext/fetch/23_request.js
@@ -9,7 +9,7 @@
 /// <reference path="./lib.deno_fetch.d.ts" />
 /// <reference lib="esnext" />
 
-import { core, primordials } from "ext:core/mod.js";
+import { core, internals, primordials } from "ext:core/mod.js";
 const {
   ArrayPrototypeMap,
   ArrayPrototypeSlice,
@@ -269,10 +269,20 @@ class Request {
   /** @type {AbortSignal} */
   get [_signal]() {
     const signal = this[_signalCache];
-    if (signal !== undefined) {
+    // This signal not been created yet, and the request is still in progress
+    if (signal === undefined) {
+      const signal = newSignal();
+      this[_signalCache] = signal;
       return signal;
     }
-    return (this[_signalCache] = newSignal());
+    // This signal has not been created yet, but the request has already completed
+    if (signal === false) {
+      const signal = newSignal();
+      this[_signalCache] = signal;
+      signal[signalAbort](signalAbortError);
+      return signal;
+    }
+    return signal;
   }
   get [_mimeType]() {
     const values = getDecodeSplitHeader(
@@ -593,11 +603,20 @@ const signalAbortError = new DOMException(
 ObjectFreeze(signalAbortError);
 
 function abortRequest(request) {
-  if (request[_signal]) {
+  if (request[_signalCache] !== undefined) {
     request[_signal][signalAbort](signalAbortError);
+  } else {
+    request[_signalCache] = false;
   }
 }
 
+function getCachedAbortSignal(request) {
+  return request[_signalCache];
+}
+
+// For testing
+internals.getCachedAbortSignal = getCachedAbortSignal;
+
 export {
   abortRequest,
   fromInnerRequest,
diff --git a/tests/unit/serve_test.ts b/tests/unit/serve_test.ts
index 8978c4f7e0182..74628ace18034 100644
--- a/tests/unit/serve_test.ts
+++ b/tests/unit/serve_test.ts
@@ -23,6 +23,7 @@ const {
   addTrailers,
   serveHttpOnListener,
   serveHttpOnConnection,
+  getCachedAbortSignal,
   // @ts-expect-error TypeScript (as of 3.7) does not support indexing namespaces by symbol
 } = Deno[Deno.internal];
 
@@ -2838,6 +2839,34 @@ for (const delay of ["delay", "nodelay"]) {
   }
 }
 
+// Test for the internal implementation detail of cached request signals. Ensure that the request's
+// signal is aborted if we try to access it after the request has been completed.
+Deno.test(
+  { permissions: { net: true } },
+  async function httpServerSignalCancelled() {
+    let stashedRequest;
+    const { finished, abort } = await makeServer((req) => {
+      // The cache signal is `undefined` because it has not been requested
+      assertEquals(getCachedAbortSignal(req), undefined);
+      stashedRequest = req;
+      return new Response("ok");
+    });
+    await (await fetch(`http://localhost:${servePort}`)).text();
+    abort();
+    await finished;
+
+    // `false` is a semaphore for a signal that should be aborted on creation
+    assertEquals(getCachedAbortSignal(stashedRequest!), false);
+    // Requesting the signal causes it to be materialized
+    assert(stashedRequest!.signal.aborted);
+    // The cached signal is now a full `AbortSignal`
+    assertEquals(
+      getCachedAbortSignal(stashedRequest!).constructor,
+      AbortSignal,
+    );
+  },
+);
+
 Deno.test(
   { permissions: { net: true } },
   async function httpServerCancelFetch() {