Skip to content

Cancel remaining fields on exceptions #238

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 26 additions & 7 deletions src/graphql/execution/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,16 @@

from __future__ import annotations

from asyncio import ensure_future, gather, shield, wait_for
from asyncio import (
FIRST_EXCEPTION,
CancelledError,
create_task,
ensure_future,
gather,
shield,
wait,
wait_for,
)
from contextlib import suppress
from copy import copy
from typing import (
Expand Down Expand Up @@ -459,12 +468,18 @@ async def get_results() -> dict[str, Any]:
field = awaitable_fields[0]
results[field] = await results[field]
else:
results.update(
zip(
awaitable_fields,
await gather(*(results[field] for field in awaitable_fields)),
)
)
tasks = {}
for field in awaitable_fields:
tasks[create_task(results[field])] = field # type: ignore[arg-type]

done, pending = await wait(tasks, return_when=FIRST_EXCEPTION)
Copy link
Author

Choose a reason for hiding this comment

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

The other approach is to allow all fields to finish executing and then raise the exception.

if pending:
for task in pending:
task.cancel()
await wait(pending)

results.update((tasks[task], task.result()) for task in done)
Copy link
Author

Choose a reason for hiding this comment

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

This maintains the previous behaviour of not setting any results if an exception is raised, but I could change this to set the results we do have and then raise the exception.

Choose a reason for hiding this comment

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

Does this behave in-line with the spec regarding partial results?

https://spec.graphql.org/draft/#sec-Handling-Execution-Errors


return results

return get_results()
Expand Down Expand Up @@ -538,6 +553,10 @@ async def await_completed() -> Any:
try:
return await completed
except Exception as raw_error:
# Before Python 3.8 CancelledError inherits Exception and
# so gets caught here.
if isinstance(raw_error, CancelledError):
raise
self.handle_field_error(
raw_error,
return_type,
Expand Down
38 changes: 38 additions & 0 deletions tests/execution/test_parallel.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
GraphQLInt,
GraphQLInterfaceType,
GraphQLList,
GraphQLNonNull,
GraphQLObjectType,
GraphQLSchema,
GraphQLString,
Expand Down Expand Up @@ -193,3 +194,40 @@ async def is_type_of_baz(obj, *_args):
{"foo": [{"foo": "bar", "foobar": 1}, {"foo": "baz", "foobaz": 2}]},
None,
)

@pytest.mark.asyncio
async def cancel_on_exception():
barrier = Barrier(2)
completed = False

async def succeed(*_args):
nonlocal completed
await barrier.wait()
completed = True

async def fail(*_args):
raise Exception

schema = GraphQLSchema(
GraphQLObjectType(
"Query",
{
"foo": GraphQLField(GraphQLNonNull(GraphQLBoolean), resolve=fail),
"bar": GraphQLField(GraphQLBoolean, resolve=succeed),
},
)
)

ast = parse("{foo, bar}")

awaitable_result = execute(schema, ast)
assert isinstance(awaitable_result, Awaitable)
result = await asyncio.wait_for(awaitable_result, 1.0)

assert result.errors
assert not result.data

# Unblock succeed() and check that it does not complete
await barrier.wait()
await asyncio.sleep(0)
assert not completed