Skip to content

Commit 4b0c113

Browse files
yaacovCRyaacovCR
authored
fix(incremental): do not initiate non-pending execution groups (graphql#4140)
Currently, when early execution is disabled, we still use the early execution logic to initiate execution groups, which may cause early initiation of non-pending execution groups. alternative to graphql#4109, causes potentially stacking delays when combinations of shared and non-shared keys between sibling defers cause separate deferred grouped field sets for the same deferred fragment.
1 parent d9fc656 commit 4b0c113

File tree

3 files changed

+272
-26
lines changed

3 files changed

+272
-26
lines changed

‎src/execution/IncrementalGraph.ts

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,10 @@ export class IncrementalGraph {
108108
reconcilableResults: ReadonlyArray<ReconcilableDeferredGroupedFieldSetResult>;
109109
}
110110
| undefined {
111-
// TODO: add test case?
112-
/* c8 ignore next 3 */
113-
if (!this._rootNodes.has(deferredFragmentRecord)) {
114-
return;
115-
}
116-
if (deferredFragmentRecord.deferredGroupedFieldSetRecords.size > 0) {
111+
if (
112+
!this._rootNodes.has(deferredFragmentRecord) ||
113+
deferredFragmentRecord.deferredGroupedFieldSetRecords.size > 0
114+
) {
117115
return;
118116
}
119117
const reconcilableResults = Array.from(
@@ -243,17 +241,16 @@ export class IncrementalGraph {
243241
private _onDeferredGroupedFieldSet(
244242
deferredGroupedFieldSetRecord: DeferredGroupedFieldSetRecord,
245243
): void {
246-
const deferredGroupedFieldSetResult = deferredGroupedFieldSetRecord.result;
247-
const result =
248-
deferredGroupedFieldSetResult instanceof BoxedPromiseOrValue
249-
? deferredGroupedFieldSetResult.value
250-
: deferredGroupedFieldSetResult().value;
251-
252-
if (isPromise(result)) {
244+
let deferredGroupedFieldSetResult = deferredGroupedFieldSetRecord.result;
245+
if (!(deferredGroupedFieldSetResult instanceof BoxedPromiseOrValue)) {
246+
deferredGroupedFieldSetResult = deferredGroupedFieldSetResult();
247+
}
248+
const value = deferredGroupedFieldSetResult.value;
249+
if (isPromise(value)) {
253250
// eslint-disable-next-line @typescript-eslint/no-floating-promises
254-
result.then((resolved) => this._enqueue(resolved));
251+
value.then((resolved) => this._enqueue(resolved));
255252
} else {
256-
this._enqueue(result);
253+
this._enqueue(value);
257254
}
258255
}
259256

‎src/execution/__tests__/defer-test.ts

Lines changed: 250 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
1-
import { expect } from 'chai';
1+
import { assert, expect } from 'chai';
22
import { describe, it } from 'mocha';
33

44
import { expectJSON } from '../../__testUtils__/expectJSON.js';
55
import { expectPromise } from '../../__testUtils__/expectPromise.js';
66
import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js';
77

8+
import { promiseWithResolvers } from '../../jsutils/promiseWithResolvers.js';
9+
810
import type { DocumentNode } from '../../language/ast.js';
911
import { parse } from '../../language/parser.js';
1012

@@ -856,6 +858,253 @@ describe('Execute: defer directive', () => {
856858
]);
857859
});
858860

861+
it('Initiates deferred grouped field sets only if they have been released as pending', async () => {
862+
const document = parse(`
863+
query {
864+
... @defer {
865+
a {
866+
... @defer {
867+
b {
868+
c { d }
869+
}
870+
}
871+
}
872+
}
873+
... @defer {
874+
a {
875+
someField
876+
... @defer {
877+
b {
878+
e { f }
879+
}
880+
}
881+
}
882+
}
883+
}
884+
`);
885+
886+
const { promise: slowFieldPromise, resolve: resolveSlowField } =
887+
promiseWithResolvers();
888+
let cResolverCalled = false;
889+
let eResolverCalled = false;
890+
const executeResult = experimentalExecuteIncrementally({
891+
schema,
892+
document,
893+
rootValue: {
894+
a: {
895+
someField: slowFieldPromise,
896+
b: {
897+
c: () => {
898+
cResolverCalled = true;
899+
return { d: 'd' };
900+
},
901+
e: () => {
902+
eResolverCalled = true;
903+
return { f: 'f' };
904+
},
905+
},
906+
},
907+
},
908+
enableEarlyExecution: false,
909+
});
910+
911+
assert('initialResult' in executeResult);
912+
913+
const result1 = executeResult.initialResult;
914+
expectJSON(result1).toDeepEqual({
915+
data: {},
916+
pending: [
917+
{ id: '0', path: [] },
918+
{ id: '1', path: [] },
919+
],
920+
hasNext: true,
921+
});
922+
923+
const iterator = executeResult.subsequentResults[Symbol.asyncIterator]();
924+
925+
expect(cResolverCalled).to.equal(false);
926+
expect(eResolverCalled).to.equal(false);
927+
928+
const result2 = await iterator.next();
929+
expectJSON(result2).toDeepEqual({
930+
value: {
931+
pending: [{ id: '2', path: ['a'] }],
932+
incremental: [
933+
{
934+
data: { a: {} },
935+
id: '0',
936+
},
937+
{
938+
data: { b: {} },
939+
id: '2',
940+
},
941+
{
942+
data: { c: { d: 'd' } },
943+
id: '2',
944+
subPath: ['b'],
945+
},
946+
],
947+
completed: [{ id: '0' }, { id: '2' }],
948+
hasNext: true,
949+
},
950+
done: false,
951+
});
952+
953+
expect(cResolverCalled).to.equal(true);
954+
expect(eResolverCalled).to.equal(false);
955+
956+
resolveSlowField('someField');
957+
958+
const result3 = await iterator.next();
959+
expectJSON(result3).toDeepEqual({
960+
value: {
961+
pending: [{ id: '3', path: ['a'] }],
962+
incremental: [
963+
{
964+
data: { someField: 'someField' },
965+
id: '1',
966+
subPath: ['a'],
967+
},
968+
{
969+
data: { e: { f: 'f' } },
970+
id: '3',
971+
subPath: ['b'],
972+
},
973+
],
974+
completed: [{ id: '1' }, { id: '3' }],
975+
hasNext: false,
976+
},
977+
done: false,
978+
});
979+
980+
expect(eResolverCalled).to.equal(true);
981+
982+
const result4 = await iterator.next();
983+
expectJSON(result4).toDeepEqual({
984+
value: undefined,
985+
done: true,
986+
});
987+
});
988+
989+
it('Initiates unique deferred grouped field sets after those that are common to sibling defers', async () => {
990+
const document = parse(`
991+
query {
992+
... @defer {
993+
a {
994+
... @defer {
995+
b {
996+
c { d }
997+
}
998+
}
999+
}
1000+
}
1001+
... @defer {
1002+
a {
1003+
... @defer {
1004+
b {
1005+
c { d }
1006+
e { f }
1007+
}
1008+
}
1009+
}
1010+
}
1011+
}
1012+
`);
1013+
1014+
const { promise: cPromise, resolve: resolveC } =
1015+
// eslint-disable-next-line @typescript-eslint/no-invalid-void-type
1016+
promiseWithResolvers<void>();
1017+
let cResolverCalled = false;
1018+
let eResolverCalled = false;
1019+
const executeResult = experimentalExecuteIncrementally({
1020+
schema,
1021+
document,
1022+
rootValue: {
1023+
a: {
1024+
b: {
1025+
c: async () => {
1026+
cResolverCalled = true;
1027+
await cPromise;
1028+
return { d: 'd' };
1029+
},
1030+
e: () => {
1031+
eResolverCalled = true;
1032+
return { f: 'f' };
1033+
},
1034+
},
1035+
},
1036+
},
1037+
enableEarlyExecution: false,
1038+
});
1039+
1040+
assert('initialResult' in executeResult);
1041+
1042+
const result1 = executeResult.initialResult;
1043+
expectJSON(result1).toDeepEqual({
1044+
data: {},
1045+
pending: [
1046+
{ id: '0', path: [] },
1047+
{ id: '1', path: [] },
1048+
],
1049+
hasNext: true,
1050+
});
1051+
1052+
const iterator = executeResult.subsequentResults[Symbol.asyncIterator]();
1053+
1054+
expect(cResolverCalled).to.equal(false);
1055+
expect(eResolverCalled).to.equal(false);
1056+
1057+
const result2 = await iterator.next();
1058+
expectJSON(result2).toDeepEqual({
1059+
value: {
1060+
pending: [
1061+
{ id: '2', path: ['a'] },
1062+
{ id: '3', path: ['a'] },
1063+
],
1064+
incremental: [
1065+
{
1066+
data: { a: {} },
1067+
id: '0',
1068+
},
1069+
],
1070+
completed: [{ id: '0' }, { id: '1' }],
1071+
hasNext: true,
1072+
},
1073+
done: false,
1074+
});
1075+
1076+
resolveC();
1077+
1078+
expect(cResolverCalled).to.equal(true);
1079+
expect(eResolverCalled).to.equal(false);
1080+
1081+
const result3 = await iterator.next();
1082+
expectJSON(result3).toDeepEqual({
1083+
value: {
1084+
incremental: [
1085+
{
1086+
data: { b: { c: { d: 'd' } } },
1087+
id: '2',
1088+
},
1089+
{
1090+
data: { e: { f: 'f' } },
1091+
id: '3',
1092+
subPath: ['b'],
1093+
},
1094+
],
1095+
completed: [{ id: '2' }, { id: '3' }],
1096+
hasNext: false,
1097+
},
1098+
done: false,
1099+
});
1100+
1101+
const result4 = await iterator.next();
1102+
expectJSON(result4).toDeepEqual({
1103+
value: undefined,
1104+
done: true,
1105+
});
1106+
});
1107+
8591108
it('Can deduplicate multiple defers on the same object', async () => {
8601109
const document = parse(`
8611110
query {

‎src/execution/execute.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2127,16 +2127,16 @@ function executeDeferredGroupedFieldSets(
21272127
deferMap,
21282128
);
21292129

2130-
const shouldDeferThisDeferUsageSet = shouldDefer(
2131-
parentDeferUsages,
2132-
deferUsageSet,
2133-
);
2134-
2135-
deferredGroupedFieldSetRecord.result = shouldDeferThisDeferUsageSet
2136-
? exeContext.enableEarlyExecution
2137-
? new BoxedPromiseOrValue(Promise.resolve().then(executor))
2138-
: () => new BoxedPromiseOrValue(executor())
2139-
: new BoxedPromiseOrValue(executor());
2130+
if (exeContext.enableEarlyExecution) {
2131+
deferredGroupedFieldSetRecord.result = new BoxedPromiseOrValue(
2132+
shouldDefer(parentDeferUsages, deferUsageSet)
2133+
? Promise.resolve().then(executor)
2134+
: executor(),
2135+
);
2136+
} else {
2137+
deferredGroupedFieldSetRecord.result = () =>
2138+
new BoxedPromiseOrValue(executor());
2139+
}
21402140

21412141
newDeferredGroupedFieldSetRecords.push(deferredGroupedFieldSetRecord);
21422142
}

0 commit comments

Comments
 (0)