Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problems with await in async dependencies (when using dependencies and middleware simultaneously) #4719

Closed
9 tasks done
Rashid567 opened this issue Mar 24, 2022 · 35 comments
Closed
9 tasks done

Comments

@Rashid567
Copy link

First Check

  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the FastAPI documentation, with the integrated search.
  • I already searched in Google "How to X in FastAPI" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to FastAPI but to Pydantic.
  • I already checked if it is not related to FastAPI but to Swagger UI.
  • I already checked if it is not related to FastAPI but to ReDoc.

Commit to Help

  • I commit to help with one of those options 👆

Example Code

import fastapi
import datetime as dt
import sqlalchemy as sa

from sqlalchemy.ext.asyncio import AsyncEngine, AsyncSession
from sqlalchemy.ext.asyncio import create_async_engine
from sqlalchemy.orm import sessionmaker


class _Conn:
    radius_engine: AsyncEngine
    radius_session_maker: sessionmaker


def init_pg_connections() -> None:
    _Conn.radius_engine = create_async_engine(
        'postgresql+asyncpg://postgres:password@host:port/db_name',
        connect_args={
            'server_settings': {
                'application_name': 'test_app'
            }
        },
        pool_size=2, max_overflow=10
    )
    _Conn.radius_session_maker = sessionmaker(
        bind=_Conn.radius_engine,
        autoflush=False, autocommit=False,
        expire_on_commit=False,
        class_=AsyncSession
    )


async def get_db():
    db: AsyncSession = _Conn.radius_session_maker()
    try:
        yield db
    finally:
        print('finally get_db. Before close')
        await db.close()
        print('finally get_db. After close')


def get_app():
    app = fastapi.FastAPI(
        on_startup=[init_pg_connections]
    )

    @app.middleware('http')
    async def add_process_time_header(request: fastapi.Request, call_next):
        start_time = dt.datetime.now()
        response = await call_next(request)
        process_time = (dt.datetime.now() - start_time).total_seconds()
        response.headers["X-Process-Time"] = str(process_time)
        return response

    @app.post('/test')
    async def test(
            data: int = fastapi.Body(5, embed=True),
            db: AsyncSession = fastapi.Depends(get_db)
    ):
        res = (await db.execute(sa.text('Select * FROM radacct'))).scalars().all()
        print('test', f'{data=} {res=}')
        return 'OK'
    return app


if __name__ == '__main__':
    import uvicorn
    _app = get_app()
    uvicorn.run(_app, host='10.10.10.98', port=7776)

Description

FastAPI version>=0.74 has a very strange problem with dependency (func get_db in example).

  1. If you run this code as it is and send request from swagger every thing is ok (Connection with db will close and you see message 'finally get_db. After close' in console).

  2. If you run this code as it is and use curl or aiohttp to send request, then "await db.close()" is never finish and you will not see message 'finally get_db. After close' in console. And if you continue to send requests, then you get warnings from Garbage collector and SQLAlchemy:

The garbage collector is trying to clean up connection <AdaptedConnection <asyncpg.connection.Connection object at 0x7fd3626a4740>>. This feature is unsupported on async dbapi, since no IO can be performed at this stage to reset the connection. Please close out all connections when they are no longer used, calling "close()" or using a context manager to manage their lifetime.

/usr/lib/python3.10/ipaddress.py:45: SAWarning: The garbage collector is trying to clean up connection <AdaptedConnection <asyncpg.connection.Connection object at 0x7fd3626a4740>>. This feature is unsupported on async dbapi, since no IO can be performed at this stage to reset the connection. Please close out all connections when they are no longer used, calling "close()" or using a context manager to manage their lifetime.

return IPv4Address(address)

  1. If you comment middleware and send requests from any client (swagger, curl and aiohttp), then every thing is ok.

  2. If you run this code as it is with FastAPI version <= 0.73, then every thing is ok.

In each cases clients has no problems in getting 200 responses from server.
Unfortunately i have no idea why this happening.

Requirements:
anyio==3.5.0
asgiref==3.5.0
asyncpg==0.25.0
click==8.0.4
coloredlogs==15.0.1
fastapi==0.75.0 | 0.73.0
greenlet==1.1.2
h11==0.13.0
httptools==0.3.0
humanfriendly==10.0
idna==3.3
mypy==0.931
mypy-extensions==0.4.3
pydantic==1.9.0
python-dateutil==2.8.2
python-dotenv==0.19.2
PyYAML==6.0
six==1.16.0
sniffio==1.2.0
SQLAlchemy==1.4.32
sqlalchemy2-stubs==0.0.2a20
starlette==0.17.1
tomli==2.0.1
typing_extensions==4.1.1
uvicorn==0.17.5
uvloop==0.16.0
watchgod==0.7
websockets==10.2

Operating System

Linux

Operating System Details

No response

FastAPI Version

=0.74

Python Version

3.9 and 3.10

Additional Context

No response

@Rashid567 Rashid567 added the question Question or problem label Mar 24, 2022
@lesnik512
Copy link

Yes, having the same problem with get_db dependency not closing connection and the same error message. And it started with 0.74 release https://github.com/tiangolo/fastapi/releases/tag/0.74.0

from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
from sqlalchemy.orm import sessionmaker

from app.config import settings


engine = create_async_engine(
    settings.DB_DSN,
    echo=settings.DB_ECHO,
    pool_size=settings.DB_POOL_SIZE,
    max_overflow=settings.DB_MAX_OVERFLOW,
    future=True,
)
async_session = sessionmaker(engine, expire_on_commit=False, class_=AsyncSession, future=True, autoflush=False)


async def get_db():
    db = async_session()
    try:
        yield db
    finally:
        await db.close()

And I have middlewares also

@b1-luettje
Copy link
Contributor

b1-luettje commented Apr 19, 2022

Same here with async_session used inside a context manager, pinning fastapi to 0.73.0 fixes it. Using asyncmy as driver.

async def get_db_session(request: Request) -> AsyncGenerator[AsyncSession, None]:
    async_sessionmaker = request.app.state.sessionmaker
    session: AsyncSession
    async with async_sessionmaker() as session:
        try:
            yield session
        except SQLAlchemyError:
            await shield(session.rollback())
            logger = get_logger()
            logger.exception("sqlalchemy_exception")

@XCanG
Copy link

XCanG commented Apr 19, 2022

Have similar bug when update from 0.73.2 to 0.75.2.

I have async connection to postgres DB and connection started to pile up until I reach DB max client limit.

This is screenshot from pgAdmin, from Dashboard and statistic tabs, as you can see connection stuck at ROLLBACK
image

My code for Dependency:

# imports...

engine = create_async_engine(DATABASE_URL, echo=DEBUG, future=True)

# ....

async def get_session() -> AsyncGenerator[AsyncSession, None]:
    async_session = sessionmaker(
        engine,
        class_=AsyncSession,  # type: ignore
        expire_on_commit=False,
    )

    async with async_session() as session:
        yield session
        await session.close()

Then in code I use it like:

@router.get("/count", response_model=UserCount)
async def counts_users(session: AsyncSession = Depends(get_session), current_user: User = Depends(get_current_user)) -> UserCount:
    # code ...
    # in CRUD:
    query = await session.execute(...)
    return UserCount(users=query.scalar_one())

@byrman
Copy link

byrman commented Apr 21, 2022

This is screenshot from pgAdmin, from Dashboard and statistic tabs, as you can see connection stuck at ROLLBACK

Same problem here with 0.74.1, but I don't think the connections are really stuck at ROLLBACK: they are idle and ROLLBACK is just the last query that was (successfully) executed. As reported above, 0.73.0 seems fine.

@byrman
Copy link

byrman commented Apr 21, 2022

It seems that the problem has to do with Depends. If I don't use FastAPI's dependency injection mechanism and create a session myself inside the router function, my problem is gone:

async def foo_list(
    # *,
    # session: AsyncSession = Depends(get_session),
):
    async with Session() as session:
        # Now it works

@adriangb
Copy link
Contributor

adriangb commented Apr 25, 2022

It looks like SQLAlchemy is using contextvars internally: https://github.com/sqlalchemy/sqlalchemy/blob/57286c3137cbb572a03fbec9bf5c428b76804d4a/lib/sqlalchemy/util/_concurrency_py3k.py#L10

And I think all of you are using BaseHTTPMiddleware (maybe via @app.middleware("http")), which has known issues when interacting with contextvars: https://github.com/encode/starlette/blob/5a9b41475ae1f54942ee67f90154f5da8f36e117/tests/middleware/test_base.py#L192-L223

Try removing your middleware. If that fixes the issue, then you just need to rewrite your middleware to not use BaseHTTPMiddleware (i.e., write a generic ASGI middleware).

@byrman
Copy link

byrman commented Apr 25, 2022

I'm using @app.middleware("http") and contextvars myself. By the way, I'm only experiencing this SAWarning in production, not in development. In production, my app is running behind nginx as a proxy. Strangely enough, the SAWarning and the problem with database connections is gone if I remove nginx from the equation. I don't yet understand why, but was wondering whether you are using this setup as well.

@adriangb
Copy link
Contributor

adriangb commented Apr 25, 2022

No idea why Nginx would have anything to do with this, I'm guessing it's just a symptom and not the cause.

As per above BaseHTTPMiddleware is known to cause issues with contextvars, although the 0.75.0 release was meant to resolve this, I believe there are other issues, e.g. this cancellation which might have unintended consequences when mixed with contextvars and FastAPI's di system. This will probably be fixed by encode/starlette#1441

Here's my minimal reproducible example:

from typing import Awaitable, Callable

from fastapi import FastAPI, Depends, Request ,Response
from sqlalchemy import text
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy.ext.asyncio import create_async_engine
from sqlalchemy.orm import sessionmaker


app = FastAPI()

@app.on_event("startup")
def startup() -> None:
    engine = create_async_engine(
        'postgresql+asyncpg://postgres:postgres@localhost:5433/postgres',
    )
    app.state.session_maker = sessionmaker(
        bind=engine,
        class_=AsyncSession
    )


async def get_db():
    db: AsyncSession = app.state.session_maker()
    try:
        yield db
    finally:
        print('finally get_db. Before close')
        await db.close()
        print('finally get_db. After close')


@app.get('/')
async def endpoint(
    db: AsyncSession = Depends(get_db)
):
    await db.execute(text('Select 1'))


@app.middleware('http')
async def mw(
    request: Request,
    call_next: Callable[[Request], Awaitable[Response]]
) -> Response:
    return await call_next(request)


if __name__ == "__main__":
    import uvicorn
    uvicorn.run(app)

Again, I would recommend you try rewriting your middleware as generic ASGI middleware and go from there. See this thread for some details: #2215 (comment)

@byrman
Copy link

byrman commented Apr 26, 2022

Thanks for your help, @adriangb, it's very much appreciated!

Here's my minimal reproducible example:

Should this reproduce the problem? I don't see the SAWarning and db connections don't pile up when running your code.

@byrman
Copy link

byrman commented Apr 26, 2022

Sorry, my bad: I was using swagger, but the problem starts when using curl, as reported by @Rashid567.

@byrman
Copy link

byrman commented Apr 26, 2022

Interestingly, if I swap Uvicorn for Hypercorn, your code seems to run fine, @adriangb. Does that make sense?

@adriangb
Copy link
Contributor

Maybe they handle lifespans differently? I know that Uvicorn runs lifespans in a sibling task to requests, which IMO can also lead to problems with things that use contextvars and the like.

@byrman
Copy link

byrman commented Apr 27, 2022

Adrian, I followed your advise, that is, ditched @app.middleware("http") and rewrote my middleware as generic ASGI middleware. Haven't seen the error since. Thank you so much!

@lesnik512
Copy link

I'm using https://github.com/trallnag/prometheus-fastapi-instrumentator which is using BaseHTTPMiddleware

I will try to replace it with https://github.com/stephenhillier/starlette_exporter

@byrman
Copy link

byrman commented Apr 28, 2022

Hmm, I didn't notice it earlier, because it's logged at INFO, but with my new middleware I get this at startup:

web_1             | INFO:     ASGI 'lifespan' protocol appears unsupported.

The application is functioning, but @app.on_event("startup") is no longer executed.

@byrman
Copy link

byrman commented Apr 28, 2022

Ah, you'll have to manage the lifetime scope: https://pgjones.dev/blog/how-to-write-asgi-middleware-2021/.

@adriangb
Copy link
Contributor

Can this issue be closed?

@davidfilat
Copy link

Yes
This only happens if you use return instead of yield in the generator that supplies the session.

@byrman
Copy link

byrman commented Jun 12, 2022

This only happens if you use return instead of yield in the generator that supplies the session.

Not true, I was definitely using yield. @adriangb's suggestion solved it for me, but it's a workaround, so I'm not sure the problem is really solved. I think it's up to @Rashid567, the owner of this issue, to decide?

@gdraynz
Copy link

gdraynz commented Jun 13, 2022

This is definitely still an issue, both with return and yield.
The workaround using an ASGI middleware works, but I would still like to be able to use a simple BaseHTTPMiddleware...

@adriangb
Copy link
Contributor

BaseHTTPMiddleware is fundamentally broken, I would not count on it being fixed.

@HenriBlacksmith
Copy link

HenriBlacksmith commented Jun 23, 2022

We face a similar problem after upgrading FastAPI from 0.73.0 to 0.78.0. From the comments above, it seems that the problem started with 0.74.0. From the diff : 0.73.0...0.74.0, I assume that it is probably caused by: #4575

@Rashid567
Copy link
Author

Rashid567 commented Sep 15, 2022

I checked issue again with latest packages. And also dropped db from test.

Python 3.10.7

requirements.txt

fastapi==0.84.0
uvicorn==0.18.3
hypercorn==0.14.3

Code

import asyncio
import datetime as dt
import fastapi


async def fake_get_db():
    db = 'fake_conn'
    try:
        yield db
    finally:
        print('Before finally await')
        await asyncio.sleep(1)
        print('After finally await')


def get_app():
    app = fastapi.FastAPI()

    @app.middleware('http')
    async def add_process_time_header(request: fastapi.Request, call_next):
        start_time = dt.datetime.now()
        response = await call_next(request)
        process_time = (dt.datetime.now() - start_time).total_seconds()
        response.headers["X-Process-Time"] = str(process_time)
        return response

    @app.get('/test')
    async def test(fake_db: str = fastapi.Depends(fake_get_db)):
        return 'OK'

    return app


if __name__ == '__main__':
    import uvicorn
    _app = get_app()
    uvicorn.run(_app, host='0.0.0.0', port=7776)

The problem still remains and it's not in db client.
It is directly related to the connection of the client to the server. If client close connection immediately, then problem appears. Otherwise no.

For example this code works fine:

import requests
from time import sleep

with requests.session() as s:
    print(s.get('http://localhost:7776/test'))
    sleep(1)

But if you comment sleep(1), you will not reach print('After finally await').

@adriangb Nginx use http 1.0 when sending requests to the app and close connection after getting response. This is why @byrman gets errors.

Also i tried:

  • Replace uvicorn with hypercorn. And it has no such problems.
  • Tried to use previous version of uvicorn. It does not help

I think it's dirrectly connected to AsyncExitStackMiddleware in fastapi. But how i don't have a clue

@byrman i don't think "not using http middleware with dependencies" and "not using uvicorn" is solution. So i am not going to close issue

@JarroVGIT
Copy link
Contributor

That is really interesting. The Session of requests does not yet close when you sleep for a second, which allows the FastAPI server to actually continue the task in the finally statement. If you remove the sleep(1) in your Session context manager, and you put breakpoints on the lines await asyncio.sleep(1) and print('After finally await'), you also sometimes get the correct result (and sometimes you don't).

It seems that, when using BaseHttpMiddleware, any pending task in the event loop is terminated once the connection is severed from the client side. And since the finally block is still in pending state (because it yields control to the event loop when calling asyncio.sleep()), it is also canceled.

Not behaviour one would expect, for sure. And again, when not using BaseHttpMiddleware this is not an issue at all.

@wangxin688
Copy link

wangxin688 commented Sep 30, 2022

Facing the same issue in 0.85.0, Depends(get_session) not close normally. In pgadmin shows that connections with IDLE state. I did a test on async_session or sync_session, it turns out both of them are working abnormal

@wangxin688
Copy link

Facing the same issue in 0.85.0, Depends(get_session) not close normally. In pgadmin shows that connections with IDLE state. I did a test on async_session or sync_session, it turns out both of them are working abnormal

after deep investigation, the issue was resolved by using Null pool

@wangxin688
Copy link

wangxin688 commented Oct 1, 2022

Facing the same issue in 0.85.0, Depends(get_session) not close normally. In pgadmin shows that connections with IDLE state. I did a test on async_session or sync_session, it turns out both of them are working abnormal

after deep investigation, the issue was resolved by using Null pool, It's not related to dependencies.
see: https://docs.sqlalchemy.org/en/14/core/pooling.html#using-connection-pools-with-multiprocessing-or-os-fork

Before:

from sqlalchemy import create_engine
from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
from sqlalchemy.orm.session import sessionmaker

from app.core.config import settings

if settings.ENVIRONMENT == "PYTEST":
    async_sqlalchemy_database_uri = settings.TEST_SQLALCHEMY_DATABASE_URI
else:
    async_sqlalchemy_database_uri = settings.ASYNC_DEFAULT_SQLALCHEMY_DATABASE_URI

async_engine = create_async_engine(async_sqlalchemy_database_uri, pool_pre_ping=True)
async_session = sessionmaker(async_engine, expire_on_commit=False, class_=AsyncSession)

get_session:

async def get_session():
    async with new_session() as session:
        yield session

Depends(get_session) test:
image

After:

from sqlalchemy import create_engine
from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
from sqlalchemy.orm.session import sessionmaker
from sqlalchemy.pool import NullPool

from app.core.config import settings

if settings.ENVIRONMENT == "PYTEST":
    async_sqlalchemy_database_uri = settings.TEST_SQLALCHEMY_DATABASE_URI
else:
    async_sqlalchemy_database_uri = settings.ASYNC_DEFAULT_SQLALCHEMY_DATABASE_URI

async_engine = create_async_engine(async_sqlalchemy_database_uri, pool_pre_ping=True, poolclass=NullPool)
async_session = sessionmaker(async_engine, expire_on_commit=False, class_=AsyncSession)

image

@XCanG
Copy link

XCanG commented Oct 3, 2022

Is there any drawbacks when using NullPool? Like as it will increase latency for each connection.

I also check docs and there is QueuePool (limit N) / StaticPool (limit 1) that might be used in this case? (Not sure if it will be better or worse)

@waza-ari
Copy link

waza-ari commented Oct 3, 2022

I am facing a similar issue as a regression in 0.85.0. MySQL connections not being closed correctly when injected using dependency injection. Pinning FastAPI to 0.84.0 fixes the issue.

This issue persists when using NullPool. Out of 93 connections being established during the tests, only ~84 are torn down. Going back to 0.84.0 and without any code changes all connections are closed again.

Further details (at an earlier stage of troubleshooting, at this point I didn't make the connection to FastAPI yet) can be found here: https://stackoverflow.com/questions/73927410/pytest-fixtures-sqalchemy-connections-not-closing-when-running-in-docker

After further investigation, removing the http middlewares does work around the issue as well, so does downgrading to 0.84.0

@mvbrn
Copy link

mvbrn commented Oct 27, 2022

I have the same problem after an upgrade to 0.85, database session dependency starts leaking connections

@JarroVGIT
Copy link
Contributor

Do you use BaseHTTPMiddleware in your code?

@wangxin688
Copy link

drawbacks

I have implement NullPool in our prod env for a few weeks, working fine till now

@mvbrn
Copy link

mvbrn commented Nov 9, 2022

@JarroVGIT yes, I use BaseHTTPMiddleware

@JarroVGIT
Copy link
Contributor

@mvbrn there seem to be some issues with the BaseHTTPMiddleware, and I have seen this issue come up as well. The yield part is not executed sometimes, haven’t been able to pinpoint why but I would expect this to happen with larger requests maybe?

Either way, there is discussion ongoing on deprecating it in the Starlette repo.

@aristeu13
Copy link

If I don't use Depends or any other middleware, do I risk explode database sessions?

Repository owner locked and limited conversation to collaborators Feb 28, 2023
@tiangolo tiangolo converted this issue into discussion #8443 Feb 28, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests