-
-
Notifications
You must be signed in to change notification settings - Fork 327
[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
base: 18.0
Are you sure you want to change the base?
Changes from all commits
7ce51b4
457751d
1f7d17e
ca1aee6
13f9de0
56a6d4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) |
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"] |
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) |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,136 @@ | ||||||||||||
# 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: | ||||||||||||
env["fastapi.endpoint"].sudo() | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remove this line? |
||||||||||||
return env["fastapi.endpoint"].sudo().get_app(root_path) | ||||||||||||
Comment on lines
+78
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
|
||||||||||||
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] | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lmignon the problem is that you have extended its There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
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 #510