Improve detection of async callables

[Kludex]

• Closes objects defining async __call__ not run/awaited when used in on_startup #886
• Related to Fixed - async __call__ not run/awaited in the BackgroundTask #1256

References:

• Static Files Error With The Latest Version (3.3.2) django/asgiref#251
• https://github.com/django/asgiref/blob/3554e05def93e086690d19e9f38e593f1b5b7b61/asgiref/sync.py#L28-L40
• https://gitlab.com/pgjones/quart/-/blob/main/src/quart/utils.py#L102-128

[bibajz]

Nice!

Quick check with grep reveals that asyncio is now mentioned only in starlette/_utils and tests ;)

[bibajz]

Hi @Kludex, would it make sense to use inspect.iscoroutinefunction here?

I understand why we cannot just use the said function alone, since async def __call__(...) is not captured by it:

>>> import inspect
>>> class A:
...     async def __call__(self):
...             ...
... 
>>> inspect.iscoroutinefunction(A())
False

I have looked at the code of asyncio.iscoroutinefunction and it only additionally check for the deprecated @coroutine decorator.

Cheers,
Libor

[bibajz]

By the way, the unwrapping pattern of while isinstance(obj, functools.partial), I have run a grep on the main branch and

$ rg 'isinstance\(\w+, (partial|functools.partia)l\)'
starlette/routing.py
46:    while isinstance(obj, functools.partial):
202:        while isinstance(endpoint_handler, functools.partial):
283:        while isinstance(endpoint_handler, functools.partial):

Of the cases found, the one relevant here is line 46 in 'iscoroutinefunction_or_partial' function.

Does it make sense to use your newly defined function instead of it?

Edit: Looking at it further, wouldn't the replacement of iscoroutinefunction_or_partial by your checker actually solve this issue #734 ? Especially this one: #734 (comment)

^ @Kludex

[Kludex]

Edit: Looking at it further, wouldn't the replacement of iscoroutinefunction_or_partial by your checker actually solve this issue #734 ? Especially this one: #734 (comment)

Doesn't look like 😞

Of the cases found, the one relevant here is line 46 in 'iscoroutinefunction_or_partial' function.
Does it make sense to use your newly defined function instead of it?

One of the goals for this PR was to remove/deprecate that, and I forgot! 😆
Thanks!

[Kludex]

Edit: Looking at it further, wouldn't the replacement of iscoroutinefunction_or_partial by your checker actually solve this issue #734 ? Especially this one: #734 (comment)

Doesn't look like disappointed

Hmmm.. Now I understand why ismethod is included on asgiref.

It looks like it solves that comment, yes. But I'll have to do something like:

def find_function(obj: typing.Any) -> typing.Callable:
    while inspect.ismethod(obj):
        obj = obj.__func__

    while isinstance(obj, functools.partial):
        obj = obj.func

    return obj


def iscoroutinefunction(obj: typing.Any) -> bool:
    obj = find_function(obj)

    return asyncio.iscoroutinefunction(obj) or (
        callable(obj) and asyncio.iscoroutinefunction(obj.__call__)
    )

And then use the find_function on the WebSocketRoute and Route.

[bibajz]

And not even like this? I have reproduced the behaviour from your branch and it works -

as in, /a prints A and /b prints B in browser

from starlette import applications, responses, routing                                                                                                                                        
    
    
class Ping:    
    def __init__(self, message):    
        self._message = message    
    
    async def __call__(self, scope, request, response):    
        plain_text_resp = responses.PlainTextResponse(self._message)    
        return await plain_text_resp(scope, request, response)    
      
      
app = applications.Starlette(      
    routes=[      
        routing.Route("/a", Ping("A"), methods=["GET"]),      
        routing.Route("/b", Ping("B"), methods=["GET"]),      
    ]      
) 

Or am I mixing different concepts here?

[Kludex]

And not even like this? I have reproduced the behaviour from your branch and it works -

as in, /a prints A and /b prints B in browser

from starlette import applications, responses, routing                                                                                                                                        
    
    
class Ping:    
    def __init__(self, message):    
        self._message = message    
    
    async def __call__(self, scope, request, response):    
        plain_text_resp = responses.PlainTextResponse(self._message)    
        return await plain_text_resp(scope, request, response)    
      
      
app = applications.Starlette(      
    routes=[      
        routing.Route("/a", Ping("A"), methods=["GET"]),      
        routing.Route("/b", Ping("B"), methods=["GET"]),      
    ]      
) 

Or am I mixing different concepts here?

Yeah, you're mixing things. That's not the same as the example provided on the comment you mentioned above. Your example (on the quoted of this message) should never be supported...

[bibajz]

Ad #1444 (comment) : Yeah, I figured. xD

Anyway, as you pointed to asgiref - the correct way is then

1. unmethod
2. unpartial
3. iscoroutinefunction is that so?

[Kludex]

Actually, I'm going to let the state of this PR as is.

My previous statement about the ismethod was wrong.

[Kludex]

Thanks for the review @bibajz ! :)

[bibajz]

Ad #1444 (comment)

Yeah, it looks like distinguishing coroutine/normal functions is pretty tricky, so good idea to leave for another day.

Glad to help, I really dig Starlette and other stuff from encode ecosystem, so if you can point me to some issue that would be worth to look at here, let me know!

Cheers
Libor

[Kludex]

We need to organize ourselves a little better. It's hard for me to tell you where the help is needed... You can check the issues and PRs and see if you can help on them. I'm available on our Gitter if you need something.

[bibajz]

Thanks, I will look around and pick something. :)

[tomchristie]

What's the simplest possible example of an issue that this resolves from a user perspective?

[frerikandriessen]

@tomchristie I ran into this issue while working on a custom route_handler in fastapi. I'm guessing the exact specifics of what and why are not that interesting to you, but this fix would actually be used (at least by me). I could of course solve it by not using a class, or wrapping the class in a async function, but I think in the spirit of duck-typing, an async callable class instance should be handled the same way as the normal async functions.

[florimondmanca]

Coming from reviewing #1644

This is a nit, but should we call this is_async_callable?

AFAICT, having "something we can call and await" is all we care about when checking for apps or endpoints.

"Coroutine function" is a well-defined word in the Python glossary: a "function which returns a coroutine object", defined with "async def".

https://docs.python.org/3/glossary.html#term-coroutine-function

Also, the current naming makes it look as a compatibility shim on asyncio.iscoroutinefunction which focuses on checking just the definition above^, whereas we do some more checks, such as looking for __call__.

[Kludex]

This is a nit, but should we call this is_async_callable?

Sure. 👍 I'll adapt later today. Thanks for the review. 🙏

[Kludex]

What's the simplest possible example of an issue that this resolves from a user perspective?

As a user, I'll be able to create an endpoint with async callable objects, async callable methods, and functool.partial:

class Foo:
    async def __call__(self, request):
        return Response(...)
        
class Foo:
    async def method(self, request):
        return Response(...)

This also includes creating startup/shutdown events, background tasks and exception handlers as such. See more on the test.

As a user, I don't see any reason for the framework to restrict my choices here.

[abersheeran]

Good job!

[Kludex]

Thanks everybody! 🎉