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][FIX] fastapi: Avoid zombie threads #486

Open
wants to merge 5 commits into
base: 16.0
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion fastapi/models/fastapi_endpoint.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# Copyright 2022 ACSONE SA/NV
# License LGPL-3.0 or later (http://www.gnu.org/licenses/LGPL).

import asyncio
import logging
import threading
from functools import partial
from itertools import chain
from typing import Any, Callable, Dict, List, Tuple
Expand All @@ -19,6 +21,26 @@
_logger = logging.getLogger(__name__)


# Thread-local storage for event loops
# Using a thread-local storage allows to have a dedicated event loop per thread
# and avoid the need to create a new event loop for each request. It's also
# compatible with the multi-worker mode of Odoo.
_event_loop_storage = threading.local()
Copy link

@sebalix sebalix Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, having one loop per thread is mandatory, missed this in #485 👍
EDIT: changing my mind, not working as expected, see below ⬇️

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have doubt here: this will attach the loop to the current thread, which is OK in workers mode as the current thread is always the main thread of the worker, but in multi-threads, that would mean we will have one loop per spawned thread to handle the user's request, which is not the main thread. Am I wrong?

I try to test all of this locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is Odoo spawning a new thread for each request in multithread mode?

Copy link

@sebalix sebalix Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I did to reproduce this issue:

  1. Here is Odoo running in multi-threads (2 exactly, forced with ODOO_MAX_HTTP_THREADS + cron threads disabled, and without queue_job neither), so we have the main thread+2 = 3:
    image

  2. Request done on a FastAPI endpoint (e.g. curl http://localhost:8069/{root_path}/sales), one daemon thread spawned to handle the loop as expected:
    image

  3. Beside in a Odoo shell, I reset the cache and relaunch a request on FastAPI endpoint:

    >>> env.registry.clear_caches()
    >>> env.registry.signal_changes()

    curl http://localhost:8069/{root_path}/sales

  4. Repeating step 3. several times: I get a new thread each time, so we are not re-using the loop on requests, while it was working as expected with [16.0][FIX] fastapi: prevent the creation of multiple event loops/threads #485 because having the loop created at the module level was done only once in the main thread, not for each subsequent thread created by Odoo to handle user's requests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebalix does that mean sharing the same event loop across threads? Is that safe?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is Odoo spawning a new thread for each request in multithread mode?

Yes: https://github.com/odoo/odoo/blob/18.0/odoo/service/server.py#L205-L217

@sebalix does that mean sharing the same event loop across threads? Is that safe?

I don't know... 🤷‍♂️ maybe not something supported by default (reading https://www.neilbotelho.com/blog/multithreaded-async.html ).

And what if we attach the event loop to the current thread? Or one not with daemon=True? So we get one event loop created for each user's request, does it imply a lot of overhead? (and we maybe lose the benefit of FastAPI by doing so)



def get_or_create_event_loop() -> asyncio.AbstractEventLoop:
"""
Get or create a reusable event loop for the current thread.
"""
if not hasattr(_event_loop_storage, "loop"):
loop = asyncio.new_event_loop()
loop_thread = threading.Thread(target=loop.run_forever, daemon=True)
loop_thread.start()
_event_loop_storage.loop = loop
_event_loop_storage.thread = loop_thread
return _event_loop_storage.loop


class FastapiEndpoint(models.Model):

_name = "fastapi.endpoint"
Expand Down Expand Up @@ -213,7 +235,8 @@ def get_app(self, root_path):
app = FastAPI()
app.mount(record.root_path, record._get_app())
self._clear_fastapi_exception_handlers(app)
return ASGIMiddleware(app)
event_loop = get_or_create_event_loop()
return ASGIMiddleware(app, loop=event_loop)

def _clear_fastapi_exception_handlers(self, app: FastAPI) -> None:
"""
Expand Down
Loading