don't cancel existing subscriptions when updating cache from another source #10597

phryneas · 2023-02-24T17:13:48Z

This PR fixes an issue first reproduced in #10587.

For an explanation of the bug, take a look at this commit (a test) (That test is not very visible in the overall diff since I reformatted the ObservableQuery test.)

Basically: when a cache update from another source was triggered, that other cache update discarded & unsubscribed the currently running Link - even if it only was reading from cache.

With this PR, concasts now contain an array of "descriptions", describing the actions they contain - a link request, an update from cache, or unknown.

This allows the logic in reobserve to only replace the current concast if the incoming data source was a link request - which should be a more sensible approach.

Unfortunately, this adds about 100b of bundle size - but especially with long-running queries with @defer in the future I don't see a way around this fix.

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-02-24T17:13:52Z

🦋 Changeset detected

Latest commit: 84686b8

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	Patch

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

netlify · 2023-02-24T17:17:17Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`169840f`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/6422a1495948390008183fdb
😎 Deploy Preview	https://deploy-preview-10597--apollo-client-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

phryneas · 2023-03-02T14:04:09Z

This could also have some overlap with #10614, so I'm mentioning that here so I don't forget to take a look at that in this context.

phryneas · 2023-03-20T14:33:03Z

src/core/__tests__/ObservableQuery.ts

+              b: "",
+            },
+          },
+          // TODO: this should be `true`, but that seems to be a separate bug!


This seems like invalid behavior to me, but also is an "independent" bug.
I will try to handle this in a separate pull request.

phryneas · 2023-03-27T13:29:31Z

src/core/QueryManager.ts

+      // there is just no way we can synchronously get the *right* value here,
+      // so we will assume `true`, which is the behaviour before the bug fix in
+      // #10597. This means that bug is not fixed in that case, and is probably
+      // un-fixable with reasonable effort for the edge case of @export as
+      // directives.
+      containsDataFromLink = true;


This is just the unfortunate truth - we need this information synchronously to act on it, and there is no way we can do that. So this PR has to exclude fixing that.

phryneas · 2023-03-27T13:31:05Z

I managed to get this PR "out of the Concast", to have much more local impact - and less bundle size. As a result, some internal functions now return a tuple instead of a single value, but that seemed to be the "least impact" solution here.

@benjamn could you please take another look?

benjamn · 2023-03-27T16:12:15Z

src/core/QueryManager.ts

-    networkStatus: NetworkStatus,
-  ): ConcastSourcesArray<ApolloQueryResult<TData>> {
+    networkStatus: NetworkStatus
+  ): [containsDataFromLink: boolean, ...sources: ConcastSourcesArray<ApolloQueryResult<TData>>] {


I'm very glad to see we can keep this logic out of the Concast implementation, but I'm a little uneasy with combining distinct pieces of information into one tuple array, especially when one of those items is also an array (sources), and the other item is an unnamed boolean value (containsDataFromLink).

For readability and maintainability (like adding more info properties in the future), I think I would prefer returning an object with named properties instead of a tuple, even though this is private code that we fully control.

However, I do appreciate why a tuple might minify better than an object with named properties, so I did a quick investigation to see how much bundle size would be added by using objects instead of tuples. The object-based approach adds +172 bytes, still coming in under the current bundle size limit of 32.72KB, which I think is worth it for readability and simpler typing.

Since I've already done this experiment, rather than asking you to reproduce the results, I could just push my commit to this branch, if that's okay with you @phryneas?

Here's the commit: 18cb2f3

#10597 (comment)

benjamn · 2023-03-27T16:44:01Z

src/core/QueryManager.ts

-          return [false, resultsFromCache(diff, queryInfo.markReady())];
+          return { fromLink: false, sources: [resultsFromCache(diff, queryInfo.markReady())] };
        }

        if (returnPartialData || shouldNotify) {
-          return [true, resultsFromCache(diff), resultsFromLink()];
+          return { fromLink: true, sources: [resultsFromCache(diff), resultsFromLink()] };


I originally used a helper function here since I thought this repetitive code might be bad for bundle size, but it seems GZIP is able to compress the two repeated prefixes reasonably well:

return{fromLink:false,sources:[ return{fromLink:true,sources:[

benjamn

I'm good with the current state of this PR if you are, @phryneas!

jerelmiller · 2023-03-27T19:50:47Z

src/core/__tests__/ObservableQuery.ts

@@ -1,41 +1,39 @@
-import gql from 'graphql-tag';


Is the commit that formats this file somewhere in .git-blame-ignore-revs? I didn't see at as part of this PR.

it is in here. But it's a good point, I'll have to rewrite history on this PR so it can be merged without squashing.

jerelmiller

Had a couple of minor points of feedback, but this PR looks good otherwise. Thanks for getting this fixed! Looks like it was a tricky one to figure out 😄

jerelmiller · 2023-03-27T20:01:54Z

.changeset/swift-mails-search.md

+"@apollo/client": patch
+---
+
+Fix a potential race condition between a link and parallel cache updates


Suggested change

Fix a potential race condition between a link and parallel cache updates

Fix a race condition where parallel cache updates from multiple queries would sometimes result in incorrect `data`.

See if you like this a bit better. I think the "between a link" feels a bit too ambiguous, or at least I'm not sure it adequately describes the fix here. Seems like the "multiple queries" is important looking at your test. Feel free to tweak this a bit more, but just wanted something that might help a dev looking at our changelog understand what we fixed!

I've reworded it to "Fix a bug where an incoming cache update could prevent future updates from the active link."

jerelmiller · 2023-03-27T20:03:50Z

src/core/QueryManager.ts

-        ).then(fromVariables)
-        : fromVariables(normalized.variables!)
-    );
+    let concast: Concast<ApolloQueryResult<TData>>, containsDataFromLink: boolean;


Suggested change

let concast: Concast<ApolloQueryResult<TData>>, containsDataFromLink: boolean;

let concast: Concast<ApolloQueryResult<TData>>,

containsDataFromLink: boolean;

I know this seems petty, but I honestly searched for a while before I realized this variable was defined in the same line as concast. For readability, could we put this on its own line to make it easier to see where this variable is created?

Not petty, perfectly valid complaint :)

(cherry picked from commit 0a67647)

… from the active link. Co-authored-by: Ben Newman <ben@apollographql.com>

phryneas · 2023-03-28T09:39:58Z

@jerelmiller once you're happy with the wording, this is ready for the "Rebase and Merge" (we have one separate formatting commit) button :)

phryneas · 2023-03-29T08:52:56Z

Well, I didn't hear any complaints about the new wording, so I'm merging :)

jerelmiller · 2023-03-31T19:40:14Z

@phryneas whoops missed your ping. Sorry about that! It's good 😄

phryneas requested a review from benjamn February 24, 2023 17:13

phryneas force-pushed the pr/explore-10587 branch from c1e7b79 to 9fff05a Compare March 17, 2023 16:45

phryneas marked this pull request as ready for review March 20, 2023 14:31

phryneas commented Mar 20, 2023

View reviewed changes

phryneas requested review from jerelmiller and alessbell March 20, 2023 14:38

phryneas commented Mar 27, 2023

View reviewed changes

benjamn reviewed Mar 27, 2023

View reviewed changes

benjamn added a commit that referenced this pull request Mar 27, 2023

Return { concast, fromLink } objects instead of flat tuples.

18cb2f3

#10597 (comment)

benjamn reviewed Mar 27, 2023

View reviewed changes

benjamn approved these changes Mar 27, 2023

View reviewed changes

jerelmiller reviewed Mar 27, 2023

View reviewed changes

jerelmiller approved these changes Mar 27, 2023

View reviewed changes

format "ObservableQuery" test

78809ce

(cherry picked from commit 0a67647)

phryneas force-pushed the pr/explore-10587 branch from 169840f to 6ab5479 Compare March 28, 2023 09:26

Fix a bug where an incoming cache update could prevent future updates…

84686b8

… from the active link. Co-authored-by: Ben Newman <ben@apollographql.com>

phryneas force-pushed the pr/explore-10587 branch from 6ab5479 to 84686b8 Compare March 28, 2023 09:29

phryneas merged commit 8fb9d19 into main Mar 29, 2023

phryneas deleted the pr/explore-10587 branch March 29, 2023 08:53

github-actions bot mentioned this pull request Mar 29, 2023

Version Packages #10630

Merged

alessbell mentioned this pull request Apr 13, 2023

fix: failing useSuspenseQuery tests #10763

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't cancel existing subscriptions when updating cache from another source #10597

don't cancel existing subscriptions when updating cache from another source #10597

phryneas commented Feb 24, 2023 •

edited

Loading

changeset-bot bot commented Feb 24, 2023 •

edited

Loading

netlify bot commented Feb 24, 2023 •

edited

Loading

phryneas commented Mar 2, 2023

phryneas Mar 20, 2023

phryneas Mar 27, 2023

phryneas commented Mar 27, 2023

benjamn Mar 27, 2023

benjamn Mar 27, 2023

benjamn Mar 27, 2023

benjamn left a comment

jerelmiller Mar 27, 2023

phryneas Mar 28, 2023

jerelmiller left a comment

jerelmiller Mar 27, 2023

phryneas Mar 28, 2023

jerelmiller Mar 27, 2023

phryneas Mar 28, 2023

phryneas commented Mar 28, 2023

phryneas commented Mar 29, 2023

jerelmiller commented Mar 31, 2023

	Fix a potential race condition between a link and parallel cache updates
	Fix a race condition where parallel cache updates from multiple queries would sometimes result in incorrect `data`.

	let concast: Concast<ApolloQueryResult<TData>>, containsDataFromLink: boolean;
	let concast: Concast<ApolloQueryResult<TData>>,
	containsDataFromLink: boolean;

don't cancel existing subscriptions when updating cache from another source #10597

don't cancel existing subscriptions when updating cache from another source #10597

Conversation

phryneas commented Feb 24, 2023 • edited Loading

Checklist:

changeset-bot bot commented Feb 24, 2023 • edited Loading

🦋 Changeset detected

netlify bot commented Feb 24, 2023 • edited Loading

✅ Deploy Preview for apollo-client-docs ready!

phryneas commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 Mar 28, 2023

phryneas commented Mar 29, 2023

jerelmiller commented Mar 31, 2023

phryneas commented Feb 24, 2023 •

edited

Loading

changeset-bot bot commented Feb 24, 2023 •

edited

Loading

netlify bot commented Feb 24, 2023 •

edited

Loading