fix(graphcache): Fix deadlocked layers between deferred and optimistic layers #2861
Conversation
kitten
force-pushed
the
fix/deferred-layer-deadlock
branch
from
4ca5b4b to
323b803
Compare
| if (operation.kind === 'subscription' || result.hasNext) | ||
| reserveLayer(store.data, operation.key, true); |
There was a problem hiding this comment.
This meant that we were:
- unnecessarily adding layers to
optimisticOrdereven when that wasn't needed - where shifting non-deferred layers that already were registered before, which is unexpected. Our tests are relying on this though, so we can "cheat" here
| nextRes({ | ||
| operation: queryOpB, | ||
| data: { | ||
| __typename: 'Query', | ||
| node: { | ||
| __typename: 'Node', | ||
| id: 'node', | ||
| name: 'query b', | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(data).toHaveProperty('node.name', 'query b'); |
There was a problem hiding this comment.
This tested for something that shouldn't ever happen. A result coming in without the layer being registered. This will now write straight to the "base" layer in most cases. Most importantly, this is undefined behaviour and we shouldn't drive-by test for it
| // If the layer has future results then we'll move it past any layer that's | ||
| // still empty, so currently pending operations will take precedence over it | ||
| for ( | ||
| index = index > -1 ? index : 0; |
There was a problem hiding this comment.
This is the relevant changed line. We shift the layer based on where it was before if it's still around, rather than letting it "hop back in front"
| data.commutativeKeys.has(data.optimisticOrder[i]) && | ||
| !data.deferredKeys.has(data.optimisticOrder[i]) | ||
| ) { | ||
| data.commutativeKeys.has(data.optimisticOrder[i]) |
There was a problem hiding this comment.
Here, as discussed, we realised that deferred layers should 100% be squasheable. We basially put deferred layers in the right place, write to them, and then ... we assume that they're never squasheable.
This means that for subscriptions (or other neverending result streams) this layer never goes away, and pending layers that then were on top can never go away either, and have the potential to infinitely be "in the way" of old deferred layers.
|
As a follow up we could create an e2e test but wouldn't call it blocking for this PR as we can publish without |
Resolve #2853
Summary
This fixes a deadlock that could occur in the layers of Graphcache, when a deferred layer (subscriptions or otherwise
hasNextlayers) would be moved past previously optimistic layers (like mutations). In such cases, it would become impossible for preceding layers (i.e. mutations) to ever squash and move "out of the way" causing a deadlock that'd make it impossible for the deferred layer's data to show up when the two layers had overlaps.Instead, we now will try to clear out deferred layers more aggressively. This means that we will squash its data even when it's deferred, when data is written.
Furthermore, we then move the layer from its current position (or recreate it) when it's re-registered, i.e. when a new message/update comes in for the deferred layer.
This should keep the deferred layers data in the cache but still bump it as needed.
Set of changes
optimisticOrderlist