Fixes race condition when changing back to initial variables before request finishes #11186

jerelmiller · 2023-08-31T03:51:53Z

Fixes an issue where the wrong result was returned when switching back to initial variables with a cached result before the request for previous changed variables finished.

Prior to v3.7.11, changing variables would abort a previous request, which meant there was no cache update for the previous variables. This behavior had some edge cases and resulted in some race condition bugs. This was fixed by #10597, but because #10597 no longer aborted requests, the previous request would complete and the cache would be written, but the broadcasted result would be incorrectly associated with the current variables.

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

…lts in bug

changeset-bot · 2023-08-31T03:51:57Z

🦋 Changeset detected

Latest commit: 2b1ce1d

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

github-actions · 2023-08-31T03:53:42Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.27 KB (-0.02% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.75 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	42.32 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.52 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.27 KB (-0.01% 🔽)
import { ApolloProvider } from "dist/react/index.js"	1.21 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.2 KB (0%)
import { useQuery } from "dist/react/index.js"	4.27 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.08 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.58 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.39 KB (0%)
import { useMutation } from "dist/react/index.js"	2.52 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.5 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.23 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.19 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	4.75 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.19 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.28 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.7 KB (0%)
import { useReadQuery } from "dist/react/index.js"	2.96 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	2.91 KB (0%)
import { useFragment } from "dist/react/index.js"	2.07 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.02 KB (0%)

netlify · 2023-08-31T03:56:05Z

✅ Deploy Preview for apollo-client-docs ready!

Name	Link
🔨 Latest commit	`1bcaddb`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-client-docs/deploys/64f00e5c628b3a000870287e
😎 Deploy Preview	https://deploy-preview-11186--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 configuration.

jerelmiller · 2023-08-31T03:56:27Z

src/core/QueryInfo.ts

@@ -157,14 +157,14 @@ export class QueryInfo {
    this.dirty = false;
  }

-  getDiff(variables = this.variables): Cache.DiffResult<any> {
-    const options = this.getDiffOptions(variables);
+  getDiff(): Cache.DiffResult<any> {


Turns out there was only one area of the codebase that called getDiff with variables, which was the readCache function in QueryManager#fetchQueryByPolicy. That function calls queryInfo.init(...) before that readCache function is called, therefore updating this.variables before this function is executed.

This parameter was always confusing to me because it felt too easy to accidentally change the variables being looked at by this QueryInfo instance. I prefer more intentional changes to this.variables to try and reduce the surface area where variables could change.

There is still lots more that could be refactored to make it more intentional, but that feels outside the scope of this PR and could open a big can of worms. For now, this change seemed reasonable as a step toward that future without breaking existing functionality.

jerelmiller · 2023-08-31T03:57:30Z

src/core/QueryManager.ts

@@ -1516,7 +1516,7 @@ export class QueryManager<TStore> {
      networkStatus,
    });

-    const readCache = () => queryInfo.getDiff(variables);
+    const readCache = () => queryInfo.getDiff();


As alluded to in #11186 (comment) this was the only area of the codebase that passed variables to getDiff. Since queryInfo.init(...) is called a few lines up, this argument felt unneeded since init will set this.variables.

jerelmiller · 2023-08-31T06:08:22Z

src/core/QueryInfo.ts

+      // In case variables have changed and there is a race condition where the
+      // previous variables request finishes after the current variables
+      // request, skip the cache update.
+      equal(this.variables, variables) &&


The one bummer about this particular change is that we miss a nice opportunity to write a fetched result to the cache, even though it isn't actively used right away. Since this change prevents the cache write when variables don't match, this means that switching back to the variables will re-execute the network request since the cache wasn't previously written.

Unfortunately I couldn't find a simpler solution to this bug. The cache write triggers a cache broadcast, but it broadcasts the wrong result (or rather, the result it broadcasts is associated to the wrong set of variables). I attempted to disable broadcasting entirely in the cache.writeQuery call, but this did not seem to work.

False alarm! I was able to fix this in 20b2932

jerelmiller · 2023-08-31T06:12:44Z

/release:pr

github-actions · 2023-08-31T06:14:26Z

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11186-20230831061346.

jerelmiller · 2023-08-31T06:41:53Z

/release:pr

github-actions · 2023-08-31T06:43:09Z

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11186-20230831064231.

.changeset/odd-beers-perform.md

jerelmiller · 2023-08-31T20:26:13Z

.changeset/odd-beers-perform.md

+"@apollo/client": patch
+---
+
+Fixes an issue where triggering a query with different variables, then rapidly changing back to initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables.


I don't love the way this is worded, so any suggestions here are welcome.

I see what you mean after the reading the test, but yeah there's a lot to keep track of conceptually 😅 Maybe using VariablesA and VariablesB could be helpful? Something like:

If a user triggers a query with VariablesA, then triggers the same query with VariablesB but switches back to VariablesA before the response for the fetch made with VariablesB is returned, the user will now correctly receive the cached data for the request made with VariablesA. (Previously, the user would see the data for VariablesB even after switching the variables back to VariablesA once the request made with VariablesB completed.)

alessbell

LGTM! And glad to have that this.variables assignment expression out of the call to updateWatch :)

alessbell · 2023-09-01T15:31:07Z

.changeset/odd-beers-perform.md

+"@apollo/client": patch
+---
+
+Fixes an issue where triggering a query with different variables, then rapidly changing back to initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables.


I see what you mean after the reading the test, but yeah there's a lot to keep track of conceptually 😅 Maybe using VariablesA and VariablesB could be helpful? Something like:

If a user triggers a query with VariablesA, then triggers the same query with VariablesB but switches back to VariablesA before the response for the fetch made with VariablesB is returned, the user will now correctly receive the cached data for the request made with VariablesA. (Previously, the user would see the data for VariablesB even after switching the variables back to VariablesA once the request made with VariablesB completed.)

jerelmiller · 2023-09-01T16:31:56Z

@alessbell I like that a lot more. At least its easier to keep track of what "variables" the changelog is referring to. Thanks for the suggestion!

jerelmiller added 4 commits August 30, 2023 17:45

Add test validating that switching variables in quick succession resu…

4ba46a3

…lts in bug

Remove the argument to QueryInfo#getDiff which is not needed

bb93818

Don't write to the cache in markResult if variables don't match

1bcaddb

Add changeset

f50ffd0

jerelmiller requested review from benjamn, phryneas and alessbell August 31, 2023 03:51

jerelmiller commented Aug 31, 2023

View reviewed changes

More accurate language in test description

2d16454

jerelmiller added the 🐞 bug label Aug 31, 2023

jerelmiller commented Aug 31, 2023

View reviewed changes

Dont update watch if variables don't match

20b2932

jerelmiller mentioned this pull request Aug 31, 2023

useQuery refreshing with the wrong data (corresponding to previous variables) #11184

Closed

jerelmiller commented Aug 31, 2023

View reviewed changes

.changeset/odd-beers-perform.md Outdated Show resolved Hide resolved

Fix typo in changeset

7c173a2

jerelmiller commented Aug 31, 2023

View reviewed changes

Merge branch 'main' into issue-11184/race-condition-bug

9b32cbb

alessbell approved these changes Sep 1, 2023

View reviewed changes

Better changelog message

2b1ce1d

jerelmiller merged commit f1d429f into main Sep 1, 2023

jerelmiller deleted the issue-11184/race-condition-bug branch September 1, 2023 18:04

github-actions bot mentioned this pull request Sep 1, 2023

Version Packages #11164

Merged

github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes race condition when changing back to initial variables before request finishes #11186

Fixes race condition when changing back to initial variables before request finishes #11186

jerelmiller commented Aug 31, 2023 •

edited

Loading

changeset-bot bot commented Aug 31, 2023 •

edited

Loading

github-actions bot commented Aug 31, 2023 •

edited

Loading

netlify bot commented Aug 31, 2023

jerelmiller Aug 31, 2023

jerelmiller Aug 31, 2023 •

edited

Loading

jerelmiller Aug 31, 2023 •

edited

Loading

jerelmiller commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

jerelmiller commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

jerelmiller Aug 31, 2023

alessbell Sep 1, 2023

alessbell left a comment

alessbell Sep 1, 2023

jerelmiller commented Sep 1, 2023

Fixes race condition when changing back to initial variables before request finishes #11186

Fixes race condition when changing back to initial variables before request finishes #11186

Conversation

jerelmiller commented Aug 31, 2023 • edited Loading

Checklist:

changeset-bot bot commented Aug 31, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Aug 31, 2023 • edited Loading

size-limit report 📦

netlify bot commented Aug 31, 2023

✅ Deploy Preview for apollo-client-docs ready!

jerelmiller Aug 31, 2023

Choose a reason for hiding this comment

jerelmiller Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

jerelmiller Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

jerelmiller commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

jerelmiller commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

jerelmiller Aug 31, 2023

Choose a reason for hiding this comment

alessbell Sep 1, 2023

Choose a reason for hiding this comment

alessbell left a comment

Choose a reason for hiding this comment

alessbell Sep 1, 2023

Choose a reason for hiding this comment

jerelmiller commented Sep 1, 2023

jerelmiller commented Aug 31, 2023 •

edited

Loading

changeset-bot bot commented Aug 31, 2023 •

edited

Loading

github-actions bot commented Aug 31, 2023 •

edited

Loading

jerelmiller Aug 31, 2023 •

edited

Loading

jerelmiller Aug 31, 2023 •

edited

Loading