mirror of
https://github.com/moltbot/moltbot.git
synced 2026-04-28 17:21:52 +00:00
fix: use relayAbort helper for addEventListener to preserve AbortError reason
This commit is contained in:
committed by
Peter Steinberger
parent
5ac8d1d2bb
commit
7ec60d6449
@@ -1,4 +1,5 @@
|
|||||||
import type { AnyAgentTool } from "./pi-tools.types.js";
|
import type { AnyAgentTool } from "./pi-tools.types.js";
|
||||||
|
import { bindAbortRelay } from "../utils/fetch-timeout.js";
|
||||||
|
|
||||||
function throwAbortError(): never {
|
function throwAbortError(): never {
|
||||||
const err = new Error("Aborted");
|
const err = new Error("Aborted");
|
||||||
@@ -36,7 +37,7 @@ function combineAbortSignals(a?: AbortSignal, b?: AbortSignal): AbortSignal | un
|
|||||||
}
|
}
|
||||||
|
|
||||||
const controller = new AbortController();
|
const controller = new AbortController();
|
||||||
const onAbort = controller.abort.bind(controller);
|
const onAbort = bindAbortRelay(controller);
|
||||||
a?.addEventListener("abort", onAbort, { once: true });
|
a?.addEventListener("abort", onAbort, { once: true });
|
||||||
b?.addEventListener("abort", onAbort, { once: true });
|
b?.addEventListener("abort", onAbort, { once: true });
|
||||||
return controller.signal;
|
return controller.signal;
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import { describe, expect, it } from "vitest";
|
import { describe, expect, it } from "vitest";
|
||||||
|
import { bindAbortRelay } from "../utils/fetch-timeout.js";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Regression test for #7174: Memory leak from closure-wrapped controller.abort().
|
* Regression test for #7174: Memory leak from closure-wrapped controller.abort().
|
||||||
@@ -7,12 +8,13 @@ import { describe, expect, it } from "vitest";
|
|||||||
* surrounding lexical scope (controller, timer, locals). In long-running
|
* surrounding lexical scope (controller, timer, locals). In long-running
|
||||||
* processes these closures accumulate and prevent GC.
|
* processes these closures accumulate and prevent GC.
|
||||||
*
|
*
|
||||||
* The fix is `controller.abort.bind(controller)` which creates a minimal
|
* The fix uses two patterns:
|
||||||
* bound function with no scope capture.
|
* - setTimeout: `controller.abort.bind(controller)` (safe, no args passed)
|
||||||
*
|
* - addEventListener: `bindAbortRelay(controller)` which returns a bound
|
||||||
* This test verifies the behavioral equivalence of .bind() for both the
|
* function that ignores the Event argument, preserving the default
|
||||||
* setTimeout and addEventListener use-cases.
|
* AbortError reason.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
describe("abort pattern: .bind() vs arrow closure (#7174)", () => {
|
describe("abort pattern: .bind() vs arrow closure (#7174)", () => {
|
||||||
it("controller.abort.bind(controller) aborts the signal", () => {
|
it("controller.abort.bind(controller) aborts the signal", () => {
|
||||||
const controller = new AbortController();
|
const controller = new AbortController();
|
||||||
@@ -31,42 +33,64 @@ describe("abort pattern: .bind() vs arrow closure (#7174)", () => {
|
|||||||
clearTimeout(timer);
|
clearTimeout(timer);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("bound abort works as addEventListener callback and can be removed", () => {
|
it("bindAbortRelay() preserves default AbortError reason when used as event listener", () => {
|
||||||
const parent = new AbortController();
|
const parent = new AbortController();
|
||||||
const child = new AbortController();
|
const child = new AbortController();
|
||||||
const onAbort = child.abort.bind(child);
|
const onAbort = bindAbortRelay(child);
|
||||||
|
|
||||||
parent.signal.addEventListener("abort", onAbort, { once: true });
|
parent.signal.addEventListener("abort", onAbort, { once: true });
|
||||||
expect(child.signal.aborted).toBe(false);
|
|
||||||
|
|
||||||
parent.abort();
|
parent.abort();
|
||||||
|
|
||||||
expect(child.signal.aborted).toBe(true);
|
expect(child.signal.aborted).toBe(true);
|
||||||
|
// The reason must be the default AbortError, not the Event object
|
||||||
|
expect(child.signal.reason).toBeInstanceOf(DOMException);
|
||||||
|
expect(child.signal.reason.name).toBe("AbortError");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("removeEventListener works with saved .bind() reference", () => {
|
it("raw .abort.bind() leaks Event as reason — bindAbortRelay() does not", () => {
|
||||||
|
// Demonstrates the bug: .abort.bind() passes the Event as abort reason
|
||||||
|
const parentA = new AbortController();
|
||||||
|
const childA = new AbortController();
|
||||||
|
parentA.signal.addEventListener("abort", childA.abort.bind(childA), { once: true });
|
||||||
|
parentA.abort();
|
||||||
|
// childA.signal.reason is the Event, NOT an AbortError
|
||||||
|
expect(childA.signal.reason).not.toBeInstanceOf(DOMException);
|
||||||
|
|
||||||
|
// The fix: bindAbortRelay() ignores the Event argument
|
||||||
|
const parentB = new AbortController();
|
||||||
|
const childB = new AbortController();
|
||||||
|
parentB.signal.addEventListener("abort", bindAbortRelay(childB), { once: true });
|
||||||
|
parentB.abort();
|
||||||
|
// childB.signal.reason IS the default AbortError
|
||||||
|
expect(childB.signal.reason).toBeInstanceOf(DOMException);
|
||||||
|
expect(childB.signal.reason.name).toBe("AbortError");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("removeEventListener works with saved bindAbortRelay() reference", () => {
|
||||||
const parent = new AbortController();
|
const parent = new AbortController();
|
||||||
const child = new AbortController();
|
const child = new AbortController();
|
||||||
const onAbort = child.abort.bind(child);
|
const onAbort = bindAbortRelay(child);
|
||||||
|
|
||||||
parent.signal.addEventListener("abort", onAbort);
|
parent.signal.addEventListener("abort", onAbort);
|
||||||
// Remove before parent aborts — child should NOT be aborted
|
|
||||||
parent.signal.removeEventListener("abort", onAbort);
|
parent.signal.removeEventListener("abort", onAbort);
|
||||||
parent.abort();
|
parent.abort();
|
||||||
expect(child.signal.aborted).toBe(false);
|
expect(child.signal.aborted).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("bound abort forwards abort through combined signals", () => {
|
it("bindAbortRelay() forwards abort through combined signals", () => {
|
||||||
// Simulates the combineAbortSignals pattern from pi-tools.abort.ts
|
// Simulates the combineAbortSignals pattern from pi-tools.abort.ts
|
||||||
const signalA = new AbortController();
|
const signalA = new AbortController();
|
||||||
const signalB = new AbortController();
|
const signalB = new AbortController();
|
||||||
const combined = new AbortController();
|
const combined = new AbortController();
|
||||||
|
|
||||||
const onAbort = combined.abort.bind(combined);
|
const onAbort = bindAbortRelay(combined);
|
||||||
signalA.signal.addEventListener("abort", onAbort, { once: true });
|
signalA.signal.addEventListener("abort", onAbort, { once: true });
|
||||||
signalB.signal.addEventListener("abort", onAbort, { once: true });
|
signalB.signal.addEventListener("abort", onAbort, { once: true });
|
||||||
|
|
||||||
expect(combined.signal.aborted).toBe(false);
|
expect(combined.signal.aborted).toBe(false);
|
||||||
signalA.abort();
|
signalA.abort();
|
||||||
expect(combined.signal.aborted).toBe(true);
|
expect(combined.signal.aborted).toBe(true);
|
||||||
|
expect(combined.signal.reason).toBeInstanceOf(DOMException);
|
||||||
|
expect(combined.signal.reason.name).toBe("AbortError");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1,3 +1,5 @@
|
|||||||
|
import { bindAbortRelay } from "../utils/fetch-timeout.js";
|
||||||
|
|
||||||
type FetchWithPreconnect = typeof fetch & {
|
type FetchWithPreconnect = typeof fetch & {
|
||||||
preconnect: (url: string, init?: { credentials?: RequestCredentials }) => void;
|
preconnect: (url: string, init?: { credentials?: RequestCredentials }) => void;
|
||||||
};
|
};
|
||||||
@@ -42,7 +44,7 @@ export function wrapFetchWithAbortSignal(fetchImpl: typeof fetch): typeof fetch
|
|||||||
return fetchImpl(input, patchedInit);
|
return fetchImpl(input, patchedInit);
|
||||||
}
|
}
|
||||||
const controller = new AbortController();
|
const controller = new AbortController();
|
||||||
const onAbort = controller.abort.bind(controller);
|
const onAbort = bindAbortRelay(controller);
|
||||||
if (signal.aborted) {
|
if (signal.aborted) {
|
||||||
controller.abort();
|
controller.abort();
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import type { Dispatcher } from "undici";
|
import type { Dispatcher } from "undici";
|
||||||
import { logWarn } from "../../logger.js";
|
import { logWarn } from "../../logger.js";
|
||||||
|
import { bindAbortRelay } from "../../utils/fetch-timeout.js";
|
||||||
import {
|
import {
|
||||||
closeDispatcher,
|
closeDispatcher,
|
||||||
createPinnedDispatcher,
|
createPinnedDispatcher,
|
||||||
@@ -51,7 +52,7 @@ function buildAbortSignal(params: { timeoutMs?: number; signal?: AbortSignal }):
|
|||||||
|
|
||||||
const controller = new AbortController();
|
const controller = new AbortController();
|
||||||
const timeoutId = setTimeout(controller.abort.bind(controller), timeoutMs);
|
const timeoutId = setTimeout(controller.abort.bind(controller), timeoutMs);
|
||||||
const onAbort = controller.abort.bind(controller);
|
const onAbort = bindAbortRelay(controller);
|
||||||
if (signal) {
|
if (signal) {
|
||||||
if (signal.aborted) {
|
if (signal.aborted) {
|
||||||
controller.abort();
|
controller.abort();
|
||||||
|
|||||||
@@ -1,3 +1,16 @@
|
|||||||
|
/**
|
||||||
|
* Relay abort without forwarding the Event argument as the abort reason.
|
||||||
|
* Using .bind() avoids closure scope capture (memory leak prevention).
|
||||||
|
*/
|
||||||
|
function relayAbort(this: AbortController) {
|
||||||
|
this.abort();
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Returns a bound abort relay for use as an event listener. */
|
||||||
|
export function bindAbortRelay(controller: AbortController): () => void {
|
||||||
|
return relayAbort.bind(controller);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Fetch wrapper that adds timeout support via AbortController.
|
* Fetch wrapper that adds timeout support via AbortController.
|
||||||
*
|
*
|
||||||
|
|||||||
Reference in New Issue
Block a user