Stop feuds by skipping cache writes for unchanged network results. #6448
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
benjamn
commented
Jun 16, 2020
Comment on lines
+226
to
+259
| 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. |
There was a problem hiding this comment.
Just surfacing this implementation comment, since it explains the design space as I understand it.
benjamn
commented
Jun 16, 2020
This was referenced Jun 16, 2020
benjamn
added a commit
that referenced
this pull request
Aug 10, 2020
When application code evicts an object or its fields from the cache, and those evictions trigger network requests, the network data is often vital for repairing the damage done by the eviction, even if the network data look exactly the same as the previously-written data. In other words, after any cache eviction, we should disable the logic introduced in PR #6448 to stop query feuds. This exception is reasonable because feuding queries only write additional data into the cache, rather than evicting anything, so we can safely assume evictions do not contribute to a cycle of competing cache updates. I freely admit the method of counting evictions that I've implemented here is a bit of a hack, but it works for any ApolloCache implementation, without requiring the public ApolloCache API to support any new functionality. If we identify any other potential consumers of this information, we might consider a more official API.
benjamn
added a commit
that referenced
this pull request
Aug 10, 2020
When application code evicts an object or its fields from the cache, and those evictions trigger network requests, the network data is often vital for repairing the damage done by the eviction, even if the network data look exactly the same as the previously-written data. In other words, after any cache eviction, we should disable the logic introduced in PR #6448 to stop query feuds. This exception is reasonable because feuding queries only write additional data into the cache, rather than evicting anything, so we can safely assume evictions do not contribute to a cycle of competing cache updates. I freely admit the method of counting evictions that I've implemented here is a bit of a hack, but it works for any ApolloCache implementation, without requiring the public ApolloCache API to support any new functionality. If we identify any other potential consumers of this information, we might consider a more official API.
hwillson
pushed a commit
that referenced
this pull request
Aug 21, 2020
When application code evicts an object or its fields from the cache, and those evictions trigger network requests, the network data is often vital for repairing the damage done by the eviction, even if the network data look exactly the same as the previously-written data. In other words, after any cache eviction, we should disable the logic introduced in PR #6448 to stop query feuds. This exception is reasonable because feuding queries only write additional data into the cache, rather than evicting anything, so we can safely assume evictions do not contribute to a cycle of competing cache updates. I freely admit the method of counting evictions that I've implemented here is a bit of a hack, but it works for any ApolloCache implementation, without requiring the public ApolloCache API to support any new functionality. If we identify any other potential consumers of this information, we might consider a more official API.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FetchPolicyofcache-first), 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 other 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.
Should fix #6307, #6370, #6434, and #6444.