`RuntimeWarning: coroutine ... never awaited` when a sibling resolver raises an error

[magicmark]

Repro

import asyncio
from graphql import (
    graphql,
    GraphQLSchema,
    GraphQLObjectType,
    GraphQLField,
    GraphQLNonNull,
    GraphQLString,
)


async def resolve_foo(obj, info):
    return "foo!"


def resolve_bar(obj, info):
    raise Exception('yikes')


schema = GraphQLSchema(
    query=GraphQLObjectType(
        name="RootQueryType",
        fields={
            "foo": GraphQLField(GraphQLNonNull(GraphQLString), resolve=resolve_foo),
            "bar": GraphQLField(GraphQLNonNull(GraphQLString), resolve=resolve_bar),
        },
    )
)

result = asyncio.run(graphql(schema, "{ foo bar }"))
print(result)

Prints

$ python3 test2.py
ExecutionResult(data=None, errors=[GraphQLError('yikes', locations=[SourceLocation(line=1, column=7)], path=['bar'])])
sys:1: RuntimeWarning: coroutine 'ExecutionContext.execute_field.<locals>.await_result' was never awaited
sys:1: RuntimeWarning: coroutine 'resolve_foo' was never awaited

Version info

$ pip freeze | grep graphql-core
graphql-core==3.2.6

I'm guessing what could be happening here is that:

• non-nullable field error (bar) bubbles up and cancels execution for the rest of the type's fields
• ...but we already kicked off the foo async resolver function
• ...but by aborting, we don't ever unwrap the future / await the result for, hence the warning.

Which makes total sense -- and indeed if we do schema.execute("{ bar foo }") we don't see the warning printed -- because the async function never even gets kicked off.

I think maybe we need to be calling task.cancel() or .close() somewhere?

• https://docs.python.org/3/library/asyncio-task.html#asyncio.Task.cancel
• https://docs.python.org/3/reference/datamodel.html#coroutine.close

(originally misatttributed to strawberry strawberry-graphql/strawberry#3820)

Thanks!

[magicmark]

I'm having trouble submitting writing a test for this, i can't seem to catch it with

with warnings.catch_warnings(record=True) as w:

attempt

   @pytest.mark.asyncio
    async def cancelled_sibling_async_resolvers_do_not_print_warning():
        async def async_resolver(obj, info):
            return 'foo'
    
        schema = GraphQLSchema(
            GraphQLObjectType(
                "Query",
                {
                    "syncNullError": GraphQLField(
                        GraphQLNonNull(GraphQLString), resolve=lambda _obj, _info: None
                    ),
                    "asyncResolver": GraphQLField(
                        GraphQLNonNull(GraphQLString), resolve=async_resolver
                    ),
                },
            )
        )

        document = parse(
            """
            {
              asyncResolver
              syncNullError
            }
            """
        )

        with warnings.catch_warnings(record=True) as w:
            warnings.simplefilter("always")
            result = await execute(schema, document)
            print(w)
            breakpoint()
       

fwiw i did hack together a "fix" though which removes the warnings, which is what i was hoping to eventually incorporate: https://i.fluffy.cc/c7fMHMcxqKfd4BKx7JrZSMQ9SMW4mQwQ.html#L28-31,L118-122 (adds an abort callback to .close out the awaitables)

when/if i get more time soon i'll pick this back up. thanks!

[Cito]

I'll look into it as soon as I find some time. Can you check whether the problem still exists in v3.3.0a7?

[magicmark]

@Cito looks like it, yes.

╔═ markl@GQXP9G4HJM: ~/Downloads/graphql-core-3.3.0a7  via 🐍 v3.13.2 
╚═ ♪ cat pyproject.toml | head -n3
[tool.poetry]
name = "graphql-core"
version = "3.3.0a7"

╔═ markl@GQXP9G4HJM: ~/Downloads/graphql-core-3.3.0a7  via 🐍 v3.13.2 
╚═ ♪ poetry run python repro.py       
ExecutionResult(data=None, errors=[GraphQLError('yikes', locations=[SourceLocation(line=1, column=7)], path=['bar'])])
<sys>:0: RuntimeWarning: coroutine 'ExecutionContext.complete_awaitable_value' was never awaited
<sys>:0: RuntimeWarning: coroutine 'resolve_foo' was never awaited

thanks!

[magicmark]

graphql-core/tests/execution/test_executor.py

Line 525 in 416247c

def handles_sync_errors_combined_with_async_ones():

is a test that expects this behaviour 🤔

[erikwrede]

@magicmark I'd say this test is supposed to replicate the following spec wording for sync + async: https://spec.graphql.org/October2021/#sel-GANPLFABABqEozF
How would your change affect that behavior? Does the cancel in this case cause the task to be executed regardless?

For Py > 3.11, a TaskGroup could help: https://docs.python.org/3/library/asyncio-task.html#asyncio.TaskGroup

[magicmark]

Thanks @erikwrede! Looks like my proposed change is spec compliant.

If this occurs, any sibling fields which have not yet executed or have not yet yielded a value may be cancelled to avoid unnecessary work.

may be cancelled to avoid unnecessary work. <-- we currently do not cancel the coroutine, hence the warning. Explicitly calling .close() would. To my eyes this would be permissible per the spec wording.

TaskGroup could also work! Will look into if that (and see if we're bound by version requirements/polyfils are available)

thanks!

[mgorven]

This is causing a problem for us where the zombie resolvers continue using the SQLAlchemy session after it has been closed, resulting in leaked database connections. I've submitted #238 with a possible solution to this.