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
yaacovCR committed Oct 30, 2024
1 parent 9f634a8 commit 3ab05ef
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 22 deletions.
63 changes: 63 additions & 0 deletions src/execution/__tests__/nonnull-test.ts
View file
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
77 changes: 55 additions & 22 deletions src/execution/execute.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,13 @@ export interface ExecutionContext {
validatedExecutionArgs: ValidatedExecutionArgs;
errors: Array<GraphQLError> | undefined;
canceller: Canceller | undefined;
completed: boolean;
cancellableStreams: Set<CancellableStreamRecord> | undefined;
}

interface IncrementalContext {
errors: Array<GraphQLError> | undefined;
completed: boolean;
deferUsageSet?: DeferUsageSet | undefined;
}

Expand Down Expand Up @@ -317,6 +319,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(
validatedExecutionArgs,
errors: undefined,
canceller: abortSignal ? new Canceller(abortSignal) : undefined,
completed: false,
cancellableStreams: undefined,
};
try {
Expand Down Expand Up @@ -367,8 +370,12 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(

if (isPromise(graphqlWrappedResult)) {
return graphqlWrappedResult.then(
(resolved) => buildDataResponse(exeContext, resolved),
(resolved) => {
exeContext.completed = true;
return buildDataResponse(exeContext, resolved);
},
(error: unknown) => {
exeContext.completed = true;
exeContext.canceller?.unsubscribe();
return {
data: null,
Expand All @@ -377,8 +384,10 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(
},
);
}
exeContext.completed = true;
return buildDataResponse(exeContext, graphqlWrappedResult);
} catch (error) {
exeContext.completed = true;
// TODO: add test case for synchronous null bubbling to root with cancellation
/* c8 ignore next */
exeContext.canceller?.unsubscribe();
Expand Down Expand Up @@ -1757,6 +1766,10 @@ function completeObjectValue(
incrementalContext: IncrementalContext | undefined,
deferMap: ReadonlyMap<DeferUsage, DeferredFragmentRecord> | undefined,
): PromiseOrValue<GraphQLWrappedResult<ObjMap<unknown>>> {
if ((incrementalContext ?? exeContext).completed) {
throw new Error('Completed, aborting.');
}

// If there is an isTypeOf predicate function, call it with the
// current result. If isTypeOf returns false, then raise an error rather
// than continuing execution.
Expand Down Expand Up @@ -2274,6 +2287,7 @@ function collectExecutionGroups(
groupedFieldSet,
{
errors: undefined,
completed: false,
deferUsageSet,
},
deferMap,
Expand Down Expand Up @@ -2333,6 +2347,7 @@ function executeExecutionGroup(
deferMap,
);
} catch (error) {
incrementalContext.completed = true;
return {
pendingExecutionGroup,
path: pathToArray(path),
Expand All @@ -2342,21 +2357,27 @@ function executeExecutionGroup(

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

incrementalContext.completed = true;
return buildCompletedExecutionGroup(
incrementalContext.errors,
pendingExecutionGroup,
Expand Down Expand Up @@ -2411,7 +2432,7 @@ function buildSyncStreamItemQueue(
initialPath,
initialItem,
exeContext,
{ errors: undefined },
{ errors: undefined, completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2442,7 +2463,7 @@ function buildSyncStreamItemQueue(
itemPath,
value,
exeContext,
{ errors: undefined },
{ errors: undefined, completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2534,7 +2555,7 @@ async function getNextAsyncStreamItemResult(
itemPath,
iteration.value,
exeContext,
{ errors: undefined },
{ errors: undefined, completed: false },
fieldDetailsList,
info,
itemType,
Expand Down Expand Up @@ -2581,11 +2602,16 @@ function completeStreamItem(
incrementalContext,
new Map(),
).then(
(resolvedItem) =>
buildStreamItemResult(incrementalContext.errors, resolvedItem),
(error: unknown) => ({
errors: withError(incrementalContext.errors, error as GraphQLError),
}),
(resolvedItem) => {
incrementalContext.completed = true;
return buildStreamItemResult(incrementalContext.errors, resolvedItem);
},
(error: unknown) => {
incrementalContext.completed = true;
return {
errors: withError(incrementalContext.errors, error as GraphQLError),
};
},
);
}

Expand Down Expand Up @@ -2614,6 +2640,7 @@ function completeStreamItem(
result = { rawResult: null, incrementalDataRecords: undefined };
}
} catch (error) {
incrementalContext.completed = true;
return {
errors: withError(incrementalContext.errors, error),
};
Expand All @@ -2633,14 +2660,20 @@ function completeStreamItem(
return { rawResult: null, incrementalDataRecords: undefined };
})
.then(
(resolvedItem) =>
buildStreamItemResult(incrementalContext.errors, resolvedItem),
(error: unknown) => ({
errors: withError(incrementalContext.errors, error as GraphQLError),
}),
(resolvedItem) => {
incrementalContext.completed = true;
return buildStreamItemResult(incrementalContext.errors, resolvedItem);
},
(error: unknown) => {
incrementalContext.completed = true;
return {
errors: withError(incrementalContext.errors, error as GraphQLError),
};
},
);
}

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

Expand Down

0 comments on commit 3ab05ef

Please sign in to comment.