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.

Sign up for GitHub

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

fix(graphcache): Fix deadlocked layers between deferred and optimistic layers #2861

Merged
merged 3 commits into from
Dec 7, 2022
Merged

fix(graphcache): Fix deadlocked layers between deferred and optimistic layers #2861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
323b803
fix(graphcache): Fix deadlocked layers between deferred and optimisti…
kitten Dec 6, 2022
0cc8dc4
Refactor InMemoryData.reserveLayer and reset index for commutative la…
kitten Dec 6, 2022
cf4db2f
Skip calling reserveLayer for non-deferred results
kitten Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/poor-items-shake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@urql/exchange-graphcache': patch
---

Fix a deadlock condition in Graphcache's layers, which is caused by subscriptions (or other deferred layers) starting before one-off mutation layers. This causes the mutation to not be completed, which keeps its data preferred above the deferred layer. That in turn means that layers stop squashing, which causes new results to be missing indefinitely, when they overlap.
23 changes: 0 additions & 23 deletions exchanges/graphcache/src/cacheExchange.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1811,12 +1811,6 @@ describe('commutativity', () => {
variables: undefined,
});

const queryOpB = client.createRequestOperation('query', {
key: 3,
query,
variables: undefined,
});

expect(data).toBe(undefined);

nextOp(queryOpA);
Expand All @@ -1838,23 +1832,6 @@ describe('commutativity', () => {
nextOp(mutationOp);
expect(reexec).toHaveBeenCalledTimes(1);
expect(data).toHaveProperty('node.name', 'optimistic');

// NOTE: We purposefully skip the following:
// nextOp(queryOpB);

nextRes({
operation: queryOpB,
data: {
__typename: 'Query',
node: {
__typename: 'Node',
id: 'node',
name: 'query b',
},
},
});

expect(data).toHaveProperty('node.name', 'query b');
Comment on lines -1845 to -1857
Copy link
Member Author

Choose a reason for hiding this comment

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

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

});

it('applies mutation results on top of commutative queries', () => {
Expand Down
7 changes: 2 additions & 5 deletions exchanges/graphcache/src/cacheExchange.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,8 @@ export const cacheExchange = <C extends Partial<CacheExchangeOpts>>(
optimisticKeysToDependencies.delete(operation.key);
}

reserveLayer(
store.data,
operation.key,
operation.kind === 'subscription' || result.hasNext
);
if (operation.kind === 'subscription' || result.hasNext)
reserveLayer(store.data, operation.key, true);
Comment on lines +216 to +217
Copy link
Member Author

Choose a reason for hiding this comment

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

This meant that we were:

  • unnecessarily adding layers to optimisticOrder even 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


let queryDependencies: void | Dependencies;
let data: Data | null = result.data;
Expand Down
50 changes: 23 additions & 27 deletions exchanges/graphcache/src/store/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,9 @@ export const clearDataState = () => {
while (
--i >= 0 &&
data.refLock.has(data.optimisticOrder[i]) &&
data.commutativeKeys.has(data.optimisticOrder[i]) &&
!data.deferredKeys.has(data.optimisticOrder[i])
) {
data.commutativeKeys.has(data.optimisticOrder[i])
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

)
squashLayer(data.optimisticOrder[i]);
}
}

currentOwnership = null;
Expand Down Expand Up @@ -527,36 +525,34 @@ export const reserveLayer = (
layerKey: number,
hasNext?: boolean
) => {
// Find the current index for the layer, and remove it from
// the order if it exists already
let index = data.optimisticOrder.indexOf(layerKey);
if (index > -1) data.optimisticOrder.splice(index, 1);

if (hasNext) {
data.deferredKeys.add(layerKey);
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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"

index < data.optimisticOrder.length &&
!data.deferredKeys.has(data.optimisticOrder[index]) &&
(!data.refLock.has(data.optimisticOrder[index]) ||
!data.commutativeKeys.has(data.optimisticOrder[index]));
index++
);
} else {
data.deferredKeys.delete(layerKey);
}

let index = data.optimisticOrder.indexOf(layerKey);
if (index > -1) {
if (!data.commutativeKeys.has(layerKey) && !hasNext) {
data.optimisticOrder.splice(index, 1);
// Protect optimistic layers from being turned into non-optimistic layers
// while preserving optimistic data
// Protect optimistic layers from being turned into non-optimistic layers
// while preserving optimistic data
if (index > -1 && !data.commutativeKeys.has(layerKey))
clearLayer(data, layerKey);
} else {
return;
}
}

// 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 = 0;
hasNext &&
index < data.optimisticOrder.length &&
!data.deferredKeys.has(data.optimisticOrder[index]) &&
(!data.refLock.has(data.optimisticOrder[index]) ||
!data.commutativeKeys.has(data.optimisticOrder[index]));
index++
);
}

// Register the layer with the deferred or "top" index and
// mark it as commutative
data.optimisticOrder.splice(index, 0, layerKey);
data.commutativeKeys.add(layerKey);
};
Expand Down