AbortSignal.timeout: keep the timer armed when the signal loses its observers

src/jsc/bindings/webcore/AbortSignal.cpp

@@ -297,14 +297,6 @@ void AbortSignal::eventListenersDidChange()
         else
             m_timeoutObserverCount.fetch_sub(1, std::memory_order_relaxed);
     }
-
-    // When a timeout signal loses all observers there is nothing left to
-    // notify when the timer fires, so cancel it eagerly.
-    // JSAbortSignalOwner::isReachableFromOpaqueRoots then no longer keeps the
-    // wrapper alive and ~AbortSignal() runs on collection; this just frees the
-    // native timer sooner.
-    if (m_timeout && !aborted() && !hasTimeoutObserver())
-        cancelTimer();
 }
 
 uint32_t AbortSignal::addAbortAlgorithmToSignal(AbortSignal& signal, Ref<AbortAlgorithm>&& algorithm)

test/js/web/abort/abort.test.ts

@@ -1,6 +1,6 @@
 import { describe, expect, test } from "bun:test";
-import { writeFileSync } from "fs";
-import { bunEnv, bunExe, tempDir, tmpdirSync } from "harness";
+import { watch, writeFileSync } from "fs";
+import { bunEnv, bunExe, isWindows, tempDir, tmpdirSync } from "harness";
 import { tmpdir } from "os";
 import { join } from "path";
 
@@ -180,3 +180,163 @@ describe("AbortSignal", () => {
     expect(exitCode).toBe(0);
   });
 });
+
+// https://dom.spec.whatwg.org/#dom-abortsignal-timeout: a timeout signal aborts
+// once its deadline passes for as long as the signal exists. Whether anything
+// was listening in the meantime only matters for GC, never for whether it fires.
+// The native timer used to be cancelled as soon as the signal's listener count
+// dropped to zero, leaving a signal the program still held stuck at
+// aborted === false. Node and the browsers abort it in every case below.
+describe.concurrent("AbortSignal.timeout() still fires after its observers go away", () => {
+  // Resolves after the deadline of a signal armed before this call with a
+  // shorter delay: the fence sits behind it in the timer heap, so by the time
+  // the fence fires that signal's own timer has had its turn. Waiting on a
+  // fence rather than on the signal under test keeps the latter unobserved.
+  function fence(ms: number): Promise<void> {
+    const signal = AbortSignal.timeout(ms);
+    return new Promise(resolve => signal.addEventListener("abort", () => resolve(), { once: true }));
+  }
+
+  function summarize(signal: AbortSignal) {
+    return { aborted: signal.aborted, reason: signal.reason?.name };
+  }
+
+  const churn: [string, (signal: AbortSignal) => void][] = [
+    [
+      "an abort listener was added and removed",
+      signal => {
+        const listener = () => {};
+        signal.addEventListener("abort", listener);
+        signal.removeEventListener("abort", listener);
+      },
+    ],
+    [
+      "onabort was set and cleared",
+      signal => {
+        signal.onabort = () => {};
+        signal.onabort = null;
+      },
+    ],
+    [
+      "its abort listener was removed through the listener's { signal } option",
+      signal => {
+        const controller = new AbortController();
+        signal.addEventListener("abort", () => {}, { signal: controller.signal });
+        controller.abort();
+      },
+    ],
+    [
+      // Not even a removal: adding a listener for any other event type updates
+      // the listener bookkeeping while the abort listener count is still zero.
+      "a listener for an unrelated event type was added",
+      signal => {
+        signal.addEventListener("unrelated", () => {});
+      },
+    ],
+    [
+      // Native consumers observe the signal through a native callback rather
+      // than a JS listener; fs.watch() drops its callback synchronously in close().
+      "the fs.watch() it was passed to was closed",
+      signal => {
+        watch(import.meta.dir, { signal }).close();
+      },
+    ],
+  ];
+
+  test.each(churn)("after %s", async (_, apply) => {
+    const signal = AbortSignal.timeout(1);
+    apply(signal);
+    await fence(20);
+    expect(summarize(signal)).toEqual({ aborted: true, reason: "TimeoutError" });
+  });
+
+  test("consumers attached after the listener churn still see the abort", async () => {
+    const signal = AbortSignal.timeout(1);
+    const listener = () => {};
+    signal.addEventListener("abort", listener);
+    signal.removeEventListener("abort", listener);
+
+    const events: string[] = [];
+    signal.addEventListener("abort", event => events.push(event.type));
+    const dependent = AbortSignal.any([signal]);
+    await fence(20);
+
+    let thrown: DOMException | undefined;
+    try {
+      signal.throwIfAborted();
+    } catch (error) {
+      thrown = error as DOMException;
+    }
+    expect({ events, dependent: summarize(dependent), thrown: thrown?.name }).toEqual({
+      events: ["abort"],
+      dependent: { aborted: true, reason: "TimeoutError" },
+      thrown: "TimeoutError",
+    });
+    expect(thrown).toBe(signal.reason);
+  });
+
+  // Same native-callback release as fs.watch() above, but asynchronous: the
+  // subprocess lets go of the signal when the child exits.
+  test("after the Bun.spawn() child it was passed to has exited", async () => {
+    // The child has to be gone before the deadline. A shell exits in ~1ms
+    // (a debug build of bun takes 100ms+ just to start), so 500ms leaves
+    // plenty of room for a loaded CI machine.
+    const signal = AbortSignal.timeout(500);
+    const deadline = fence(600);
+    await using proc = Bun.spawn({
+      cmd: isWindows ? [process.env.comspec || "cmd.exe", "/c", "exit", "0"] : ["/bin/sh", "-c", "exit 0"],
+      signal,
+      stdout: "ignore",
+      stderr: "ignore",
+    });
+    expect({ exitCode: await proc.exited, ...summarize(signal) }).toEqual({
+      exitCode: 0,
+      aborted: false,
+      reason: undefined,
+    });
+
+    await deadline;
+    expect(summarize(signal)).toEqual({ aborted: true, reason: "TimeoutError" });
+  });
+
+  // A Request keeps its signal as a native ref and wraps it again on demand, so
+  // the signal's JS wrapper can be collected while the timeout is still
+  // observable through request.signal. The timer has to survive the wrapper as
+  // well; only the signal itself going away (or aborting) may stop it.
+  // Subprocess so heapStats() only sees this scenario's signals.
+  test("after its wrapper was collected while a Request still held the signal", async () => {
+    const src = `
+      const { heapStats } = require("bun:jsc");
+      const wrappers = () => heapStats().objectTypeCounts.AbortSignal ?? 0;
+      const N = 32;
+      // A full GC of the debug heap takes ~100ms under ASAN; the deadline only
+      // has to come after it.
+      const deadline = 1000;
+      const started = performance.now();
+      const requests = [];
+      for (let i = 0; i < N; i++) {
+        requests.push(new Request("http://localhost/", { signal: AbortSignal.timeout(deadline) }));
+      }
+      const fence = AbortSignal.timeout(deadline + 100);
+      const withWrappers = wrappers();
+      // Fresh stack first, so nothing conservatively scanned still points at a wrapper.
+      await new Promise(resolve => setImmediate(resolve));
+      Bun.gc(true);
+      const collected = withWrappers - wrappers();
+      const collectedBeforeDeadline = performance.now() - started < deadline;
+      await new Promise(resolve => fence.addEventListener("abort", resolve, { once: true }));
+      // request.signal wraps the native signal again.
+      const timedOut = requests.filter(r => r.signal.aborted && r.signal.reason.name === "TimeoutError").length;
+      console.log(JSON.stringify({ N, collected, collectedBeforeDeadline, timedOut }));
+    `;
+    await using proc = Bun.spawn({ cmd: [bunExe(), "-e", src], env: bunEnv, stderr: "pipe" });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(stderr).toBe("");
+    const { N, collected, collectedBeforeDeadline, timedOut } = JSON.parse(stdout);
+    // The wrappers have to be gone before the timers fire for the run to mean
+    // anything; allow a straggler or two in case something still pins one.
+    expect(collected).toBeGreaterThanOrEqual(N - 4);
+    expect({ collectedBeforeDeadline, timedOut }).toEqual({ collectedBeforeDeadline: true, timedOut: N });
+    expect(exitCode).toBe(0);
+  });
+});