Skip to content
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

[16.0] fastapi: daemon thread spawned by a2wsgi.ASGIMiddleware #484

Open
sebalix opened this issue Jan 7, 2025 · 5 comments
Open

[16.0] fastapi: daemon thread spawned by a2wsgi.ASGIMiddleware #484

sebalix opened this issue Jan 7, 2025 · 5 comments
Labels
question Further information is requested

Comments

@sebalix
Copy link

sebalix commented Jan 7, 2025

Module

fastapi

Describe the bug

When <fastapi.endpoint>.get_app is called, it returns the FastAPI app wrapped in a2wsgi.ASGIMiddleware.

If the parameter loop is not set, this class is spawning a daemon thread automatically:
https://github.com/abersheeran/a2wsgi/blob/v1.10.8/a2wsgi/asgi.py#L130-L133

Is there an event loop we could re-use, or create a specific one (not daemonized) instead of having this automatically created one each time a request is dispatched on FastAPI app?

Additional context
We are reporting this because we spotted several daemon threads on Odoo (more than 30 on some of them), but we are still not sure if they could be linked to a performance issue (still digging). Note: we are running this Odoo instance in multi-threads (so we could have these daemon threads running forever as no worker is recycled).

@sebalix sebalix added the bug Something isn't working label Jan 7, 2025
@sebalix
Copy link
Author

sebalix commented Jan 7, 2025

Any insight @lmignon about it (as you wrote this part)?

@lmignon
Copy link
Contributor

lmignon commented Jan 7, 2025

@sebalix. The event loop is not created each time a request is dispatched. It's created the first time you call the <fastapi.endpoint>.get_app method on your thread or process since the result is stored into thread local cache by db @tools.ormcache("root_path") In this way we ensure a proper isolation of the event loop by request processing whatever the running mode of odoo.

@lmignon lmignon added question Further information is requested and removed bug Something isn't working labels Jan 7, 2025
@sebalix
Copy link
Author

sebalix commented Jan 8, 2025

Thank you for your answer. We dug a bit more and sadly, a new event loop as well as a new daemon thread are created each time the ORM cache is cleared, leading to multiple event loops/threads running in parallel.
This behavior is probably mitigated with Odoo running with workers (related threads vanished once the worker is recycled), but in multi-threads mode these loops/threads persist.
Here is a piece of code we can run in Odoo shell that reproduces the issue:

>>> import threading
>>> threading.active_count()
1
>>> threading.enumerate()
[<_MainThread(MainThread, started 139891588409152)>]

>>> app = env["fastapi.endpoint"].get_app("/endpoint")    # Spawn a deamon thread to run a newly created event loop
>>> threading.active_count()
2
>>> threading.enumerate()
[<_MainThread(MainThread, started 139891588409152)>, <Thread(Thread-1 (run_forever), started daemon 139891339724544)>]

>>> app = env["fastapi.endpoint"].get_app("/endpoint")    # Hit the cache
>>> threading.active_count()    # No new thread created
2
>>> threading.enumerate()
[<_MainThread(MainThread, started 139891588409152)>, <Thread(Thread-1 (run_forever), started daemon 139891339724544)>]

>>> env["fastapi.endpoint"].get_app.clear_cache(env["fastapi.endpoint"])
>>> app = env["fastapi.endpoint"].get_app("/endpoint")    # Spawn a new deamon thread + event loop
>>> threading.active_count()
3
>>> threading.enumerate()
[<_MainThread(MainThread, started 139891588409152)>, <Thread(Thread-1 (run_forever), started daemon 139891339724544)>, <Thread(Thread-2 (run_forever), started daemon 139891330283264)>]

IMO we cannot let ASGIMiddleware handling itself the loop/thread with Odoo running on top. Odoo should probably handle this loop (to run in a thread), and pass it as parameter to ASGIMiddleware.

@lmignon
Copy link
Contributor

lmignon commented Jan 8, 2025

@sebalix OK I see the problem. I've an idea on how to address-it in a way compatible with the multi workers/threads modes. I'll provide some code later today...

lmignon added a commit to acsone/rest-framework that referenced this issue Jan 8, 2025
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
@lmignon
Copy link
Contributor

lmignon commented Jan 8, 2025

see #486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants