Unmounting a component in the middle of refetching queries will result in cache not getting updated properly

[dannycochran]

Intended outcome:

A mutation with refetchQueries should properly update the cache upon refetchQueries network requests successfully completing, even if the underlying component housing the query is unmounted for whatever reason.

For instance:

const { data ) = useQuery(GetAllFoos);
const [createNewFoo] = useMutation(CREATE_NEW_FOO);

const onCreateNewFoo = useCallback(async () => {
  const result = await createNewFoo({...}, refetchQueries: ['GetAllFoos']);
  history.push(`/newRoute/${result.data.id}`)
}, [createNewFoo);

In this scenario, the refetch queries fire and actually complete. However, in between the time that refetchQueries has completed and the cache write occurs, the component housing the query unmounts. This triggers a series of events:

1. The query cleanup is called here when unmounting: https://github.com/apollographql/apollo-client/blob/main/src/react/hooks/utils/useBaseQuery.ts#L74

2. This calls removeQuerySubscription

3. This calls a number of other functions before finally calling queryInfo.stop()

4. Elsewhere, the query is now ready to write to cache and hits this block of code, but because this.stopped is now true, it will skip the cache write.

IMO I think the usage of this.stopped to gate the cache write does not make sense. Even if a query is unmounted or gone, any results coming in to markResult can still be updated with valid data, but just skip broadcasting any queries like it's already doing here.

Actual outcome:

The cache write is skipped because the component housing the query has unmounted in between the time the refetchQueries have completed and the markReady function is called.

How to reproduce the issue:

It's tricky to reproduce but the code snippet above illustrates how to get into the race condition.

Versions

  System:
    OS: macOS 11.2.2
  Binaries:
    Node: 12.18.4 - /usr/local/bin/node
    Yarn: 1.22.4 - ~/npm-global/bin/yarn
    npm: 6.14.6 - /usr/local/bin/npm
  Browsers:
    Chrome: 90.0.4430.212
    Safari: 14.0.3
  npmPackages:
    @apollo/client: ^3.3.16 => 3.3.16 
    apollo-upload-client: 14.1.3 => 14.1.3 
    apollo3-cache-persist: 0.9.1 => 0.9.1 
  npmGlobalPackages:
    apollo: 2.27.0

[benjamn]

Thanks for the detailed analysis @dannycochran!

After some digging into the Git history: #7278 was the PR where queryInfo.stopped was introduced, but the line using this.stopped in markResult was added in #7661.

In the longer term (AC4), these QueryInfo objects hopefully won't be so tangled up in the React mount/unmount lifecycle. In that world, I think it definitely makes sense to keep recording cache writes regardless of the component's mounted status, unless a more drastic form of shutdown (like client.stop()) has been requested.

[dannycochran]

@benjamn thanks for the response, #7661 makes sense and it seems like we'd want to preserve that behavior to prevent the original garbage collection bug.

Do you remember where that bug was happening? I'm wondering if we can just allow a bypass of some sort from useBaseQuery:

  useEffect(() = () => {
    return () => queryData.cleanup({ allowPendingCacheWrite: true });
  }, []);

and then:

  stopped = false;
  allowPendingCacheWrite = false;

  public markResult(...) {
    const shouldWriteToCache = (!this.stopped || this.allowPendingCacheWrite) && allowCacheWrite;
    if (options.fetchPolicy === 'no-cache') {
      this.diff = { result: result.data, complete: true };
    } else if (shouldWriteToCache) {
      /* ... */
    }
  }

  public stop(options?: StopOptions) {
    if (!this.stopped) {
      this.stopped = true;
      this.allowPendingCacheWrite = options?.allowPendingCacheWrite ?? false;

      /* ... */
    }
  }

[brainkim]

@dannycochran See my comments here #8440. I can’t get reliably reproduce the race condition in the test suite, so I’m unsure how to proceed. Sorry!

[dannycochran]

@brainkim I think to reproduce you just need to:

1. write a mutation test with refetchQueries, and the refetched queries should not resolve immediately, e.g. put a timeout
2. unmount whatever "useBaseQuery" was responsible for refetching a query before the query has resolved
3. note that the refetched query resolved with data but it never wrote to cache.

edit: here's a reproduction repository:
https://github.com/dannycochran/usequery/tree/unmounting