`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. #11040

phryneas · 2023-07-05T15:31:19Z

Before this change, when HttpLink/BatchHttpLink created an AbortController
internally, that would always be .aborted 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.

Compare: getsentry/sentry-javascript#8345 and apollographql/apollo-client-nextjs#24

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests

changeset-bot · 2023-07-05T15:31:22Z

🦋 Changeset detected

Latest commit: fcf54d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-07-05T15:33:07Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	36.89 KB (-0.3% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.47 KB (-0.28% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	42.08 KB (-0.29% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.41 KB (-0.52% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.24 KB (-0.61% 🔽)
import { ApolloProvider } from "dist/react/index.js"	1.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.22 KB (0%)
import { useQuery } from "dist/react/index.js"	4.26 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.08 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.57 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.39 KB (0%)
import { useMutation } from "dist/react/index.js"	2.5 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.48 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	3.56 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	2.98 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	3.88 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.32 KB (0%)
import { useReadQuery } from "dist/react/index.js"	2.66 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	2.64 KB (0%)
import { useFragment } from "dist/react/index.js"	2.05 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2 KB (0%)

.changeset/smooth-goats-cheat.md

jerelmiller · 2023-07-05T22:39:17Z

src/link/batch-http/batchHttpLink.ts

-        const { controller: _controller, signal } = createSignalIfSupported();
-        controller = _controller;
-        if (controller) (options as any).signal = signal;
+      let controller: AbortController | undefined;


🎉 Love this change. Love seeing any start to disappear!

src/link/http/__tests__/HttpLink.ts

jerelmiller · 2023-07-05T22:50:00Z

src/link/http/__tests__/HttpLink.ts

+        expect(created).toBe(true);
+        expect(aborted).toBe(true);


Another reason is that I prefer to inline the setup is that it isolates test state. Because we are asserting on test state that can be written by any test in this describe block, it is impossible to run these tests in parallel because any given test could update this state and affect another. If we can move this state to be local to the test, we don't have to worry about race conditions or test state that affects each other. These types of test failures are SUPER difficult to debug when they happen.

Jest will never run multiple tests in the same file at the same time (unless you actively specify that they should, using test.concurrent) - test parallelism in jest is always about running multiple different files in different node instances at the same time. A file itself will always run top-to-bottom.
There can never be any collision between tests - or otherwise, these tests writing on globalThis, or any kind of mock, would be incredibly dangerous from the start :)

The only way parallelism affects test flimsyness is CPU usage: if the CPU is maxed out, tests just run slower.

Thats fair. I forget jest parallelism out-of-the-box is per-file, not per test.

My point still remains though that I tend to prefer avoiding shared state between multiple tests because they become impossible to debug if the tests need to run in a certain order to pass.

I like the refactor you did though extracting to a function. You're still able to reduce the boilerplate, but my concern about shared state is gone. Thanks for that!

jerelmiller · 2023-07-05T22:56:36Z

src/link/http/__tests__/HttpLink.ts

+        sub.unsubscribe();
+
+        expect(created).toBe(true);
+        expect(aborted).toBe(true);


This test is interesting to me. Based on the implementation and the description in the changeset, I would have expected aborted here to be false since "the AbortController will only be .abort()ed by outside events" (according to the changeset), yet I see no "outside events" in this test.

Looking through some of the other tests, perhaps a tweak to the name of the test would help? The fact that the request hasn't completed is relevant here.

My suggestion 👇

it("unsubscribing cancels internally created AbortController when request hasn't completed"

sub.unsubscribe(); - tearing down the running link subscription because the result it isn't needed anymore would be that outside event.
But yeah, I can tweak the wording a bit :)

jerelmiller · 2023-07-05T22:59:07Z

src/link/http/__tests__/HttpLink.ts

-        150,
-      );
+      it('passing a signal in means no AbortController is created internally', () => {
+        const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: {} } });


Suggested change

const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: {} } });

const controller = new AbortController();

const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: controller.signal } });

I'd prefer if we could test this like the real world to ensure it works as expected.

Yet another reason why I prefer inlining the setup since you'd benefit from not mocking out AbortController here 🙂

The problem with using the unmocked AbortController would be that we cannot test if there is ever a new AbortController() call inside the link - I think mocking of globals is mostly impossible.
I'll see what I can do here.

I actually wonder how valuable this test is since its encroaching on the "testing internal implementation details" territory. We should only need to care what the observable end-user effects are, which in this case is that we don't want to abort requests after they've completed. Whether or not we create an abort controller internally or not technically shouldn't matter.

This would solve the "can't mock global" problem since you wouldn't need to check whether something was created internal to the link or not.

Thoughts?

I'll noodle on that for a bit. I feel like it's important to test that the external AbortSignal is not overwritten, so maybe I can adjust the test accordingly.

jerelmiller · 2023-07-05T23:07:08Z

src/link/http/__tests__/HttpLink.ts

Any chance you could add some of these tests for batch http link? Since the batch link doesn't use HttpLink under the hood, this should give us confidence that implementation works as expected (plus it provides documentation that the batch link also works this way)

Sure thing.

Once you're happy with how the tests are written for the HttpLink, I'll recreate them over there :)

src/link/http/createHttpLink.ts

remove optional chaining

phryneas · 2023-07-06T11:30:03Z

Okay, that should be it - requesting a re-review :)

phryneas · 2023-07-07T08:23:55Z

/release:pr

github-actions · 2023-07-07T08:25:42Z

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

jerelmiller

I'm almost completely satisfied with the tests. See my latest comments. Appreciate the refactor you did with the test setup!

src/link/http/__tests__/HttpLink.ts

jerelmiller · 2023-07-10T17:46:31Z

src/link/http/__tests__/HttpLink.ts

+        const mockStatus = {
+          aborted: false,
+          created: false
+        }


Suggested change

const mockStatus = {

aborted: false,

created: false

}

const mockStatus: 'unused' | 'created' | 'aborted' = 'unused';

Would something like this be easier to work with than 2 booleans? Its impossible to abort an AbortController that wasn't created, so this should encapsulate the behavior nicely. Feel free to use undefined in place of unused here if you prefer that to mean "never instantiated".

Its just a test though, so I'm not hung up on this, just figured the single value might be a bit nicer to work with than multiple flags.

I've given that another refactor - this is now all gone and we're working with a wrapped version of the original AbortController.

src/link/http/__tests__/HttpLink.ts

jerelmiller · 2023-07-10T17:55:48Z

src/link/http/__tests__/HttpLink.ts

-        }),
-        150,
-      );
+      it('passing a signal in means no AbortController is created internally', () => {


The more I think about this, the more I think this test isn't needed. It's testing an implementation detail rather than externally observable behavior. I feel like your next 2 tests test the observable behavior you're trying to fix in this PR and is plenty sufficient.

It's pretty much the "this PR didn't remove intended existing behaviour" test - but I've changed it a bit, so now it should work without touching any implementation details.

src/link/http/__tests__/HttpLink.ts

Co-authored-by: Jerel Miller <[email protected]>

…ortcontroller-ganularity

phryneas · 2023-07-11T13:11:51Z

Okay, at this point I'm so convinced that you will be okay with the tests that I already copied them over ;)

phryneas · 2023-07-11T13:14:36Z

src/link/http/__tests__/HttpLink.ts

+      function mockFetch() {
+        const text = jest.fn(async () => '{}');
+        const fetch = jest.fn(async (uri, options) => ({ text }));
+        return { text, fetch }
+      }


The global text and fetch mocks in this file are... weird. They do not properly reset in-between tests, and I'm too afraid to touch that, so here's this new helper for these new tests instead, which creates non-global helpers and doesn't influence other types.

jerelmiller

💯 🔥 Thanks so much for updating those tests! Love the changes you made there. 🚢 it

phryneas added 5 commits July 5, 2023 15:21

keep observer reference out of readMultipartBody

ae5a3a8

reset controller reference after request

fd3a79b

tests

bf6c2b6

inline simplified version of createSignalIfSupported

0b36195

changeset

3d9fddb

phryneas self-assigned this Jul 5, 2023

jerelmiller requested changes Jul 5, 2023

View reviewed changes

jerelmiller added this to the Release 3.8 milestone Jul 6, 2023

phryneas added 3 commits July 6, 2023 12:25

use correct type in handleError,

f9f1022

remove optional chaining

restructure tests as suggested in PR review

766964b

suggested wording change

0723cce

Merge branch 'release-3.8' into pr/httplink-abortcontroller-ganularity

8bb804d

jerelmiller reviewed Jul 10, 2023

View reviewed changes

phryneas and others added 4 commits July 11, 2023 13:16

Update .changeset/smooth-goats-cheat.md

8767527

Co-authored-by: Jerel Miller <[email protected]>

PR feedback

bf36ad4

move fetch mock into mockFetch function

13b2ca9

Merge remote-tracking branch 'origin/release-3.8' into pr/httplink-ab…

97dbfe2

…ortcontroller-ganularity

phryneas commented Jul 11, 2023

View reviewed changes

jerelmiller approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'release-3.8' into pr/httplink-abortcontroller-ganularity

fcf54d6

phryneas merged commit 125ef5b into release-3.8 Jul 12, 2023
4 checks passed

github-actions bot mentioned this pull request Jul 12, 2023

Version Packages (rc) #11057

Merged

phryneas deleted the pr/httplink-abortcontroller-ganularity branch July 12, 2023 09:53

This comment was marked as outdated.

Sign in to view

This was referenced Aug 2, 2023

Version Packages (alpha) #11122

Closed

Version Packages #11126

Merged

Version Packages (alpha) #11127

Closed

alessbell mentioned this pull request Aug 10, 2023

Fix issue where a bare install had some issues with a @types/node import in some cases #11141

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. #11040

`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. #11040

phryneas commented Jul 5, 2023 •

edited

Loading

changeset-bot bot commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

jerelmiller Jul 5, 2023

jerelmiller Jul 5, 2023

phryneas Jul 6, 2023 •

edited

Loading

jerelmiller Jul 10, 2023

jerelmiller Jul 5, 2023

phryneas Jul 6, 2023

jerelmiller Jul 5, 2023

phryneas Jul 6, 2023

jerelmiller Jul 10, 2023

phryneas Jul 11, 2023

jerelmiller Jul 5, 2023

phryneas Jul 6, 2023

phryneas Jul 6, 2023

phryneas commented Jul 6, 2023

phryneas commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

jerelmiller left a comment

jerelmiller Jul 10, 2023

phryneas Jul 11, 2023

jerelmiller Jul 10, 2023

phryneas Jul 11, 2023

phryneas commented Jul 11, 2023

phryneas Jul 11, 2023

jerelmiller left a comment

This comment was marked as outdated.

	const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: {} } });
	const controller = new AbortController();
	const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: controller.signal } });

HttpLink/BatchHttpLink: Abort AbortController signal more granularly. #11040

HttpLink/BatchHttpLink: Abort AbortController signal more granularly. #11040

Conversation

phryneas commented Jul 5, 2023 • edited Loading

Checklist:

changeset-bot bot commented Jul 5, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 5, 2023 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Jul 6, 2023

phryneas commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

jerelmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Jul 11, 2023

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. #11040

`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. #11040

phryneas commented Jul 5, 2023 •

edited

Loading

changeset-bot bot commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

phryneas Jul 6, 2023 •

edited

Loading