[BUG] useQuery race-condition bug

[hakonxxx]

useQuery race condition bug, not update cache when one of multi queries done which refer same cacheId.

Root Cause

maybe is the root cause of #10359

Reproduction

Suggested

const typePolicies = {
  SchemaTest: { merge: true }
}
type SchemaTest = {
  a: any
  b: any
  c: any
  d: any
}
const doc1 = gql`query get1() {
  schemaTest {
   a
   b
 }
}`
const doc2 = gql`query get1() {
  schemaTest {
   a
   c
 }
}`
const doc3 = gql`query get1() {
  schemaTest {
   b
   d
 }
}`

Usage

More easily to reproduce if fill cache with partial mock schemaTest data when ApolloClient initiation.

// assume fetchPolicy is 'cache-and-network'
const useReproduction = () => {
  useQuery(doc1)
  useQuery(doc2)
  useQuery(doc3)
}

Expected Behavior

After all requests done, the cache should be the merge of doc1, doc2 and doc3.

Actual Behavior

After all requests done, the cache could be any combinations of doc1/doc2/doc3.

Versions

@apollo/client: 3.6.9

[jerelmiller]

Hey @hakonxxx 👋

I tried to reproduce this as best I could given the same @apollo/client version and was unable to do so. Check out this codesandbox for a reproduction.

I made an effort to:

• Use the same type names
• Use the same type policy
• Name the GraphQL queries using the same operation name for the 3 queries
• Use 3 useQuery hooks in the same component.
• Try and simulate the race condition by randomizing the time it takes to execute each request

As you can see, the cache appears to merge all the fields correctly, even as requests complete over time.

Would you be willing to try and provide a runnable example that demonstrates failing behavior? Given the information I have here, I'm not sure I have enough to go on to figure out what might be happening.

[hakonxxx]

Hi @jerelmiller , thx for reply~~

i have fork a new sandbox, changes:

1. new InMemoryCache.restore a mock data, which is different from resolver: { ROOT_QUERY: { __typename: "Query", schemaType: { __typename: "SchemaType", a: "a", b: "", c: "", d: "" } } }
2. documents have overlap fields, for example Record<'a' | 'b', string> and Record<'a' | 'b' | 'c', string>

refresh the page several times, and you will see some situations like:

the cache is expected to be { __typename: "SchemaType", a: "a", b: "b", c: "c", d: "d" } right?

BTW:

1. once markResult would broadcastWatches which call queryInfo.setDiff
2. but only trigger queryInfo.notify(which would finally call reobserveCacheFirst) when the oldDiff is diff from newDiff
   so i mock a different cache data when init memoryCache instance.

[hakonxxx]

I doubt the usage of multi queries with overlap fields, but sometimes we just do it indeed :(

So back to this case, maybe there are better solutions instead of just discarding the maybe-staled response(what a waste and may confuse users and FE), maybe we could partially update the cache?

Also there is bug even just discarding the response(sometimes we discarded, sometimes not), because of setTimeout.

[phryneas]

Adding a Replay of the problem happening here.

[dylanwulf]

@hakonxxx not sure if this is helpful, but your reproduction bug seems to get fixed if you change nextFetchPolicy from cache-and-network to cache-first.

[hakonxxx]

@hakonxxx not sure if this is helpful, but your reproduction bug seems to get fixed if you change nextFetchPolicy from cache-and-network to cache-first.

Yes:

1. queryManager.fetchQueryObservable would call queryInfo.observableQuery.applyNextFetchPolicy sync after queryManager.fetchQueryByPolicy and the observableQuery.fetchPolicy changed to cache-first
2. when cache broadcast watches, the queryInfo.notify would not trigger listeners, because queryInfo.shouldNotify would check current fetchPolicy
3. which means not trigger reobserveCacheFirst , not discard the inflight-response

So the behavior seems more weird...

[phryneas]

apollo-client/src/core/ObservableQuery.ts

Line 844 in f75de97

this.concast.removeObserver(this.observer);

This here is the culprit.

We have a chain of
[incoming result]
-> QueryInfo.markResult
-> ...
-> QueryInfo.setDiff
-> [ for other impacted queries ] notify
-> queryInfo.oqListener
-> ObservableQuery.reobserveCacheFirst
-> ObservableQuery.reobserve
-> removing the previous observable - that still waits for the network result
-> killing the concast for that
-> asyncMap will never run into the getResultsFromLink mapping function

That said, having identified the reason and knowing how to fix it are two different things. I'm gonna dig a bit deeper.

[hakonxxx]

I think two things need to consider:

1. maybe contributors could discuss about what behavior is expected here
2. setTimeout make the behaviors random

[phryneas]

Oh, we'll definitely be discussing this internally to see what's going on, this is an excellent reproduction of a race condition that should (as far as I'm concerned) not be happening.

Thank you so much for this reproduction!

[hakonxxx]

seems @3.7.11 fix this bug codesandbox, appreciated~ cc @phryneas

[bignimbus]

Thanks @hakonxxx 🙏🏻 closing this as resolved 🚀

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.