objects defining async __call__ not run/awaited when used in on_startup
cjw296 opened this issue ยท 12 comments
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
not sure about that but seems like on_startup expects a list of callables,
app = Starlette(on_startup=[Foo().__call__])
will trigger your exception
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.
@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.
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.
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.)
Turns out this issue also affects partial:
@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.
@cjw296 Maybe you can look at this function, it has enough judgment. https://github.com/abersheeran/index.py/blob/master/indexpy/concurrency.py#L9
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.
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
@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.
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 .
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