Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run final mutation update against non-optimistic data. #6551

Merged
merged 6 commits into from
Jul 8, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jul 6, 2020

Providing an optimisticResponse when performing a mutation is never mandatory, since optimistic updates are an optimization for perceived performance. By corollary, if you switch back and forth between providing an optimisticResponse and simply waiting for the mutation to finish, that choice should have no lasting impact on the final state of the cache. After the mutation finishes, you should not be able to tell that there was ever an optimisticResponse.

InMemoryCache maintains a system of layered EntityStore objects to represent optimistic updates, so it can easily throw individual layers away when a mutation finishes, ultimately leaving only the "realistic" data behind. However, in cases where two or more optimistic mutations are running at the same time, directly competing to update the same data, it is possible for the final update function call for one mutation to leak still-active optimistic data from the other mutation into the root layer of the cache. This leaking of optimistic data poses a subtle problem for the goals described above, because it means using an optimistic response can have lasting effects on non-optimistic cache data.

To address this problem, we need to make optimistic data invisible to the final (non-optimistic) invocation of the mutation update function, somehow. Because of the layering of the InMemoryCache, this turns out to be relatively straightforward, since the root layer is accessible in isolation from any optimistic data that might be layered on top of it.

Specifically, the final time we call the update function for the mutation, we do so within a cache transaction where cache.optimisticData is temporarily set to refer to the same root EntityStore object as cache.data, so a typical readQuery-transform-writeQuery update pattern cannot accidentally read from the optimistic data of other concurrent mutations. Note that we do not need to wait for all optimistic mutations to finish before running the final update function for an individual mutation, because we have the ability to hide optimistic data without removing it (yet).

This bug is relatively rare because it requires two or more optimistic mutations to be competing to update the same data at the same time, which is uncommon. However, the bug is reliably reproducible under such circumstances, and is now adequately regression-tested (see tests included in this PR).

@benjamn benjamn added this to the Release 3.0 milestone Jul 6, 2020
@benjamn benjamn self-assigned this Jul 6, 2020
@benjamn benjamn requested review from hwillson and jcreighton July 6, 2020 16:24
Comment on lines +683 to +685
}, error => {
subscriptionHandle.unsubscribe();
expect(error.message).toBe(`Hello... It's me.`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests were failing silently because of the way the tryFunctionOrLogError function was turning exceptions into console errors. In other words, eliminating tryFunctionOrLogError is important to ensure these tests will actually report future regressions in the way exceptions are handled for mutation update functions.

Comment on lines +2033 to +2057
expect(cache.extract(true)).toEqual({
ROOT_QUERY: {
__typename: "Query",
items: [
// If we wanted to keep optimistic data as up-to-date as
// possible, we could rerun all optimistic transactions
// after writing to the root (non-optimistic) layer of the
// cache, which would result in mutationItem appearing in
// this list along with manualItem1 and manualItem2
// (presumably in that order). However, rerunning those
// optimistic transactions would trigger additional
// broadcasts for optimistic query watches, with
// intermediate results that (re)combine optimistic and
// non-optimistic data. Since rerendering the UI tends to be
// expensive, we should prioritize broadcasting states that
// matter most, and in this case that means broadcasting the
// initial optimistic state (for perceived performance),
// followed by the final, authoritative, non-optimistic
// state. Other intermediate states are a distraction, as
// they will probably soon be superseded by another (more
// authoritative) update. This particular state is visible
// only because we haven't rolled back this manual Layer
// just yet (see cache.removeOptimistic below).
manualItem1,
manualItem2,
Copy link
Member Author

@benjamn benjamn Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this comment since it brings up a tricky issue that I investigated but decided not to change in this PR.

Replaying optimistic transactions involves running arbitrary user code, so doing it repeatedly (e.g. whenever the root layer changes) could have serious performance (and perhaps even logical) implications. I considered keeping track of which fields were read by the previous update function, so that the cache could skip replaying updates not affected by the latest changes, but I think that overcomplicates the problem, and leads to a situation where it's very hard to predict how many times your update function will run.

At the end of the day, any marginal benefits of repeatedly recomputing optimistic data are generally overshadowed by the benefits of just throwing the optimistic data away after the mutation finishes.

benjamn added a commit that referenced this pull request Jul 6, 2020
@benjamn benjamn force-pushed the non-optimistic-final-mutation-update branch from 0b3ab8a to 0d733f7 Compare July 7, 2020 14:03
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great solution @benjamn 👍 - thanks!

@benjamn benjamn merged commit d188b3f into master Jul 8, 2020
@benjamn benjamn deleted the non-optimistic-final-mutation-update branch July 8, 2020 19:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants