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 3455728
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 23 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
91 changes: 68 additions & 23 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 @@ -1019,6 +1030,14 @@ function completeValue(
incrementalContext: IncrementalContext | undefined,
deferMap: ReadonlyMap<DeferUsage, DeferredFragmentRecord> | undefined,
): PromiseOrValue<GraphQLWrappedResult<unknown>> {
if ((incrementalContext ?? exeContext).completed) {
return {
rawResult: null,
incrementalDataRecords: undefined,
errors: undefined,
};
}

// If result is an Error, throw a located error.
if (result instanceof Error) {
throw result;
Expand Down Expand Up @@ -2279,6 +2298,7 @@ function collectExecutionGroups(
path,
groupedFieldSet,
{
completed: false,
deferUsageSet,
},
deferMap,
Expand Down Expand Up @@ -2338,6 +2358,7 @@ function executeExecutionGroup(
deferMap,
);
} catch (error) {
incrementalContext.completed = true;
return {
pendingExecutionGroup,
path: pathToArray(path),
Expand All @@ -2347,16 +2368,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 +2436,7 @@ function buildSyncStreamItemQueue(
initialPath,
initialItem,
exeContext,
{},
{ completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2436,7 +2467,7 @@ function buildSyncStreamItemQueue(
itemPath,
value,
exeContext,
{},
{ completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2528,7 +2559,7 @@ async function getNextAsyncStreamItemResult(
itemPath,
iteration.value,
exeContext,
{},
{ completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2575,10 +2606,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 +2642,7 @@ function completeStreamItem(
};
}
} catch (error) {
incrementalContext.completed = true;
return {
errors: [error],
};
Expand All @@ -2620,13 +2658,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 3455728

Please sign in to comment.