HttpLink/BatchHttpLink: Abort AbortController signal more granula…

…rly. (#11040) * keep `observer` reference out of `readMultipartBody` * reset controller reference after request * tests * inline simplified version of `createSignalIfSupported` * changeset * use correct type in `handleError`, remove optional chaining * restructure tests as suggested in PR review * suggested wording change * Update .changeset/smooth-goats-cheat.md Co-authored-by: Jerel Miller <[email protected]> * PR feedback * move `fetch` mock into `mockFetch` function --------- Co-authored-by: Jerel Miller <[email protected]>
apollographql · Jul 12, 2023 · 125ef5b · 125ef5b
1 parent 28f3f92
commit 125ef5b
Show file tree

Hide file tree

Showing 7 changed files with 259 additions and 104 deletions.
diff --git a/.changeset/smooth-goats-cheat.md b/.changeset/smooth-goats-cheat.md
@@ -0,0 +1,10 @@
+---
+'@apollo/client': minor
+---
+
+`HttpLink`/`BatchHttpLink`: Abort the `AbortController` signal more granularly.
+Before this change, when `HttpLink`/`BatchHttpLink` created an `AbortController`
+internally, the signal would always be `.abort`ed after the request was completed. 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 completing.
diff --git a/src/link/batch-http/__tests__/batchHttpLink.ts b/src/link/batch-http/__tests__/batchHttpLink.ts
@@ -4,9 +4,10 @@ import { ASTNode, print, stripIgnoredCharacters } from 'graphql';
 
 import { ApolloLink } from '../../core/ApolloLink';
 import { execute } from '../../core/execute';
-import { Observable } from '../../../utilities/observables/Observable';
+import { Observable, Observer } from '../../../utilities/observables/Observable';
 import { BatchHttpLink } from '../batchHttpLink';
 import { itAsync } from '../../../testing';
+import { FetchResult } from '../../core';
 
 const sampleQuery = gql`
   query SampleQuery {
@@ -1029,4 +1030,100 @@ describe('SharedHttpTest', () => {
       new Error('BatchHttpLink: Trying to send a client-only query to the server. To send to the server, ensure a non-client field is added to the query or enable the `transformOptions.removeClientFields` option.')
     );
   });
+
+  describe('AbortController', () => {
+    const originalAbortController = globalThis.AbortController;
+    afterEach(() => {
+      globalThis.AbortController = originalAbortController;
+    });
+
+    function trackGlobalAbortControllers() {
+      const instances: AbortController[] = []
+      class AbortControllerMock {
+        constructor() {
+          const instance = new originalAbortController()
+          instances.push(instance)
+          return instance
+        }
+      }
+
+      globalThis.AbortController = AbortControllerMock as any;
+      return instances;
+    }
+
+    const failingObserver: Observer<FetchResult> = {
+      next: () => {
+        fail('result should not have been called');
+      },
+      error: e => {
+        fail(e);
+      },
+      complete: () => {
+        fail('complete should not have been called');
+      },
+    }
+
+    function mockFetch() {
+      const text = jest.fn(async () => '{}');
+      const fetch = jest.fn(async (uri, options) => ({ text }));
+      return { text, fetch }
+    }
+
+    it("aborts the request when unsubscribing before the request has completed", () => {
+      const { fetch } = mockFetch();
+      const abortControllers = trackGlobalAbortControllers();
+
+      const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+
+      const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver);
+      sub.unsubscribe();
+
+      expect(abortControllers.length).toBe(1);
+      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 } = mockFetch();
+      const externalAbortController = new AbortController();
+
+      const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: externalAbortController.signal } });
+
+      const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver);
+      sub.unsubscribe();
+
+      expect(fetch.mock.calls.length).toBe(1);
+      expect(fetch.mock.calls[0][1]).toEqual(expect.objectContaining({ signal: externalAbortController.signal }))
+    });
+
+    it('resolving fetch does not cause the AbortController to be aborted', async () => {
+      const { text, fetch } = mockFetch();
+      const abortControllers = trackGlobalAbortControllers();
+      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(abortControllers.length).toBe(1);
+      expect(abortControllers[0].signal.aborted).toBe(false);
+    });
+
+    it('an unsuccessful fetch does not cause the AbortController to be aborted', async () => {
+      const { fetch } = mockFetch();
+      const abortControllers = trackGlobalAbortControllers();
+      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(abortControllers.length).toBe(1);
+      expect(abortControllers[0].signal.aborted).toBe(false);
+    });
+  });
 });
diff --git a/src/link/batch-http/batchHttpLink.ts b/src/link/batch-http/batchHttpLink.ts
@@ -16,7 +16,6 @@ import {
   selectHttpOptionsAndBodyInternal,
   defaultPrinter,
   fallbackHttpConfig,
-  createSignalIfSupported,
 } from '../http/index.js';
 import { BatchLink } from '../batch/index.js';
 import { filterOperationVariables } from "../utils/filterOperationVariables.js";
@@ -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

diff --git a/src/link/http/__tests__/HttpLink.ts b/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,101 @@ 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;
+    describe('AbortController', () => {
+      const originalAbortController = globalThis.AbortController;
+      afterEach(() => {
+        globalThis.AbortController = originalAbortController;
+      });
 
-      fetch.mockReturnValueOnce(Promise.resolve({ text }));
-      text.mockReturnValueOnce(
-        Promise.resolve('{ "data": { "hello": "world" } }'),
-      );
+      function trackGlobalAbortControllers() {
+        const instances: AbortController[] = []
+        class AbortControllerMock {
+          constructor() {
+            const instance = new originalAbortController()
+            instances.push(instance)
+            return instance
+          }
+        }
 
-      const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+        globalThis.AbortController = AbortControllerMock as any;
+        return instances;
+      }
 
-      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');
         },
+      }
+
+      function mockFetch() {
+        const text = jest.fn(async () => '{}');
+        const fetch = jest.fn(async (uri, options) => ({ text }));
+        return { text, fetch }
+      }
+
+      it("aborts the request when unsubscribing before the request has completed", () => {
+        const { fetch } = mockFetch();
+        const abortControllers = trackGlobalAbortControllers();
+
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any });
+
+        const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver);
+        sub.unsubscribe();
+
+        expect(abortControllers.length).toBe(1);
+        expect(abortControllers[0].signal.aborted).toBe(true);
       });
-      sub.unsubscribe();
 
-      setTimeout(
-        makeCallback(resolve, reject, () => {
-          delete (global as any).AbortController;
-          expect(called).toBe(true);
-          fetch.mockReset();
-          text.mockReset();
-        }),
-        150,
-      );
+      it('a passed-in signal will be forwarded to the `fetch` call and not be overwritten by an internally-created one', () => {
+        const { fetch } = mockFetch();
+        const externalAbortController = new AbortController();
+
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: externalAbortController.signal } });
+
+        const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver);
+        sub.unsubscribe();
+
+        expect(fetch.mock.calls.length).toBe(1);
+        expect(fetch.mock.calls[0][1]).toEqual(expect.objectContaining({ signal: externalAbortController.signal }))
+      });
+
+      it('resolving fetch does not cause the AbortController to be aborted', async () => {
+        const { text, fetch } = mockFetch();
+        const abortControllers = trackGlobalAbortControllers();
+        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(abortControllers.length).toBe(1);
+        expect(abortControllers[0].signal.aborted).toBe(false);
+      });
+
+      it('an unsuccessful fetch does not cause the AbortController to be aborted', async () => {
+        const { fetch } = mockFetch();
+        const abortControllers = trackGlobalAbortControllers();
+        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(abortControllers.length).toBe(1);
+        expect(abortControllers[0].signal.aborted).toBe(false);
+      });
     });
 
     const body = '{';

diff --git a/src/link/http/createHttpLink.ts b/src/link/http/createHttpLink.ts
@@ -9,7 +9,7 @@ import { selectURI } from './selectURI.js';
 import {
   handleError,
   readMultipartBody,
-  readJsonBody
+  parseAndCheckHttpResponse
 } from './parseAndCheckHttpResponse.js';
 import { checkFetcher } from './checkFetcher.js';
 import type {
@@ -20,7 +20,6 @@ import {
   defaultPrinter,
   fallbackHttpConfig
 } from './selectHttpOptionsAndBody.js';
-import { createSignalIfSupported } from './createSignalIfSupported.js';
 import { rewriteURIForGET } from './rewriteURIForGET.js';
 import { fromError, filterOperationVariables } from '../utils/index.js';
 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

diff --git a/src/link/http/createSignalIfSupported.ts b/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 };