Skip to content

Conversation

@lmignon
Copy link
Contributor

@lmignon lmignon commented Mar 17, 2025

Before this change, an eventloop thread was created on each instanciation of the ASGIMiddleware class. We want to avoid this behavior to avoid the creation of zombie threads

Before this change, an eventloop thread was created on each instanciation of the ASGIMiddleware class. We want to avoid this behavior to avoid the creation of zombie threads
@lmignon lmignon force-pushed the 16.0-zombie-thread-next-lmi branch from 8d8d5a7 to 5505a6a Compare March 17, 2025 13:12
@lmignon
Copy link
Contributor Author

lmignon commented Mar 17, 2025

ping @sebalix @AnizR

# Since the the base class check if the given loop is
# None, we can pass False to avoid the initialization
# of the default event loop
super().__init__(app, wait_time, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit hacky but I don't see any other option.

def __init__(
self,
app: ASGIApp,
wait_time: float | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we need to keep the parent signature?

loop: Optional[asyncio.AbstractEventLoop] = None,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO no. This class is designed to be used in the specific context of the fastapi addon where we enforce the use of the pool of eventloop threads.

@lmignon
Copy link
Contributor Author

lmignon commented Apr 10, 2025

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-508-by-lmignon-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 42644d8 into OCA:16.0 Apr 10, 2025
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 90ae5a4. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants