[16.0][FIX] fastapi: Avoid zombie threads #486
Conversation
Each time a fastapi app is created, a new event loop thread is created by the ASGIMiddleware. Unfortunately, every time the cache is cleared, a new app is created with a new even loop thread. This leads to an increase in the number of threads created to manage the asyncio event loop, even though many of them are no longer in use. To avoid this problem, the thread in charge of the event loop is now created only once per thread / process and the result is stored in the thread's local storage. If a new instance of an app needs to be created following a cache reset, this ensures that the same event loop is reused. refs OCA#484
|
ping @sebalix |
|
@sbidoul additional expert advice would be welcome 😏 |
sbidoul
left a comment
sbidoul
left a comment
There was a problem hiding this comment.
Just a minor question, but otherwise this makes sense and looks good to me.
sebalix
left a comment
sebalix
left a comment
There was a problem hiding this comment.
Not approving based on my test here: https://github.com/OCA/rest-framework/pull/486/files#r1908260365
|
@sbidoul @sebalix Code will folllow |
|
@sebalix @sbidoul I've implemented a new approach based on queue.Queue to manage a dynamic pool of event loops and the fastapi application cache. I'm probably still missing a bit of code for shutting down the threads used for the event pool when the server stops. I haven't yet figured out how to implement this, even though the method is available on the pool to stop all the threads. |
lmignon
changed the title
This commit adds event loop lifecycle management to the FastAPI dispatcher. Before this commit, an event loop and the thread to run it were created each time a FastAPI app was created. The drawback of this approach is that when the app was destroyed (for example, when the cache of app was cleared), the event loop and the thread were not properly stopped, which could lead to memory leaks and zombie threads. This commit fixes this issue by creating a pool of event loops and threads that are shared among all FastAPI apps. On each call to a FastAPI app, a event loop is requested from the pool and is returned to the pool when the app is destroyed. At request time of an event loop, the pool try to reuse an existing event loop and if no event loop is available, a new event loop is created. The cache of the FastAPI app is also refactored to use it's own mechanism. It's now based on a dictionary of queues by root path by database, where each queue is a pool of FastAPI app. This allows a better management of the invalidation of the cache. It's now possible to invalidate the cache of FastAPI app by root path without affecting the cache of others root paths.
lmignon
force-pushed
the
16.0-fastapi-event-loop-lifecycle-lmi
branch
from
9ffda1a to
8c090cd
Compare
On server shutdown, ensure that created the event loops are closed properly.
lmignon
force-pushed
the
16.0-fastapi-event-loop-lifecycle-lmi
branch
from
f85ecbe to
289868d
Compare
defaultdict in python is not thread safe. Since this data structure is used to store the cache of FastAPI apps, we must ensure that the access to this cache is thread safe. This is done by using a lock to protect the access to the cache.
lmignon
force-pushed
the
16.0-fastapi-event-loop-lifecycle-lmi
branch
from
289868d to
1bf9751
Compare
This commit improves the lifecycle of the fastapi app cache. It first ensures that the cache is effectively invalidated when changes are made to the app configuration even if theses changes occur into an other server instance. It also remove the use of a locking mechanism put in place to ensure a thread safe access to a value into the cache to avoid potential concurrency issue when a default value is set to the cache at access time. This lock could lead to unnecessary contention and reduce the performance benefits of queue.Queue's fine-grained internal synchronization for a questionable gain. The only expected gain was to avoid the useless creation of a queue.Queue instance that would never be used since at the time of puting the value into the cache we are sure that a value is already present into the dictionary.
|
This PR works in production for the last 6 weeks. I plan to merge-it tomorrow. ping @AnizR @qgroulard @sebalix |
Thanks for this pr and the ping 😄 Small interrogation: can these 'zombie' threads be an explanation of a slow but steady increase of memory usage? |
It could be, but it's not the element that led to the detection of this problem, nor even a factor considered in the diagnostic phase. |
AnizR
left a comment
AnizR
left a comment
There was a problem hiding this comment.
Here is my review, thanks for you PR.
I have just one remark.
Besides that, the code seems good to me 👍
fastapi/pools/fastapi_app.py
Outdated
| try: | ||
| return pool.get_nowait() | ||
| except queue.Empty: | ||
| env["fastapi.endpoint"].sudo() |
There was a problem hiding this comment.
I think that this line is not necessary and can be removed, right?
There was a problem hiding this comment.
@AnizR I removed the useless line of code.
|
/ocabot merge minor |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 1e73351. Thanks a lot for contributing to OCA. ❤️ |
5 participants
Each time a fastapi app is created, a new event loop thread is created by the ASGIMiddleware. Unfortunately, every time the cache is cleared, a new app is created with a new even loop thread. This leads to an increase in the number of threads created to manage the asyncio event loop, even though many of them are no longer in use. To avoid this problem, the thread in charge of the event loop is now created only once per thread / process and the result is stored in the thread's local storage. If a new instance of an app needs to be created following a cache reset, this ensures that the same event loop is reused.
refs #484