-
-
Notifications
You must be signed in to change notification settings - Fork 378
[18.0][FIX] fastapi: Forwardport 16.0 pullrequest 486 - Avoid zombie threads #499
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
Merged
OCA-git-bot
merged 6 commits into
OCA:18.0
from
dynapps:18.0-fastapi-fwp-16.0-event-loop-lifecycle
Jun 4, 2025
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7ce51b4
[FIX] fastapi: Avoid zombie threads
lmignon 457751d
[IMP] fastapi: add event loop lifecycle management
lmignon 1f7d17e
[FIX] fastapi: Graceful shutdown of event loop
lmignon ca1aee6
[FIX] fastapi: Ensure thread safety of the FastAPI app cache
lmignon 13f9de0
[IMP] fastapi: Improves app cache lifecycle
lmignon 9cde646
[MIG] fastapi: Add support for multiple Odoo cache sequences and fix …
lembregtse File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # Copyright 2025 ACSONE SA/NV | ||
| # License LGPL-3.0 or later (http://www.gnu.org/licenses/LGPL). | ||
| """ | ||
| ASGI middleware for FastAPI. | ||
|
|
||
| This module provides an ASGI middleware for FastAPI applications. The middleware | ||
| is designed to ensure managed the lifecycle of the threads used to as event loop | ||
| for the ASGI application. | ||
|
|
||
| """ | ||
|
|
||
| from collections.abc import Iterable | ||
|
|
||
| import a2wsgi | ||
| from a2wsgi.asgi import ASGIResponder | ||
| from a2wsgi.wsgi_typing import Environ, StartResponse | ||
|
|
||
| from .pools import event_loop_pool | ||
|
|
||
|
|
||
| class ASGIMiddleware(a2wsgi.ASGIMiddleware): | ||
| def __call__( | ||
| self, environ: Environ, start_response: StartResponse | ||
| ) -> Iterable[bytes]: | ||
| with event_loop_pool.get_event_loop() as loop: | ||
| return ASGIResponder(self.app, loop)(environ, start_response) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| from .event_loop import EventLoopPool | ||
| from .fastapi_app import FastApiAppPool | ||
| from odoo.service.server import CommonServer | ||
|
|
||
| event_loop_pool = EventLoopPool() | ||
| fastapi_app_pool = FastApiAppPool() | ||
|
|
||
|
|
||
| CommonServer.on_stop(event_loop_pool.shutdown) | ||
|
|
||
| __all__ = ["event_loop_pool", "fastapi_app_pool"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| # Copyright 2025 ACSONE SA/NV | ||
| # License LGPL-3.0 or later (http://www.gnu.org/licenses/LGPL). | ||
|
|
||
| import asyncio | ||
| import queue | ||
| import threading | ||
| from collections.abc import Generator | ||
| from contextlib import contextmanager | ||
|
|
||
|
|
||
| class EventLoopPool: | ||
| def __init__(self): | ||
| self.pool = queue.Queue[tuple[asyncio.AbstractEventLoop, threading.Thread]]() | ||
|
|
||
| def __get_event_loop_and_thread( | ||
| self, | ||
| ) -> tuple[asyncio.AbstractEventLoop, threading.Thread]: | ||
| """ | ||
| Get an event loop from the pool. If no event loop is available, | ||
| create a new one. | ||
| """ | ||
| try: | ||
| return self.pool.get_nowait() | ||
| except queue.Empty: | ||
| loop = asyncio.new_event_loop() | ||
| thread = threading.Thread(target=loop.run_forever, daemon=True) | ||
| thread.start() | ||
| return loop, thread | ||
|
|
||
| def __return_event_loop( | ||
| self, loop: asyncio.AbstractEventLoop, thread: threading.Thread | ||
| ) -> None: | ||
| """ | ||
| Return an event loop to the pool for reuse. | ||
| """ | ||
| self.pool.put((loop, thread)) | ||
|
|
||
| def shutdown(self): | ||
| """ | ||
| Shutdown all event loop threads in the pool. | ||
| """ | ||
| while not self.pool.empty(): | ||
| loop, thread = self.pool.get_nowait() | ||
| loop.call_soon_threadsafe(loop.stop) | ||
| thread.join() | ||
| loop.close() | ||
|
|
||
| @contextmanager | ||
| def get_event_loop(self) -> Generator[asyncio.AbstractEventLoop, None, None]: | ||
| """ | ||
| Get an event loop from the pool. If no event loop is available, | ||
| create a new one. | ||
|
|
||
| After the context manager exits, the event loop is returned to | ||
| the pool for reuse. | ||
| """ | ||
| loop, thread = self.__get_event_loop_and_thread() | ||
| try: | ||
| yield loop | ||
| finally: | ||
| self.__return_event_loop(loop, thread) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| # Copyright 2025 ACSONE SA/NV | ||
| # License LGPL-3.0 or later (http://www.gnu.org/licenses/LGPL). | ||
| import logging | ||
| import queue | ||
| import threading | ||
| from collections import defaultdict | ||
| from collections.abc import Generator | ||
| from contextlib import contextmanager | ||
|
|
||
| from odoo.api import Environment | ||
|
|
||
| from fastapi import FastAPI | ||
|
|
||
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class FastApiAppPool: | ||
| """Pool of FastAPI apps. | ||
|
|
||
| This class manages a pool of FastAPI apps. The pool is organized by database name | ||
| and root path. Each pool is a queue of FastAPI apps. | ||
|
|
||
| The pool is used to reuse FastAPI apps across multiple requests. This is useful | ||
| to avoid the overhead of creating a new FastAPI app for each request. The pool | ||
| ensures that only one request at a time uses an app. | ||
|
|
||
| The proper way to use the pool is to use the get_app method as a context manager. | ||
| This ensures that the app is returned to the pool after the context manager exits. | ||
| The get_app method is designed to ensure that the app made available to the | ||
| caller is unique and not used by another caller at the same time. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| with fastapi_app_pool.get_app(env=request.env, root_path=root_path) as app: | ||
| # use the app | ||
|
|
||
| The pool is invalidated when the cache registry is updated. This ensures that | ||
| the pool is always up-to-date with the latest app configuration. It also | ||
| ensures that the invalidation is done even in the case of a modification occurring | ||
| in a different worker process or thread or server instance. This mechanism | ||
| works because every time an attribute of the fastapi.endpoint model is modified | ||
| and this attribute is part of the list returned by the `_fastapi_app_fields`, | ||
| or `_routing_impacting_fields` methods, we reset the cache of a marker method | ||
| `_reset_app_cache_marker`. As side effect, the cache registry is marked to be | ||
| updated by the increment of the `cache_sequence` SQL sequence. This cache sequence | ||
| on the registry is reloaded from the DB on each request made to a specific database. | ||
| When an app is retrieved from the pool, we always compare the cache sequence of | ||
| the pool with the cache sequence of the registry. If the two sequences are | ||
| different, we invalidate the pool and save the new cache sequence on the pool. | ||
|
|
||
| The cache is based on a defaultdict of defaultdict of queue.Queue. We are cautious | ||
| that the use of defaultdict is not thread-safe for operations that modify the | ||
| dictionary. However the only operation that modifies the dictionary is the | ||
| first access to a new key. If two threads access the same key at the same time, | ||
| the two threads will create two different queues. This is not a problem since | ||
| at the time of returning an app to the pool, we are sure that a queue exists | ||
| for the key into the cache and all the created apps are returned to the same | ||
| valid queue. And the end, the lack of thread-safety for the defaultdict could | ||
| only lead to a negligible overhead of creating a new queue that will never be | ||
| used. This is why we consider that the use of defaultdict is safe in this context. | ||
| """ | ||
|
|
||
| def __init__(self): | ||
| self._queue_by_db_by_root_path: dict[str, dict[str, queue.Queue[FastAPI]]] = ( | ||
| defaultdict(lambda: defaultdict(queue.Queue)) | ||
| ) | ||
| self.__cache_sequences = {} | ||
| self._lock = threading.Lock() | ||
|
|
||
| def __get_pool(self, env: Environment, root_path: str) -> queue.Queue[FastAPI]: | ||
| db_name = env.cr.dbname | ||
| return self._queue_by_db_by_root_path[db_name][root_path] | ||
|
|
||
| def __get_app(self, env: Environment, root_path: str) -> FastAPI: | ||
| pool = self.__get_pool(env, root_path) | ||
| try: | ||
| return pool.get_nowait() | ||
| except queue.Empty: | ||
| return env["fastapi.endpoint"].sudo().get_app(root_path) | ||
|
|
||
| def __return_app(self, env: Environment, app: FastAPI, root_path: str) -> None: | ||
| pool = self.__get_pool(env, root_path) | ||
| pool.put(app) | ||
|
|
||
| @contextmanager | ||
| def get_app( | ||
| self, env: Environment, root_path: str | ||
| ) -> Generator[FastAPI, None, None]: | ||
| """Return a FastAPI app to be used in a context manager. | ||
|
|
||
| The app is retrieved from the pool if available, otherwise a new one is created. | ||
| The app is returned to the pool after the context manager exits. | ||
|
|
||
| When used into the FastApiDispatcher class this ensures that the app is reused | ||
| across multiple requests but only one request at a time uses an app. | ||
| """ | ||
| self._check_cache(env) | ||
| app = self.__get_app(env, root_path) | ||
| try: | ||
| yield app | ||
| finally: | ||
| self.__return_app(env, app, root_path) | ||
|
|
||
| def get_cache_sequence(self, key: str) -> int: | ||
| with self._lock: | ||
| return self.__cache_sequences.get(key, 0) | ||
|
|
||
| def set_cache_sequence(self, key: str, value: int) -> None: | ||
| with self._lock: | ||
| if ( | ||
| key not in self.__cache_sequences | ||
| or value != self.__cache_sequences[key] | ||
| ): | ||
| self.__cache_sequences[key] = value | ||
|
|
||
| def _check_cache(self, env: Environment) -> None: | ||
| cache_sequences = env.registry.cache_sequences | ||
| for key, value in cache_sequences.items(): | ||
| if ( | ||
| value != self.get_cache_sequence(key) | ||
| and self.get_cache_sequence(key) != 0 | ||
| ): | ||
| _logger.info( | ||
| "Cache registry updated, reset fastapi_app pool for the current " | ||
| "database" | ||
| ) | ||
| self.invalidate(env) | ||
| self.set_cache_sequence(key, value) | ||
|
|
||
| def invalidate(self, env: Environment, root_path: str | None = None) -> None: | ||
| db_name = env.cr.dbname | ||
| if root_path: | ||
| self._queue_by_db_by_root_path[db_name][root_path] = queue.Queue() | ||
| elif db_name in self._queue_by_db_by_root_path: | ||
| del self._queue_by_db_by_root_path[db_name] | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon Why do you think it is sufficient to delete content of _queue_by_db_by_root_path (ASGIMiddleware) this way? Threads created before supposed to be cleaned by Python GC? I've done some test with Odoo cache invalidation and logging current threads number, the number is incremented by 1 after every "Caches invalidated" signalling. The threads remain in the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the difference between 2 logs is 1 Odoo cache invalidation.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon I think I've found the issue. seems like you should have redefined "init" in ASGIMiddleware
(https://github.com/abersheeran/a2wsgi/blob/master/a2wsgi/asgi.py#L130)
because every time new loop is created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veryberry The middleware used is defined here and extend https://github.com/abersheeran/a2wsgi/blob/master/a2wsgi/asgi.py#L130. The loop used comes from a poo and if one is available into the pool it will be reused.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon the problem is that you have extended its
__call__methodBut actually the
__init__is triggered first, and there loop is alway None, and new loop is created every time you callreturn ASGIMiddleware(app).I've proved it locally controlling threads after every Odoo cache invalidation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see https://github.com/OCA/rest-framework/pull/508/files
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
@lmignon just wanted you to be aware of this issue and know your opinion on it. In my case I've moved context manager usage from
__call__to__init__. It helped.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
????
without seeing the code, I find it hard to imagine that it would work correctly with the change you describes. It's important that the eventloop theard can only be used for one call at a time. That's why it's the call method that's overloaded, because once the context manager exit, the pool becomes available again for another odoo process/thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon would you be so kind to explain why it's important that the eventloop theard can only be used for one call at a time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think of the event loop thread like a single train track. If two trains (calls) try to use the same track at the same time, they’ll collide, and everything will break. To keep things running smoothly, we let one train (call) complete its journey before allowing another one on the track.