Stop feuds by skipping cache writes for unchanged network results. (#…

…6448)

Ever since the big refactoring in #6221 made the client more aggressive
about triggering network requests in response to incomplete cache data
(when using a cache-first FetchPolicy), folks testing the betas/RCs have
reported that feuding queries can end up triggering an endless loop of
unhelpful network requests.

This change does not solve the more general problem of queries competing
with each other to update the same data in incompatible ways (see #6372
for mitigation strategies), but I believe this commit will effectively put
a stop to most cases of endless network requests.

See my lengthy implementation comment for more details, since this is a
non-obvious solution to a very tricky problem.

Fixes #6307.
Fixes #6370.
Fixes #6434.
Fixes #6444.

src/core/QueryInfo.ts

@@ -165,7 +165,10 @@ export class QueryInfo {
     this.variables =
     this.networkStatus =
     this.networkError =
-    this.graphQLErrors = void 0;
+    this.graphQLErrors =
+    this.lastWatch =
+    this.lastWrittenResult =
+    this.lastWrittenVars = void 0;
 
     const oq = this.observableQuery;
     if (oq) oq.stopPolling();
@@ -192,6 +195,9 @@ export class QueryInfo {
     return this;
   }
 
+  private lastWrittenResult?: FetchResult<any>;
+  private lastWrittenVars?: WatchQueryOptions["variables"];
+
   public markResult<T>(
     result: FetchResult<T>,
     options: Pick<WatchQueryOptions,
@@ -200,6 +206,8 @@ export class QueryInfo {
       | "errorPolicy">,
     allowCacheWrite: boolean,
   ) {
+    this.graphQLErrors = isNonEmptyArray(result.errors) ? result.errors : [];
+
     if (options.fetchPolicy === 'no-cache') {
       this.diff = { result: result.data, complete: true };
 
@@ -218,11 +226,57 @@ export class QueryInfo {
         // of writeQuery, so we can store the new diff quietly and ignore
         // it when we receive it redundantly from the watch callback.
         this.cache.performTransaction(cache => {
-          cache.writeQuery({
-            query: this.document!,
-            data: result.data as T,
-            variables: options.variables,
-          });
+          if (equal(result, this.lastWrittenResult) &&
+              equal(options.variables, this.lastWrittenVars)) {
+            // If result is the same as the last result we received from
+            // the network (and the variables match too), avoid writing
+            // result into the cache again. The wisdom of skipping this
+            // cache write is far from obvious, since any cache write
+            // could be the one that puts the cache back into a desired
+            // state, fixing corruption or missing data. However, if we
+            // always write every network result into the cache, we enable
+            // feuds between queries competing to update the same data in
+            // incompatible ways, which can lead to an endless cycle of
+            // cache broadcasts and useless network requests. As with any
+            // feud, eventually one side must step back from the brink,
+            // letting the other side(s) have the last word(s). There may
+            // be other points where we could break this cycle, such as
+            // silencing the broadcast for cache.writeQuery (not a good
+            // idea, since it just delays the feud a bit) or somehow
+            // avoiding the network request that just happened (also bad,
+            // because the server could return useful new data). All
+            // options considered, skipping this cache write seems to be
+            // the least damaging place to break the cycle, because it
+            // reflects the intuition that we recently wrote this exact
+            // result into the cache, so the cache *should* already/still
+            // contain this data. If some other query has clobbered that
+            // data in the meantime, that's too bad, but there will be no
+            // winners if every query blindly reverts to its own version
+            // of the data. This approach also gives the network a chance
+            // to return new data, which will be written into the cache as
+            // usual, notifying only those queries that are directly
+            // affected by the cache updates, as usual. In the future, an
+            // even more sophisticated cache could perhaps prevent or
+            // mitigate the clobbering somehow, but that would make this
+            // particular cache write even less important, and thus
+            // skipping it would be even safer than it is today.
+            if (this.diff && this.diff.complete) {
+              // Reuse data from the last good (complete) diff that we
+              // received, when possible.
+              result.data = this.diff.result;
+              return;
+            }
+            // If the previous this.diff was incomplete, fall through to
+            // re-reading the latest data with cache.diff, below.
+          } else {
+            cache.writeQuery({
+              query: this.document!,
+              data: result.data as T,
+              variables: options.variables,
+            });
+            this.lastWrittenResult = result;
+            this.lastWrittenVars = options.variables;
+          }
 
           const diff = cache.diff<T>({
             query: this.document!,
@@ -241,10 +295,11 @@ export class QueryInfo {
             result.data = diff.result;
           }
         });
+
+      } else {
+        this.lastWrittenResult = this.lastWrittenVars = void 0;
       }
     }
-
-    this.graphQLErrors = isNonEmptyArray(result.errors) ? result.errors : [];
   }
 
   public markReady() {
@@ -254,6 +309,7 @@ export class QueryInfo {
 
   public markError(error: ApolloError) {
     this.networkStatus = NetworkStatus.error;
+    this.lastWrittenResult = this.lastWrittenVars = void 0;
 
     if (error.graphQLErrors) {
       this.graphQLErrors = error.graphQLErrors;

src/core/__tests__/QueryManager/index.ts

@@ -36,6 +36,7 @@ import observableToPromise, {
 import subscribeAndCount from '../../../utilities/testing/subscribeAndCount';
 import { stripSymbols } from '../../../utilities/testing/stripSymbols';
 import { itAsync } from '../../../utilities/testing/itAsync';
+import { ApolloClient } from '../../../ApolloClient';
 
 interface MockedMutation {
   reject: (reason: any) => any;
@@ -2049,19 +2050,6 @@ describe('QueryManager', () => {
             networkStatus: NetworkStatus.ready,
           });
         },
-        result => {
-          expect(stripSymbols(result)).toEqual({
-            data: {
-              ...data2,
-              author: {
-                ...data2.author,
-                id: data1.author.id,
-              },
-            },
-            loading: false,
-            networkStatus: NetworkStatus.ready,
-          });
-        },
       ),
     ]).then(resolve, reject);
   });
@@ -2163,6 +2151,132 @@ describe('QueryManager', () => {
     ]).then(resolve, reject);
   });
 
+  itAsync("should not write unchanged network results to cache", (resolve, reject) => {
+    const cache = new InMemoryCache({
+      typePolicies: {
+        Query: {
+          fields: {
+            info: {
+              merge(_, incoming) {
+                return incoming;
+              },
+            },
+          },
+        },
+      },
+    });
+
+    const client = new ApolloClient({
+      cache,
+      link: new ApolloLink(operation => new Observable((observer: Observer<FetchResult>) => {
+        switch (operation.operationName) {
+          case "A":
+            observer.next!({ data: { info: { a: "ay" }}});
+            break;
+          case "B":
+            observer.next!({ data: { info: { b: "bee" }}});
+            break;
+        }
+        observer.complete!();
+      })),
+    });
+
+    const queryA = gql`query A { info { a } }`;
+    const queryB = gql`query B { info { b } }`;
+
+    const obsA = client.watchQuery({
+      query: queryA,
+      returnPartialData: true,
+    });
+
+    const obsB = client.watchQuery({
+      query: queryB,
+      returnPartialData: true,
+    });
+
+    subscribeAndCount(reject, obsA, (count, result) => {
+      if (count === 1) {
+        expect(result).toEqual({
+          loading: true,
+          networkStatus: NetworkStatus.loading,
+          data: {},
+        });
+      } else if (count === 2) {
+        expect(result).toEqual({
+          loading: false,
+          networkStatus: NetworkStatus.ready,
+          data: {
+            info: {
+              a: "ay",
+            },
+          },
+        });
+      } else if (count === 3) {
+        expect(result).toEqual({
+          loading: true,
+          networkStatus: NetworkStatus.loading,
+          data: {
+            info: {},
+          },
+        });
+      } else if (count === 4) {
+        expect(result).toEqual({
+          loading: false,
+          networkStatus: NetworkStatus.ready,
+          data: {
+            info: {
+              a: "ay",
+            },
+          },
+        });
+        setTimeout(resolve, 100);
+      } else {
+        reject(new Error(`Unexpected ${JSON.stringify({count,result})}`));
+      }
+    });
+
+    subscribeAndCount(reject, obsB, (count, result) => {
+      if (count === 1) {
+        expect(result).toEqual({
+          loading: true,
+          networkStatus: NetworkStatus.loading,
+          data: {},
+        });
+      } else if (count === 2) {
+        expect(result).toEqual({
+          loading: false,
+          networkStatus: NetworkStatus.ready,
+          data: {
+            info: {
+              b: "bee",
+            },
+          },
+        });
+      } else if (count === 3) {
+        expect(result).toEqual({
+          loading: true,
+          networkStatus: NetworkStatus.loading,
+          data: {
+            info: {},
+          },
+        });
+      } else if (count === 4) {
+        expect(result).toEqual({
+          loading: false,
+          networkStatus: NetworkStatus.ready,
+          data: {
+            info: {
+              b: "bee",
+            },
+          },
+        });
+        setTimeout(resolve, 100);
+      } else {
+        reject(new Error(`Unexpected ${JSON.stringify({count,result})}`));
+      }
+    });
+  });
+
   itAsync('should not error when replacing unidentified data with a normalized ID', (resolve, reject) => {
     const queryWithoutId = gql`
       query {