Skip to content

Commit

Permalink
stop resolvers after execution ends
Browse files Browse the repository at this point in the history
addresses: #3792
  • Loading branch information
yaacovCR committed Oct 29, 2024
1 parent 7dea80e commit 75624cf
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 24 deletions.
63 changes: 63 additions & 0 deletions src/execution/__tests__/nonnull-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { expect } from 'chai';
import { describe, it } from 'mocha';

import { expectJSON } from '../../__testUtils__/expectJSON.js';
import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js';

import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js';

Expand Down Expand Up @@ -526,6 +527,68 @@ describe('Execute: handles non-nullable types', () => {
});
});

describe('cancellation with null bubbling', () => {
function nestedPromise(n: number): string {
return n > 0 ? `promiseNest { ${nestedPromise(n - 1)} }` : 'promise';
}

it('returns an single error without cancellation', async () => {
const query = `
{
promiseNonNull,
${nestedPromise(4)}
}
`;

const result = await executeQuery(query, throwingData);
expectJSON(result).toDeepEqual({
data: null,
errors: [
// does not include syncNullError because result returns prior to it being added
{
message: 'promiseNonNull',
path: ['promiseNonNull'],
locations: [{ line: 3, column: 11 }],
},
],
});
});

it('stops running despite error', async () => {
const query = `
{
promiseNonNull,
${nestedPromise(10)}
}
`;

let counter = 0;
const rootValue = {
...throwingData,
promiseNest() {
return new Promise((resolve) => {
counter++;
resolve(rootValue);
});
},
};
const result = await executeQuery(query, rootValue);
expectJSON(result).toDeepEqual({
data: null,
errors: [
{
message: 'promiseNonNull',
path: ['promiseNonNull'],
locations: [{ line: 3, column: 11 }],
},
],
});
const counterAtExecutionEnd = counter;
await resolveOnNextTick();
expect(counter).to.equal(counterAtExecutionEnd);
});
});

describe('Handles non-null argument', () => {
const schemaWithNonNullArg = new GraphQLSchema({
query: new GraphQLObjectType({
Expand Down
85 changes: 61 additions & 24 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,12 @@ export interface ValidatedExecutionArgs {

export interface ExecutionContext {
validatedExecutionArgs: ValidatedExecutionArgs;
completed: boolean;
cancellableStreams: Set<CancellableStreamRecord> | undefined;
}

interface IncrementalContext {
completed: boolean;
deferUsageSet?: DeferUsageSet | undefined;
}

Expand Down Expand Up @@ -312,6 +314,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(
): PromiseOrValue<ExecutionResult | ExperimentalIncrementalExecutionResults> {
const exeContext: ExecutionContext = {
validatedExecutionArgs,
completed: false,
cancellableStreams: undefined,
};
try {
Expand Down Expand Up @@ -362,15 +365,23 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(

if (isPromise(graphqlWrappedResult)) {
return graphqlWrappedResult.then(
(resolved) => buildDataResponse(exeContext, resolved),
(error: unknown) => ({
data: null,
errors: [error as GraphQLError],
}),
(resolved) => {
exeContext.completed = true;
return buildDataResponse(exeContext, resolved);
},
(error: unknown) => {
exeContext.completed = true;
return {
data: null,
errors: [error as GraphQLError],
};
},
);
}
exeContext.completed = true;
return buildDataResponse(exeContext, graphqlWrappedResult);
} catch (error) {
exeContext.completed = true;
return { data: null, errors: [error] };
}
}
Expand Down Expand Up @@ -818,7 +829,7 @@ function executeField(
validatedExecutionArgs;
const fieldName = fieldDetailsList[0].node.name.value;
const fieldDef = schema.getField(parentType, fieldName);
if (!fieldDef) {
if (!fieldDef || (incrementalContext ?? exeContext).completed) {
return;
}

Expand Down Expand Up @@ -2279,6 +2290,7 @@ function collectExecutionGroups(
path,
groupedFieldSet,
{
completed: false,
deferUsageSet,
},
deferMap,
Expand Down Expand Up @@ -2338,6 +2350,7 @@ function executeExecutionGroup(
deferMap,
);
} catch (error) {
incrementalContext.completed = true;
return {
pendingExecutionGroup,
path: pathToArray(path),
Expand All @@ -2347,16 +2360,26 @@ function executeExecutionGroup(

if (isPromise(result)) {
return result.then(
(resolved) =>
buildCompletedExecutionGroup(pendingExecutionGroup, path, resolved),
(error: unknown) => ({
pendingExecutionGroup,
path: pathToArray(path),
errors: [error as GraphQLError],
}),
(resolved) => {
incrementalContext.completed = true;
return buildCompletedExecutionGroup(
pendingExecutionGroup,
path,
resolved,
);
},
(error: unknown) => {
incrementalContext.completed = true;
return {
pendingExecutionGroup,
path: pathToArray(path),
errors: [error as GraphQLError],
};
},
);
}

incrementalContext.completed = true;
return buildCompletedExecutionGroup(pendingExecutionGroup, path, result);
}

Expand Down Expand Up @@ -2405,7 +2428,7 @@ function buildSyncStreamItemQueue(
initialPath,
initialItem,
exeContext,
{},
{ completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2436,7 +2459,7 @@ function buildSyncStreamItemQueue(
itemPath,
value,
exeContext,
{},
{ completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2528,7 +2551,7 @@ async function getNextAsyncStreamItemResult(
itemPath,
iteration.value,
exeContext,
{},
{ completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2575,10 +2598,16 @@ function completeStreamItem(
incrementalContext,
new Map(),
).then(
(resolvedItem) => buildStreamItemResult(resolvedItem),
(error: unknown) => ({
errors: [error as GraphQLError],
}),
(resolvedItem) => {
incrementalContext.completed = true;
return buildStreamItemResult(resolvedItem);
},
(error: unknown) => {
incrementalContext.completed = true;
return {
errors: [error as GraphQLError],
};
},
);
}

Expand All @@ -2605,6 +2634,7 @@ function completeStreamItem(
};
}
} catch (error) {
incrementalContext.completed = true;
return {
errors: [error],
};
Expand All @@ -2620,13 +2650,20 @@ function completeStreamItem(
],
}))
.then(
(resolvedItem) => buildStreamItemResult(resolvedItem),
(error: unknown) => ({
errors: [error as GraphQLError],
}),
(resolvedItem) => {
incrementalContext.completed = true;
return buildStreamItemResult(resolvedItem);
},
(error: unknown) => {
incrementalContext.completed = true;
return {
errors: [error as GraphQLError],
};
},
);
}

incrementalContext.completed = true;
return buildStreamItemResult(result);
}

Expand Down

0 comments on commit 75624cf

Please sign in to comment.