♻️ Refactor internals, update is_coroutine check to reuse internal supported variants (unwrap, check class)
#14434
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Ok. False alarm. Sorry for bothering you - but you might be interested and maybe also chime in. Seems to be cadwyn issue that it badly wraps sync calls and the calls are recognized as coroutines zmievsa/cadwyn#309 |
|
After some debugging, this also seems to break below code without cadwyn dependency which is similar to the pattern in Airflow + cadwyn does. The below code works in 0.123.4 and breaks in 0.123.5. from functools import wraps
from fastapi import FastAPI
from pydantic import BaseModel
class SampleModel(BaseModel):
name: str
age: int
app = FastAPI()
def auth_required(func):
@wraps(func)
async def wrapper(*args, **kwargs):
return func(*args, **kwargs)
return wrapper
@app.get("/")
@auth_required # Custom decorator
def root():
return {"message": "Hello World"}0.123.5 |
|
But why |
|
@dolfinus This was during debugging the issue to remove cadwyn dependency and to use only fastapi to see if there are any issues. routes use |
And wrapping them with: async def wrapper(*args, **kwargs)
return func(*args, **kwargs)will make FastAPI run the syncronous code inside event loop, blocking it. It's only then wrapper method is |
|
I think the issue is valid. |
|
Ah cool. So that might mean our CI helped to find an issue :) .. Goood :) |
That should be done very, very carefully, to avoid blocking event loop. |
|
Thanks for the report folks! Let me take a look into it with your reproduction example (that helps a lot!). |
|
Wait, I'm actually confused... def auth_required(func):
@wraps(func)
async def wrapper(*args, **kwargs):
return func(*args, **kwargs)This means Not sure how that actually works on Cadwyn and what would be expected to be done there 🤔 I think @YuriiMotov found a way to replicate it, and still FastAPI errors out when it shouldn't, let me check with that. |
|
From my understanding cadwyn just assumes everything will be async and wraps it with async. In Airflow's case blocking operations are performed inside the routes like sqlalchemy db calls which are blocking using sync db driver and session. The routes used to be async but were made sync to be migrated later once sqlalchemy and all other operations become async. The issue seems to be that earlier the iscoroutinefunction was made on |
|
Yep, I'm here now: import inspect
import sys
from functools import wraps
from fastapi import FastAPI
from fastapi.concurrency import run_in_threadpool
from pydantic import BaseModel
if sys.version_info >= (3, 13): # pragma: no cover
from inspect import iscoroutinefunction
else: # pragma: no cover
from asyncio import iscoroutinefunction
class SampleModel(BaseModel):
name: str
age: int
app = FastAPI()
def wrap(func):
@wraps(func)
async def wrapper(*args, **kwargs):
if inspect.isroutine(func) and iscoroutinefunction(func):
return await func(*args, **kwargs)
if inspect.isclass(func):
return await run_in_threadpool(func, *args, **kwargs)
dunder_call = getattr(func, "__call__", None) # noqa: B004
if iscoroutinefunction(dunder_call):
return await dunder_call(*args, **kwargs)
return await run_in_threadpool(func, *args, **kwargs)
return wrapper
@app.get("/")
@wrap
def root():
return {"message": "Hello World"}That more or less simulates what Cadwyn would be doing, and still fails. We're on it. @YuriiMotov will help me write a set of tests with combinations of sync/async wrapper scenarios (for path operations and dependencies), and we'll continue from there. |
|
We're also experiencing problems with synchronous NiceGUI page functions (see zauberzeug/nicegui#5535). Therefore I investigated the problem and came up with this MRE: from functools import wraps
from fastapi import FastAPI
app = FastAPI()
def my_decorator(func):
@wraps(func)
async def wrapper():
func()
return 'Ok'
return wrapper
@app.get('/')
@my_decorator
def index():
print('Hello!') |
|
Maybe better move this conversation to discussion: #14442 |
|
This should be fixed by #14448 It's now released in FastAPI 0.123.6 🎉 |
♻️ Refactor internals, update
is_coroutinecheck to reuse internal supported variants (unwrap, check class)