-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Using dependency injection to get SQLAlchemy session can lead to deadlock #3205
Comments
I faced this problem. In 100% of cases, requests are blocked. Regardless of the frequency of requests, after about 7-10 requests, each time, requests are blocked. And so every time until you reboot. Tried it both in IDE on win10 and in Docker. On versions python 3.8 and 3.9 - nothing changes. |
I solved this problem by using this method - tiangolo/full-stack-fastapi-template#104 (comment). |
I'm not sure how background tasks are scheduled, but it doesn't look like that would always prevent the above situation. For example, running the above app using that solution still leads to deadlock with 100 concurrent users. Another possible solution is to keep @app.get('/')
def index(db: Session = Depends(get_db)):
with close_at_end(db) as db:
data = db.query(...)...
return {"data": data} where It seems like if you want to keep using dependencies, the real solution is to migrate to an async database library, and if you're already using SQLAlchemy the switch should be relatively easy with version 1.4. |
Hi everyone, During my locust test on my FastApi apis, I'm trying a 1000 users test with a 10 users spawn rate and after a moment, I have a QueuePool limit error. @contextmanager
def get_db():
session = SessionLocal()
try:
yield session
finally:
session.close()
@app.get("/users/get-mail/{mail}", response_model=schemas.User)
def check_user_by_email(mail: str):
with get_db() as db:
db_user = crud.get_user_by_email(db, email=mail)
return db_user I think there is a critical problem with Depends that @tiangolo needs to investigate because in their documentation, they advise to use Depends, but Depends is not ready for a production use ! Thanks to the community for the Session Manager solution that saves my production deployment ! |
We're seeing this issue too. I'm working on going in and figuring out the bug, but @Blue9 seems to be correct in the problem stemming from the operation code being run in a different thread than the dependency seems to be the locus. Temporary work arounds are to use a context manager, or use an async function with I would highly suggest to the FastAPI maintainers that they add a note in the documentation and on https://github.com/tiangolo/full-stack-fastapi-postgresql that there is a potential issue, as right now it is not production ready for a load greater than 30 or so concurrent users. |
We have this same issue when making use of a resource pool which acquires a resource in a generator. The example below illustrates the issue, this can be run using apache bench to simulate some actvity. ab -n 100 -c 20 http://localhost:8000/test import itertools
import asyncio
from concurrent.futures import ThreadPoolExecutor
from queue import Queue
from threading import currentThread
from fastapi.applications import FastAPI
from fastapi import Depends, Request
app = FastAPI()
pool: Queue[int] = Queue()
counter = itertools.count()
def get_data():
trace_id = next(counter)
ct = currentThread()
print(f"get trace-id-{trace_id}", ct.name, ct.native_id)
value = pool.get()
yield value
print(f"got trace-id-{trace_id}", ct.name, ct.native_id)
pool.put_nowait(value)
print(f"release trace-id-{trace_id}", ct.name, ct.native_id)
@app.get("/test")
def index(
req: Request,
data: dict[str, str] = Depends(get_data),
) -> dict:
return data
@app.on_event("startup")
def on_startup() -> None:
pool.put_nowait(1)
pool.put_nowait(2)
loop = asyncio.get_running_loop()
executor = ThreadPoolExecutor(1)
loop.set_default_executor(executor) From the output in the logs it seems like the same thread is acquiring resources from the pool and creating a deadlock. This seems to be caused by the get - resource trace-0 ThreadPoolExecutor-0_0 13479
got - resource trace-0 ThreadPoolExecutor-0_0
release - resource trace-0 ThreadPoolExecutor-0_0 13479
get - resource trace-1 ThreadPoolExecutor-0_0 13479
get - resource trace-2 ThreadPoolExecutor-0_1 13480
got - resource trace-1 ThreadPoolExecutor-0_0
got - resource trace-2 ThreadPoolExecutor-0_1
get - resource trace-3 ThreadPoolExecutor-0_0 13479
get - resource trace-4 ThreadPoolExecutor-0_1 13480 |
I've just been bitten by this issue as well. I had sqlalchemy configured with a pool of 10 and no overflow, and with 11 simultaneous connections the FastAPI server deadlocks completely and doesn't serve any request. I remove the DB dependency injection and started using the contextmanager method and now it works as it should: 10 requests are served immediately and the 11th gets handled afterwards. @tiangolo, could you clarify why dependency injection doesn't work in this case? Could you fix the examples in the documentation so more users aren't led astray? |
Hey all. I've been digging into this as well and I don't think this is a FastAPI issue per se. There are a few things going on that seem to be leading to the issue. tl;drDependencies and path operations defined as functions ( Workarounds?The suggested workaround to use a context manager within your path operation is by far the easiest solution. You're scoping the SQLAlchemy session lifecycle within the same coroutine, guaranteeing that connections can be acquired and released within that coroutine. This prevents the resource contention described in the "deep dive" section. While I haven't tried it yet.... It may be worth using the SQLAlchemy 1.4 Deep dive
^ First, it should be noted that
^ In the example code above, the SQLAlchemy connection pool is of size 4. This means that 4 requests will be able to check out a connection, while (N-4) connections will block on this line waiting for a connection to become available. (Side note, it's good practice to define connection timeouts on your
^ Also keep in mind that neither the dependency function nor the path operation are defined as coroutines. Both FastAPI and Starlette seem to want everything to run asynchronously, so these functions are invoked in a threadpool. See Starlette code, FastAPI doc 1, and FastAPI doc 2 for more information. Here is where things break. And here is our deadlock. All But what about native coroutines?While the root issue with the example code is related to the use of
^ As noted in the earlier section Here-in lies the issue. The event loop doesn't have the power to interrupt this coroutine; there is no I'd still argue this isn't really a FastAPI issue. The coroutine is executing a blocking call; whether it's SQLAlchemy or another third-party library, blocking calls pose this risk. However, I would argue the FastAPI However, I would argue the FastAPI documentation needs to be updated. The "Dependencies with yield" documentation and the "FastAPI & SQLAlchemy" example can both lead to deadlocks. |
I've read a lot of issues about this problem, but there's one thing I can't really figure out yet. Now we're using get_db as a context manager as it's preconized by a lot of people and it seems to work a lot better than using Depends. Now, the second problem is when you have a dependency, like "get_current_active_user". I've tried to refactor it like this : async def get_current_user(token: str = Depends(oauth2_scheme)) -> User:
with get_session() as db:
decoded_token = jwt.decode(token, SECRET_KEY, [ALGORITHM])
user = get_user(decoded_token['login'], db)
if not user:
raise HTTPException(400, 'User in token does not exists')
return user And my route looks like this (classic) : @playlists_router.get('/me', response_model=List[PlaylistShow], tags=['playlists'])
def get_my_playlists(current_user: User = Depends(get_current_user)):
with get_session() as db:
playlists = current_user.playlists
for p in playlists:
p.new_entries_count = len(get_new_entries_for_user(current_user.id, p.id, db))
return playlists I get this error : I've read that maybe we should do something with pydantic and Thanks for your time ! |
did you find a solution to this pblm ? |
@OpetherMB Unfortunately no, I decided to switch to Ormar which is an async ORM. It works very nicely now, but it required some refactoring. |
I notice if we create the dependency function get_session as async function, the connection pool issue gone ! so in short,
|
@happy-toro Declaring Also if it helps, I have compiled results from a list of load tests I performed with different implementations for |
That is some comprehensive testing right there, kudos! Indeed, changing your dependency function to a coroutine itself (using a sync def) won’t fix it, as the call to the DB later on will be blocking, waiting for a free connection, which will never happen as the event loop is blocked by the db call. Someone explained it above in a deep dive, good read! |
Can you folks try this "hack" (let's be clear: I am not recommending this is a solution) and see if it solves/is related to your issues? import anyio
app = FastAPI()
@app.on_event("startup")
def startup():
limiter = anyio.to_thread.current_default_thread_limiter()
limiter.total_tokens = 4096 |
I'll try @adriangb |
I've also been facing this issue on a production server, this seems like a major problem as FastAPI Dependencies with sync database connections are currently causing deadlocks @tiangolo |
@adriangb Thanks for the snippet, the limiter increase does seem to assist in the short term although 4096 is quite high & not feasible for some production systems. Is there anyway to get this PR more attention for review ? your concurrency fixes seem to be a good way to resolve this. |
Comment on it saying it fixed your issue (if you tested it and it did) |
@adriangb Thanks for speedy response, it's very much appreciated. I tested this open PR with locust, 5 endpoints and 40 concurrent users and still hitting the Queuepool error: Engine engine = create_engine(SQLALCHEMY_DATABASE_URL, pool_recycle=600)
SessionLocal = sessionmaker(bind=engine) deps: def get_db() -> Generator:
db = SessionLocal()
try:
yield db
finally:
db.close()` Errors: greenfields-api | File "/usr/local/lib/python3.9/site-packages/h11/_state.py", line 251, in _fire_event_triggered_transitions
greenfields-api | raise LocalProtocolError(
greenfields-api | h11._util.LocalProtocolError: can't handle event type ConnectionClosed when role=SERVER and state=SEND_RESPONSE
greenfields-api | ERROR:sqlalchemy.pool.impl.QueuePool:Exception during reset or similar
greenfields-api | Traceback (most recent call last):
greenfields-api | File "/usr/src/app/./app/api/dependencies.py", line 17, in get_db
greenfields-api | yield db
greenfields-api | GeneratorExit
raise exc.TimeoutError(\nsqlalchemy.exc.TimeoutError: QueuePool limit of size 5 overflow
10 reached, connection timed out, timeout 30.00 (Background on this error at: https://sqlalche.me/e/14/3o7r Uvicorn command: Main Packages:
|
I'm not sure what's going on with your test. Can you post the whole thing publicly in a reproducible format? These sorts of concurrency things are tricky, sometimes there's even more than one issue at play. For example, if you have a lot of clients and/or slow queries you'll still time out waiting for a database connection (even if there is no deadlock). |
Assuming the original need was handled, this will be automatically closed now. But feel free to add more comments or create new issues or PRs. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
First check
I've noticed that when using dependency injection with SQLAlchemy, a large number of concurrent requests can leave the app in a deadlocked state. This is especially noticeable with a small SQLAlchemy connection pool size (relative to the FastAPI thread pool size). Below is a self-contained example (you might have to tweak the pool size and the request body's sleep length but this should be a good starting point).
View
app.py
When running the above with 100 concurrent requests (I used locust), I noticed that only around 5 requests are served, and then the app freezes and is unable to serve any more requests. Below is the locustfile.
View
locustfile.py
I suspect the following is happening. (Note that
SessionLocal()
is lazy, sodb = SessionLocal()
will return immediately even if no connections are available.)N
requests come in (whereN
>= thread pool size). Theirget_db
dependencies run and yield, and we start executing their path operation functions. At this point, the entire thread pool is full. Onlypool_size
(4) requests are able to get a connection, and the remaining requests wait (in their path operation functions).pool_size
(4) spots in the thread pool. Because dependencies and requests run in separate threads, thefinally
blocks for these requests'get_db
dependencies have not run yet, so the connections for these requests have not returned to the SQLAlchemy pool.get_db
dependencies run, and we start executing their path operation functions. No connections have returned to the SQLAlchemy pool, so these requests wait. At this point, the entire thread pool is full, and every thread is waiting for a connection.finally
blocks for theirget_db
dependencies in a new thread so we can free the connections, but all of the threads are busy, so we end up waiting.This doesn't really seem like a bug in FastAPI or in SQLAlchemy, but it suggests that we should not use dependency injection like this when using synchronous database libraries. The only workaround I've found for this is to use a context manager to handle the session in the endpoint itself instead of injecting the database session directly.
Another thing I've noticed is that changing
get_db
to be an async function prevents deadlock (as does using the middleware approach), but only if the endpoint does not have aresponse_model
. If it has aresponse_model
then the app will still lock up. I believe this is because ifresponse_model
is defined, then when we runserialize_response
,field
will be non-None, and we will attempt to runfield.validate
in a separate thread. If the thread pool is full with requests waiting for connections, we won't be able to serialize the response and won't be able to close the database connection. Maybe we could serialize the response in the same thread as the path operation function; I'm not sure what the benefit of serializing in a separate thread is.There is similar discussion in tiangolo/full-stack-fastapi-template#104 and many others came to the conclusion that using a context manager is the right approach, but nothing really came of it. If others can validate that my suspicion is correct, then maybe we should change the docs to recommend using a context manager within the endpoint itself until a better solution is available.
Environment
The text was updated successfully, but these errors were encountered: