From 054ffa71b1c45a949afdc3f87256f4ede1a1a4ba Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 28 May 2025 11:59:43 +0200 Subject: [PATCH 1/6] do not overwrite own `AbortSignal` if an external `AbortSignal` is passed in --- .changeset/quiet-balloons-wave.md | 5 +++++ src/link/http/BaseHttpLink.ts | 31 +++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 .changeset/quiet-balloons-wave.md diff --git a/.changeset/quiet-balloons-wave.md b/.changeset/quiet-balloons-wave.md new file mode 100644 index 00000000000..56274dbff07 --- /dev/null +++ b/.changeset/quiet-balloons-wave.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix a situation where a passed-in `AbortSignal` would override internal unsubscription cancellation behaviour. diff --git a/src/link/http/BaseHttpLink.ts b/src/link/http/BaseHttpLink.ts index 94ca367dc1b..f75ccae13ec 100644 --- a/src/link/http/BaseHttpLink.ts +++ b/src/link/http/BaseHttpLink.ts @@ -25,6 +25,7 @@ import { import { selectURI } from "./selectURI.js"; const backupFetch = maybe(() => fetch); +function noop() {} export class BaseHttpLink extends ApolloLink { constructor(linkOptions: HttpLink.Options = {}) { @@ -89,11 +90,29 @@ export class BaseHttpLink extends ApolloLink { ); } - let controller: AbortController | undefined; - if (!options.signal && typeof AbortController !== "undefined") { - controller = new AbortController(); - options.signal = controller.signal; + let controller: AbortController | undefined = new AbortController(); + let cleanupController = () => { + controller = undefined; + }; + if (options.signal) { + // in an ideal world we could use `AbortSignal.any` here, but + // React Native uses https://github.com/mysticatea/abort-controller as + // a polyfill for `AbortController`, and it does not support `AbortSignal.any`. + const abort = controller.abort.bind(controller); + options.signal.addEventListener("abort", abort, { once: true }); + cleanupController = () => { + controller = undefined; + // on cleanup, we need to stop listening to `options.signal` to avoid memory leaks + options.signal.removeEventListener("abort", abort); + cleanupController = noop; + }; + // react native also does not support the addEventListener `signal` option + // so we have to simulate that ourself + controller.signal.addEventListener("abort", cleanupController, { + once: true, + }); } + options.signal = controller.signal; if (useGETForQueries && !isMutationOperation(operation.query)) { options.method = "GET"; @@ -132,11 +151,11 @@ export class BaseHttpLink extends ApolloLink { } }) .then(() => { - controller = undefined; + cleanupController(); observer.complete(); }) .catch((err) => { - controller = undefined; + cleanupController(); observer.error(err); }); From 31a52873ebe0cbccc3bc495c7840baf570a1c0bb Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 6 Aug 2025 18:04:00 +0200 Subject: [PATCH 2/6] capture variable in scope --- src/link/http/BaseHttpLink.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/link/http/BaseHttpLink.ts b/src/link/http/BaseHttpLink.ts index f75ccae13ec..1731b7a14f9 100644 --- a/src/link/http/BaseHttpLink.ts +++ b/src/link/http/BaseHttpLink.ts @@ -95,15 +95,16 @@ export class BaseHttpLink extends ApolloLink { controller = undefined; }; if (options.signal) { + const externalSignal = options.signal; // in an ideal world we could use `AbortSignal.any` here, but // React Native uses https://github.com/mysticatea/abort-controller as // a polyfill for `AbortController`, and it does not support `AbortSignal.any`. const abort = controller.abort.bind(controller); - options.signal.addEventListener("abort", abort, { once: true }); + externalSignal.addEventListener("abort", abort, { once: true }); cleanupController = () => { controller = undefined; // on cleanup, we need to stop listening to `options.signal` to avoid memory leaks - options.signal.removeEventListener("abort", abort); + externalSignal.removeEventListener("abort", abort); cleanupController = noop; }; // react native also does not support the addEventListener `signal` option From de4da0964a7fdfe62bf5e1e463011bf356dd08d7 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 6 Aug 2025 19:12:39 +0200 Subject: [PATCH 3/6] tweak reason, add test --- src/link/http/BaseHttpLink.ts | 11 ++-- src/link/http/__tests__/HttpLink.ts | 89 +++++++++++++++++++---------- 2 files changed, 65 insertions(+), 35 deletions(-) diff --git a/src/link/http/BaseHttpLink.ts b/src/link/http/BaseHttpLink.ts index 1731b7a14f9..032153bac5a 100644 --- a/src/link/http/BaseHttpLink.ts +++ b/src/link/http/BaseHttpLink.ts @@ -95,16 +95,19 @@ export class BaseHttpLink extends ApolloLink { controller = undefined; }; if (options.signal) { - const externalSignal = options.signal; + const externalSignal: AbortSignal = options.signal; // in an ideal world we could use `AbortSignal.any` here, but // React Native uses https://github.com/mysticatea/abort-controller as // a polyfill for `AbortController`, and it does not support `AbortSignal.any`. - const abort = controller.abort.bind(controller); - externalSignal.addEventListener("abort", abort, { once: true }); + + const listener = () => { + controller?.abort(externalSignal.reason); + }; + externalSignal.addEventListener("abort", listener, { once: true }); cleanupController = () => { controller = undefined; // on cleanup, we need to stop listening to `options.signal` to avoid memory leaks - externalSignal.removeEventListener("abort", abort); + externalSignal.removeEventListener("abort", listener); cleanupController = noop; }; // react native also does not support the addEventListener `signal` option diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index f3a9b78d88d..c19066c01b2 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1159,52 +1159,79 @@ describe("HttpLink", () => { expect(abortControllers[0].signal.aborted).toBe(true); }); - it("a passed-in signal will be forwarded to the `fetch` call and not be overwritten by an internally-created one", () => { - const fetch = jest.fn(async (_uri, _options) => - Response.json({ data: { stub: { id: "foo" } } }, { status: 200 }) - ); - const externalAbortController = new AbortController(); + it("a passed-in signal that is cancelled will fail the observable with an `AbortError`", async () => { + try { + fetchMock.restore(); + fetchMock.postOnce( + "data", + async () => '{ "data": { "stub": { "id": "foo" } } }', + { delay: 100 } + ); - const link = createHttpLink({ - uri: "data", - fetch, - fetchOptions: { signal: externalAbortController.signal }, - }); + const externalAbortController = new AbortController(); + const abortControllers = trackGlobalAbortControllers(); - const sub = execute(link, { query: sampleQuery }).subscribe( - failingObserver - ); - sub.unsubscribe(); + const link = createHttpLink({ + uri: "/data", + }); - expect(fetch.mock.calls.length).toBe(1); - expect(fetch.mock.calls[0][1]).toEqual( - expect.objectContaining({ signal: externalAbortController.signal }) - ); + const observable = execute(link, { + query: sampleQuery, + context: { + fetchOptions: { signal: externalAbortController.signal }, + }, + }); + + const internalAbortController = abortControllers[0]; + + const stream = new ObservableStream(observable); + const externalReason = new Error("External abort reason"); + + externalAbortController.abort(externalReason); + + await expect(stream).toEmitError( + // this not being `externalReason` is a quirk of `fetch-mock`: + // https://github.com/wheresrhys/fetch-mock/blob/605ec0afa6a5ff35066b9e01a9bcd688f3c25ce0/packages/fetch-mock/src/Router.ts#L164-L167 + new DOMException("The operation was aborted.", "AbortError") + ); + + expect(externalAbortController).not.toBe(internalAbortController); + expect(externalAbortController.signal.aborted).toBe(true); + expect(externalAbortController.signal.reason).toBe(externalReason); + expect(internalAbortController.signal.aborted).toBe(true); + expect(internalAbortController.signal.reason).toBe(externalReason); + } finally { + fetchMock.restore(); + } }); - it("a passed-in signal that is cancelled will fail the observable with an `AbortError`", async () => { + it("a passed-in signal will not fully overwrite the internally created one", () => { try { + const externalAbortController = new AbortController(); + const abortControllers = trackGlobalAbortControllers(); + fetchMock.restore(); fetchMock.postOnce( "data", async () => '{ "data": { "stub": { "id": "foo" } } }' ); - const externalAbortController = new AbortController(); - - const link = createHttpLink({ + const link = new HttpLink({ uri: "/data", - fetchOptions: { signal: externalAbortController.signal }, }); - const error = await new Promise((resolve) => { - execute(link, { query: sampleQuery }).subscribe({ - ...failingObserver, - error: resolve, - }); - externalAbortController.abort(); - }); - expect(error.name).toBe("AbortError"); + const sub = execute(link, { + query: sampleQuery, + context: { + fetchOptions: { signal: externalAbortController.signal }, + }, + }).subscribe(failingObserver); + const internalAbortController = abortControllers[0]; + + sub.unsubscribe(); + + expect(externalAbortController.signal.aborted).toBe(false); + expect(internalAbortController.signal.aborted).toBe(true); } finally { fetchMock.restore(); } From 9abdffcbb201ce65d34aba1746d9fe378723eddb Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 6 Aug 2025 19:14:54 +0200 Subject: [PATCH 4/6] force cleanup --- src/link/http/BaseHttpLink.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/link/http/BaseHttpLink.ts b/src/link/http/BaseHttpLink.ts index 032153bac5a..7b87136dfd0 100644 --- a/src/link/http/BaseHttpLink.ts +++ b/src/link/http/BaseHttpLink.ts @@ -105,6 +105,7 @@ export class BaseHttpLink extends ApolloLink { }; externalSignal.addEventListener("abort", listener, { once: true }); cleanupController = () => { + controller?.signal.removeEventListener("abort", cleanupController); controller = undefined; // on cleanup, we need to stop listening to `options.signal` to avoid memory leaks externalSignal.removeEventListener("abort", listener); From 25b86e5ad1339a25720e537cad7b5736a99a535b Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 6 Aug 2025 19:20:14 +0200 Subject: [PATCH 5/6] chore --- .size-limits.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.size-limits.json b/.size-limits.json index 4ca7710c07b..b63f73a46ad 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,6 +1,6 @@ { - "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (CJS)": 43776, - "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production) (CJS)": 38564, - "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\"": 33542, - "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 27587 + "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (CJS)": 43876, + "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production) (CJS)": 38663, + "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\"": 33603, + "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 27660 } From 94fea7dfb7bb8e1115264fd2dcaa5cb5828eac13 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 6 Aug 2025 11:52:32 -0600 Subject: [PATCH 6/6] Tweak test description --- src/link/http/__tests__/HttpLink.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index c19066c01b2..0e8830ce0c3 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1159,7 +1159,7 @@ describe("HttpLink", () => { expect(abortControllers[0].signal.aborted).toBe(true); }); - it("a passed-in signal that is cancelled will fail the observable with an `AbortError`", async () => { + it("a passed-in signal that is aborted will fail the observable with an `AbortError`", async () => { try { fetchMock.restore(); fetchMock.postOnce(