HttpLink/BatchHttpLink: Abort AbortController signal more granularly.

.changeset/smooth-goats-cheat.md

@@ -0,0 +1,13 @@
+---
+'@apollo/client': minor
+---
+
+`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly.
+Before this change, when `HttpLink`/`BatchHttpLink` created an `AbortController`
+internally, that would always be `.abort`ed after the request was finished in any
+way.
+This could cause issues with Sentry Session Replay and Next.js App Router Cache
+invalidations, which just replayed the fetch with the same options - including the
+cancelled `AbortSignal`.
+With this change, the `AbortController` will only be `.abort()`ed by outside events,
+not as a consequence of the request finishing or erroring.

src/link/batch-http/batchHttpLink.ts

@@ -16,7 +16,6 @@ import {
   selectHttpOptionsAndBodyInternal,
   defaultPrinter,
   fallbackHttpConfig,
-  createSignalIfSupported,
 } from '../http';
 import { BatchLink } from '../batch';
 import { filterOperationVariables } from "../utils/filterOperationVariables";
@@ -157,11 +156,10 @@ export class BatchHttpLink extends ApolloLink {
         return fromError<FetchResult[]>(parseError);
       }
 
-      let controller: any;
-      if (!(options as any).signal) {
-        const { controller: _controller, signal } = createSignalIfSupported();
-        controller = _controller;
-        if (controller) (options as any).signal = signal;
+      let controller: AbortController | undefined;
+      if (!options.signal && typeof AbortController !== 'undefined') {
+        controller = new AbortController();
+        options.signal = controller.signal;
       }
 
       return new Observable<FetchResult[]>(observer => {
@@ -173,12 +171,14 @@ export class BatchHttpLink extends ApolloLink {
           })
           .then(parseAndCheckHttpResponse(operations))
           .then(result => {
+            controller = undefined;
             // we have data and can send it to back up the link chain
             observer.next(result);
             observer.complete();
             return result;
           })
           .catch(err => {
+            controller = undefined;
             // fetch was cancelled so its already been cleaned up in the unsubscribe
             if (err.name === 'AbortError') return;
             // if it is a network error, BUT there is graphql result info

src/link/http/__tests__/HttpLink.ts

@@ -13,7 +13,7 @@ import { HttpLink } from '../HttpLink';
 import { createHttpLink } from '../createHttpLink';
 import { ClientParseError } from '../serializeFetchParameter';
 import { ServerParseError } from '../parseAndCheckHttpResponse';
-import { ServerError } from '../../..';
+import { FetchResult, ServerError } from '../../..';
 import { voidFetchDuringEachTest } from './helpers';
 import { itAsync } from '../../../testing';
 
@@ -1325,46 +1325,106 @@ describe('HttpLink', () => {
         }),
       );
     });
-    itAsync('supports being cancelled and does not throw', (resolve, reject) => {
-      let called = false;
-      class AbortController {
-        signal: {};
-        abort = () => {
-          called = true;
-        };
-      }
 
-      (global as any).AbortController = AbortController;
-
-      fetch.mockReturnValueOnce(Promise.resolve({ text }));
-      text.mockReturnValueOnce(
-        Promise.resolve('{ "data": { "hello": "world" } }'),
-      );
+    describe.only('AbortController', () => {
+      function mockGlobalAbortController() {
+        const mockStatus = {
+          aborted: false,
+          created: false
+        }
+        class AbortControllerMock {
+          constructor() {
+            mockStatus.created = true;
+          }
+          signal: {};
+          abort = () => {
+            mockStatus.aborted = true;
+          };
+        }
 
-      const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+        globalThis.AbortController = AbortControllerMock as any;
+        return mockStatus;
+      }
 
-      const sub = execute(link, { query: sampleQuery }).subscribe({
-        next: result => {
-          reject('result should not have been called');
+      const failingObserver: Observer<FetchResult> = {
+        next: () => {
+          fail('result should not have been called');
         },
         error: e => {
-          reject(e);
+          fail(e);
         },
         complete: () => {
-          reject('complete should not have been called');
+          fail('complete should not have been called');
         },
+      }
+
+      // clean up between tests
+      const originalAbortController = globalThis.AbortController;
+      afterEach(() => {
+        globalThis.AbortController = originalAbortController;
+        fetch.mockReset();
+        text.mockReset();
+      })
+
+
+      it('unsubscribing cancels internally created AbortController when request hasn\'t completed', () => {
+        const mock = mockGlobalAbortController();
+        fetch.mockResolvedValueOnce({ text });
+        text.mockResolvedValueOnce('{ "data": { "hello": "world" } }');
+
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+
+        const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver);
+        sub.unsubscribe();
+
+        expect(mock.created).toBe(true);
+        expect(mock.aborted).toBe(true);
       });
-      sub.unsubscribe();
 
-      setTimeout(
-        makeCallback(resolve, reject, () => {
-          delete (global as any).AbortController;
-          expect(called).toBe(true);
-          fetch.mockReset();
-          text.mockReset();
-        }),
-        150,
-      );
+      it('passing a signal in means no AbortController is created internally', () => {
+        const realAbortController = new AbortController();
+        const mock = mockGlobalAbortController();
+        fetch.mockResolvedValueOnce({ text });
+        text.mockResolvedValueOnce('{ "data": { "hello": "world" } }');
+
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: realAbortController.signal } });
+
+        const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver);
+        sub.unsubscribe();
+
+        expect(mock.created).toBe(false);
+        expect(mock.aborted).toBe(false);
+      });
+
+      it('resolving fetch does not cause the AbortController to be aborted', async () => {
+        const mock = mockGlobalAbortController();
+        fetch.mockResolvedValueOnce({ text });
+        text.mockResolvedValueOnce('{ "data": { "hello": "world" } }');
+
+        // (the request is already finished at that point)
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+
+        await new Promise<void>(resolve => execute(link, { query: sampleQuery }).subscribe({
+          complete: resolve
+        }));
+
+        expect(mock.created).toBe(true);
+        expect(mock.aborted).toBe(false);
+      });
+
+      it('throwing an error from fetch does not cause the AbortController to be aborted', async () => {
+        const mock = mockGlobalAbortController();
+        fetch.mockRejectedValueOnce("This is an error!")
+        // the request would be closed by the browser in the case of an error anyways
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+
+        await new Promise<void>(resolve => execute(link, { query: sampleQuery }).subscribe({
+          error: resolve
+        }));
+
+        expect(mock.created).toBe(true);
+        expect(mock.aborted).toBe(false);
+      });
     });
 
     const body = '{';

src/link/http/createHttpLink.ts

@@ -9,7 +9,7 @@ import { selectURI } from './selectURI';
 import {
   handleError,
   readMultipartBody,
-  readJsonBody
+  parseAndCheckHttpResponse
 } from './parseAndCheckHttpResponse';
 import { checkFetcher } from './checkFetcher';
 import type {
@@ -20,7 +20,6 @@ import {
   defaultPrinter,
   fallbackHttpConfig
 } from './selectHttpOptionsAndBody';
-import { createSignalIfSupported } from './createSignalIfSupported';
 import { rewriteURIForGET } from './rewriteURIForGET';
 import { fromError, filterOperationVariables } from '../utils';
 import {
@@ -119,11 +118,10 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => {
       body.variables = filterOperationVariables(body.variables, operation.query);
     }
 
-    let controller: any;
-    if (!(options as any).signal) {
-      const { controller: _controller, signal } = createSignalIfSupported();
-      controller = _controller;
-      if (controller) (options as any).signal = signal;
+    let controller: AbortController | undefined;
+    if (!options.signal && typeof AbortController !== 'undefined') {
+      controller = new AbortController();
+      options.signal = controller.signal;
     }
 
     // If requested, set method to GET if there are no mutations.
@@ -182,18 +180,26 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => {
       // removal of window.fetch, which is unlikely but not impossible.
       const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch;
 
+      const observerNext = observer.next.bind(observer);
       currentFetch!(chosenURI, options)
         .then(response => {
           operation.setContext({ response });
           const ctype = response.headers?.get('content-type');
 
           if (ctype !== null && /^multipart\/mixed/i.test(ctype)) {
-            return readMultipartBody(response, observer);
+            return readMultipartBody(response, observerNext);
           } else {
-            return readJsonBody(response, operation, observer);
+            return parseAndCheckHttpResponse(operation)(response).then(observerNext);
           }
         })
-        .catch(err => handleError(err, observer));
+        .then(() => {
+          controller = undefined;
+          observer.complete();
+        })
+        .catch(err => {
+          controller = undefined;
+          handleError(err, observer)
+        });
 
       return () => {
         // XXX support canceling this request

src/link/http/createSignalIfSupported.ts

@@ -1,3 +1,8 @@
+/**
+ * @deprecated 
+ * This is not used internally any more and will be removed in
+ * the next major version of Apollo Client.
+ */
 export const createSignalIfSupported = () => {
   if (typeof AbortController === 'undefined')
     return { controller: false, signal: false };