objects defining async __call__ not run/awaited when used in on_startup

[cjw296]

Simple reproducer:

def test_async_startup_hangs():

    class Foo:
        async def __call__(self):
            raise Exception()

    app = Starlette(on_startup=[Foo()])

    with TestClient(app) as client:
        pass

No exception is raised when it should be. This, however, raises an exception as expected:

def test_async_startup_hangs():

    async def foo():
        raise Exception()

    app = Starlette(on_startup=[foo])

    with TestClient(app) as client:
        pass

[euri10]

not sure about that but seems like on_startup expects a list of callables,
app = Starlette(on_startup=[Foo().__call__]) will trigger your exception

[cjw296]

An instance of Foo() is callable:

$ python
Python 3.7.2 (default, Dec 29 2018, 00:00:04) 
[Clang 4.0.1 (tags/RELEASE_401/final)] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo():
...     async def __call__(self):
...         pass
... 
>>> obj = Foo()
>>> callable(obj)
True

Explicitly specifying __call__ as you have feels like a workaround for this bug rather than a solution.

[florimondmanca]

@cjw296 An immediate workaround: wrap your callable object into a function?

class Foo:
    async def __call__(self):
        raise Exception()

foo = Foo()

async def on_startup():
    await foo()

 app = Starlette(on_startup=[on_startup])
with TestClient(app) as client:  # Raises `Exception`.
        pass

The long-term solution would be to fix the detection of async functions in the lifespan logic to something like:

# In an utils module
def iscoroutinefunction(obj):
    if inspect.iscoroutinefunction(obj):
        return True
    if hasattr(obj, '__call__') and inspect.iscoroutinefunction(obj.__call__):
        return True
    return False

# routing.py
if iscoroutinefunction(func):
    await func()
else:
    ... # Sync, defer to threadpool.

Not sure where that's in the codebase exactly, but it should do the trick.

[cjw296]

Yeah, agreed on both fronts, but this almost feels like a deficiency in python itself; it feels like there should be an inspect.returnscoroutinewhencalled to catch these cases, rather than assuming everything is a function or a method.

[florimondmanca]

Sure, some even wish async callable would have been defined as objects that have an async __acall__ method. :-) (I remember seeing Andrew Godwins mention this when he was figuring out how to tackle the async madness for the Django Async roadmap.)

[cjw296]

Turns out this issue also affects partial:

https://stackoverflow.com/questions/52422860/partial-asynchronous-functions-are-not-detected-as-asynchronous

@tomchristie suggested Starlette should grow its own function to make these checks, once that's happened, we should see about pushing it back into CPython core.

[abersheeran]

@cjw296 Maybe you can look at this function, it has enough judgment. https://github.com/abersheeran/index.py/blob/master/indexpy/concurrency.py#L9

[tomchristie]

TBH I'd be quite keen to moving Starlette towards always expecting an async style on endpoints, startup/shutdown functions, error handlers etc.

Users could still use sync style functions, but we'd require them to be explicitly wrapped in a threadpooling sync -> async decorator.

[cjw296]

Always async in Starlette make sense. Handling this in apps built on top of Starlette is probably the right place, letting Starlette focus on being async, and potentially more helpfully, throwing exceptions when it gets sync stuff by mistake.

@abersheeran - with that in mind, here's the function I currently have:
https://github.com/cjw296/mush/blob/3c85fd9284413ee3ee9987c1997b70819d44bad0/mush/asyncio.py#L43-L70

[retnikt]

@tomchristie +1 on that, Starlette is explicitly an async framework, and allowing sync functions seems wrong. We could add a decorator or something that uses starlette.concurrency.run_in_threadpool, but it seems the whole sync-vs-async discrepancy is a bit unneccessary.

[dpr-0]

Hi all,

I am facing same issues when using BackgroundTasks object for my awaitable and callable object, so is there any enhancement for this case detection? Like the solution from @florimondmanca.

    if hasattr(obj, '__call__') and inspect.iscoroutinefunction(obj.__call__):
        return True

I am willing to enhance this in BackgroundTasks .

[adriangb]

TBH I'd be quite keen to moving Starlette towards always expecting an async style on endpoints, startup/shutdown functions, error handlers etc.

Users could still use sync style functions, but we'd require them to be explicitly wrapped in a threadpooling sync -> async decorator.

@tomchristie wondering how you feel about this nowadays? I this moving to an async only or at least more explicit paradigm could also help with rough edges like #1258