diff --git a/doc/content/docs/timeout-and-retry.mdx b/doc/content/docs/timeout-and-retry.mdx index 426217e..cfdc59f 100644 --- a/doc/content/docs/timeout-and-retry.mdx +++ b/doc/content/docs/timeout-and-retry.mdx @@ -24,6 +24,23 @@ const res = await $fetch("/api/users", { }); ``` +The timeout composes with your own `signal` — pass both and whichever aborts +first wins, with the caller's abort reason preserved. + +```ts twoslash title="fetch.ts" +import { createFetch } from "@better-fetch/fetch"; + +const $fetch = createFetch({ + baseURL: "http://localhost:3000", +}) +const controller = new AbortController(); +// ---cut--- +const res = await $fetch("/api/users", { + timeout: 10000, + signal: controller.signal, +}); +``` + ## Auto Retry diff --git a/packages/better-fetch/src/fetch.ts b/packages/better-fetch/src/fetch.ts index c894ad8..519e12b 100644 --- a/packages/better-fetch/src/fetch.ts +++ b/packages/better-fetch/src/fetch.ts @@ -6,6 +6,7 @@ import type { BetterFetchOption, BetterFetchResponse } from "./types"; import { getURL } from "./url"; import { detectResponseType, + forwardAbortSignal, getBody, getFetch, getHeaders, @@ -39,107 +40,152 @@ export const betterFetch = async < } = await initializePlugins(url, options); const fetch = getFetch(opts); const controller = new AbortController(); - const signal = opts.signal ?? controller.signal; - const _url = getURL(__url, opts); - const headers = await getHeaders(opts); - const body = getBody(opts, headers); - const method = getMethod(__url, opts); - // one stable object for the whole request so per-request state keyed on - // its identity survives hooks that return a replacement context - const context = { - ...opts, - url: _url, - headers, - body, - method, - signal, - }; - /** - * Run all on request hooks - */ - for (const onRequest of hooks.onRequest) { - if (onRequest) { - const res = await onRequest(context); - if (typeof res === "object" && res !== null) { - Object.assign(context, res); + // the request always listens to our own controller so `timeout` stays armed + // even when the caller passes a `signal`; the caller's signal is forwarded + // onto the controller, and whichever aborts first wins. + const releaseSignal = forwardAbortSignal(controller, opts.signal); + const signal = controller.signal; + // cleared in `finally` too: a rejected fetch (caller abort, network error) + // must not leave the timer holding the event loop until the deadline + let clearTimeout = () => {}; + try { + const _url = getURL(__url, opts); + const headers = await getHeaders(opts); + const body = getBody(opts, headers); + const method = getMethod(__url, opts); + // one stable object for the whole request so per-request state keyed on + // its identity survives hooks that return a replacement context + const context = { + ...opts, + url: _url, + headers, + body, + method, + signal, + }; + /** + * Run all on request hooks + */ + for (const onRequest of hooks.onRequest) { + if (onRequest) { + const res = await onRequest(context); + if (typeof res === "object" && res !== null) { + Object.assign(context, res); + } } } - } - if ( - ("pipeTo" in context && typeof context.pipeTo === "function") || - typeof options?.body?.pipe === "function" - ) { - if (!("duplex" in context)) { - context.duplex = "half"; + if ( + ("pipeTo" in context && typeof context.pipeTo === "function") || + typeof options?.body?.pipe === "function" + ) { + if (!("duplex" in context)) { + context.duplex = "half"; + } } - } - const { clearTimeout } = getTimeout(opts, controller); - let response = await fetch(context.url, context); - clearTimeout(); + clearTimeout = getTimeout(opts, controller).clearTimeout; + let response = await fetch(context.url, context); + clearTimeout(); - const responseContext = { - response, - request: context, - }; + const responseContext = { + response, + request: context, + }; - for (const onResponse of hooks.onResponse) { - if (onResponse) { - const r = await onResponse({ - ...responseContext, - response: options?.hookOptions?.cloneResponse - ? response.clone() - : response, - }); - if (r instanceof Response) { - response = r; - } else if (typeof r === "object" && r !== null) { - response = r.response; + for (const onResponse of hooks.onResponse) { + if (onResponse) { + const r = await onResponse({ + ...responseContext, + response: options?.hookOptions?.cloneResponse + ? response.clone() + : response, + }); + if (r instanceof Response) { + response = r; + } else if (typeof r === "object" && r !== null) { + response = r.response; + } } } - } - /** - * OK Branch - */ - if (response.ok) { - const hasBody = context.method !== "HEAD"; - if (!hasBody) { + /** + * OK Branch + */ + if (response.ok) { + const hasBody = context.method !== "HEAD"; + if (!hasBody) { + return { + data: "" as any, + error: null, + } as any; + } + const responseType = detectResponseType(response); + const successContext = { + data: null as any, + response, + request: context, + }; + if (responseType === "json" || responseType === "text") { + const text = await response.text(); + const parser = context.jsonParser ?? jsonParse; + successContext.data = await parser(text); + } else { + successContext.data = await response[responseType](); + } + + /** + * Parse the data if the output schema is defined + */ + if (context?.output) { + if (context.output && !context.disableValidation) { + successContext.data = await parseStandardSchema( + context.output as StandardSchemaV1, + successContext.data, + ); + } + } + + for (const onSuccess of hooks.onSuccess) { + if (onSuccess) { + await onSuccess({ + ...successContext, + response: options?.hookOptions?.cloneResponse + ? response.clone() + : response, + }); + } + } + + if (options?.throw) { + return successContext.data; + } + return { - data: "" as any, + data: successContext.data, error: null, } as any; } - const responseType = detectResponseType(response); - const successContext = { - data: null as any, + const parser = options?.jsonParser ?? jsonParse; + const responseText = await response.text(); + const isJSONResponse = isJSONParsable(responseText); + const errorObject = isJSONResponse ? await parser(responseText) : null; + /** + * Error Branch + */ + const errorContext = { response, + responseText, request: context, + error: { + ...errorObject, + status: response.status, + statusText: response.statusText, + }, }; - if (responseType === "json" || responseType === "text") { - const text = await response.text(); - const parser = context.jsonParser ?? jsonParse; - successContext.data = await parser(text); - } else { - successContext.data = await response[responseType](); - } - - /** - * Parse the data if the output schema is defined - */ - if (context?.output) { - if (context.output && !context.disableValidation) { - successContext.data = await parseStandardSchema( - context.output as StandardSchemaV1, - successContext.data, - ); - } - } - - for (const onSuccess of hooks.onSuccess) { - if (onSuccess) { - await onSuccess({ - ...successContext, + for (const onError of hooks.onError) { + if (onError) { + await onError({ + ...errorContext, response: options?.hookOptions?.cloneResponse ? response.clone() : response, @@ -147,74 +193,41 @@ export const betterFetch = async < } } - if (options?.throw) { - return successContext.data; - } - - return { - data: successContext.data, - error: null, - } as any; - } - const parser = options?.jsonParser ?? jsonParse; - const responseText = await response.text(); - const isJSONResponse = isJSONParsable(responseText); - const errorObject = isJSONResponse ? await parser(responseText) : null; - /** - * Error Branch - */ - const errorContext = { - response, - responseText, - request: context, - error: { - ...errorObject, - status: response.status, - statusText: response.statusText, - }, - }; - for (const onError of hooks.onError) { - if (onError) { - await onError({ - ...errorContext, - response: options?.hookOptions?.cloneResponse - ? response.clone() - : response, - }); - } - } - - if (options?.retry) { - const retryStrategy = createRetryStrategy(options.retry); - const _retryAttempt = options.retryAttempt ?? 0; - if (await retryStrategy.shouldAttemptRetry(_retryAttempt, response)) { - for (const onRetry of hooks.onRetry) { - if (onRetry) { - await onRetry(responseContext); + if (options?.retry) { + const retryStrategy = createRetryStrategy(options.retry); + const _retryAttempt = options.retryAttempt ?? 0; + if (await retryStrategy.shouldAttemptRetry(_retryAttempt, response)) { + for (const onRetry of hooks.onRetry) { + if (onRetry) { + await onRetry(responseContext); + } } + const delay = retryStrategy.getDelay(_retryAttempt); + await new Promise((resolve) => setTimeout(resolve, delay)); + return await betterFetch(url, { + ...options, + retryAttempt: _retryAttempt + 1, + }); } - const delay = retryStrategy.getDelay(_retryAttempt); - await new Promise((resolve) => setTimeout(resolve, delay)); - return await betterFetch(url, { - ...options, - retryAttempt: _retryAttempt + 1, - }); } - } - if (options?.throw) { - throw new BetterFetchError( - response.status, - response.statusText, - isJSONResponse ? errorObject : responseText, - ); + if (options?.throw) { + throw new BetterFetchError( + response.status, + response.statusText, + isJSONResponse ? errorObject : responseText, + ); + } + return { + data: null, + error: { + ...errorObject, + status: response.status, + statusText: response.statusText, + }, + } as any; + } finally { + clearTimeout(); + releaseSignal(); } - return { - data: null, - error: { - ...errorObject, - status: response.status, - statusText: response.statusText, - }, - } as any; }; diff --git a/packages/better-fetch/src/test/fetch.test.ts b/packages/better-fetch/src/test/fetch.test.ts index c3cfc7f..50747f3 100644 --- a/packages/better-fetch/src/test/fetch.test.ts +++ b/packages/better-fetch/src/test/fetch.test.ts @@ -1,7 +1,12 @@ import { createApp, toNodeListener } from "h3"; import { type Listener, listen } from "listhen"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; -import { BetterFetchError, betterFetch, createFetch } from ".."; +import { + BetterFetchError, + type FetchEsque, + betterFetch, + createFetch, +} from ".."; import { router } from "./test-router"; describe("fetch", () => { @@ -203,6 +208,87 @@ describe("fetch", () => { ).rejects.toThrow(/aborted/); }); + // only ever settles by abort, so the request's deadline is the only thing + // that can end it + const hangingFetch: FetchEsque = (_url, init) => + new Promise((_, reject) => { + const signal = init?.signal; + if (!signal) { + return; + } + if (signal.aborted) { + reject(signal.reason); + return; + } + signal.addEventListener("abort", () => reject(signal.reason), { + once: true, + }); + }); + + it("aborts on timeout when no signal is passed", async () => { + await expect( + betterFetch(getURL("ok"), { + timeout: 50, + customFetchImpl: hangingFetch, + }), + ).rejects.toThrow(/aborted/); + }); + + it("aborts on timeout when a signal is also passed", async () => { + const controller = new AbortController(); + await expect( + betterFetch(getURL("ok"), { + timeout: 50, + signal: controller.signal, + customFetchImpl: hangingFetch, + }), + ).rejects.toThrow(/aborted/); + expect(controller.signal.aborted).toBe(false); + }); + + it("aborts with the caller's reason when the signal fires before the timeout", async () => { + const controller = new AbortController(); + const reason = new Error("cancelled by the caller"); + setTimeout(() => controller.abort(reason), 10); + await expect( + betterFetch(getURL("ok"), { + timeout: 5000, + signal: controller.signal, + customFetchImpl: hangingFetch, + }), + ).rejects.toBe(reason); + }); + + it("clears the timeout when the caller aborts before the deadline", async () => { + const controller = new AbortController(); + const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout"); + setTimeout(() => controller.abort(), 10); + await betterFetch(getURL("ok"), { + timeout: 5000, + signal: controller.signal, + customFetchImpl: hangingFetch, + }).catch(() => {}); + // the armed deadline timer is released, not left to hold the loop + expect(clearTimeoutSpy).toHaveBeenCalled(); + clearTimeoutSpy.mockRestore(); + }); + + it("detaches the forwarded abort listener once the request settles", async () => { + const controller = new AbortController(); + const removeEventListener = vi.spyOn( + controller.signal, + "removeEventListener", + ); + const { data } = await betterFetch(getURL("ok"), { + signal: controller.signal, + }); + expect(data).toBe("ok"); + expect(removeEventListener).toHaveBeenCalledWith( + "abort", + expect.any(Function), + ); + }); + it("resolves a dynamic path param", async () => { const { data } = await betterFetch(getURL("param/:id"), { params: ["2"] }); expect(data).toBe("/param/2"); diff --git a/packages/better-fetch/src/utils.ts b/packages/better-fetch/src/utils.ts index 279bd0f..4681f01 100644 --- a/packages/better-fetch/src/utils.ts +++ b/packages/better-fetch/src/utils.ts @@ -264,8 +264,11 @@ export function getTimeout( clearTimeout: () => void; } { let abortTimeout: ReturnType | undefined; - if (!options?.signal && options?.timeout) { - abortTimeout = setTimeout(() => controller?.abort(), options?.timeout); + // a caller `signal` no longer disarms the timer: the request listens to + // `controller`, and a forwarded caller signal aborts it too, so whichever + // fires first wins instead of the signal silently cancelling the deadline. + if (options?.timeout) { + abortTimeout = setTimeout(() => controller?.abort(), options.timeout); } return { abortTimeout, @@ -277,6 +280,34 @@ export function getTimeout( }; } +/** + * Forwards a caller supplied signal onto the request's own controller, so the + * request can listen to a single signal while still honoring both `signal` and + * `timeout`. The caller's reason is preserved on the way through. + * + * Returns a cleanup that detaches the listener once the request settles, so a + * long-lived signal reused across many requests doesn't accumulate listeners. + */ +export function forwardAbortSignal( + controller: AbortController, + signal?: AbortSignal | null, +): () => void { + if (!signal) { + return () => {}; + } + if (signal.aborted) { + controller.abort(signal.reason); + return () => {}; + } + const onAbort = () => { + controller.abort(signal.reason); + }; + signal.addEventListener("abort", onAbort, { once: true }); + return () => { + signal.removeEventListener("abort", onAbort); + }; +} + export function bodyParser(data: any, responseType: ResponseType) { if (responseType === "json") { return JSON.parse(data);