-
Notifications
You must be signed in to change notification settings - Fork 0
Add config option to control maximum gateway worker thread count #10
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: base-sha/17fa5ee48109ff6c245bd30d18e9910d3e81e3b1
Are you sure you want to change the base?
Conversation
This pull request was cloned from Note: the URL is not a link to avoid triggering a notification on the original pull request. |
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.
Hey @Hellebore - I've reviewed your changes and they look great!
General suggestions:
- Consider validating the
GATEWAY_WORKER_COUNT
value to ensure it falls within a reasonable range, preventing misconfiguration that could lead to system instability. - Document the new configuration option, including guidance on how to choose an appropriate value based on expected load and available system resources.
- Review the impact of this change on existing deployments to ensure that the default behavior remains consistent and that performance is not negatively impacted for users who do not adjust the new setting.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
gateway: Gateway, | ||
listen: HostAndPort | list[HostAndPort], | ||
use_ssl: bool = False, | ||
threads: int | None = None, |
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.
suggestion (performance): Consider providing a default value for
threads
that aligns with system capabilities.
Using None
as a default for threads
and later substituting it with a hardcoded value (1000) might not be optimal for all environments. Consider a default that scales with the number of available CPU cores.
threads: int | None = None, | |
threads: int = os.cpu_count() or 4, |
lifespan_listener: Optional[ASGILifespanListener] = None, | ||
websocket_listener=None, | ||
) -> None: | ||
self.gateway = gateway | ||
|
||
self.event_loop = event_loop or asyncio.get_event_loop() | ||
self.executor = _ThreadPool(threads, thread_name_prefix="asgi_gw") | ||
self.executor = _ThreadPool(threads or 1000, thread_name_prefix="asgi_gw") |
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.
suggestion (code_refinement): Use of
or
for default parameter handling is clear but consider centralizing default values.
While the use of or
for providing a default value is straightforward, centralizing default values (e.g., as constants) can make future adjustments and readability better.
self.executor = _ThreadPool(threads or 1000, thread_name_prefix="asgi_gw") | |
DEFAULT_THREAD_COUNT = 1000 # Centralized default value for easier adjustments and readability | |
self.executor = _ThreadPool(threads or DEFAULT_THREAD_COUNT, thread_name_prefix="asgi_gw") |
@@ -655,6 +655,8 @@ def populate_edge_configuration( | |||
GATEWAY_LISTEN, | |||
) = populate_edge_configuration(os.environ) | |||
|
|||
GATEWAY_WORKER_COUNT = int(os.environ.get("GATEWAY_WORKER_COUNT") or 1000) |
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.
suggestion (code_refinement): Ensure environment variable fallback values are consistent across the codebase.
The fallback of 1000 for GATEWAY_WORKER_COUNT
is used directly in multiple places. Consider defining this as a constant to ensure consistency and ease of maintenance.
GATEWAY_WORKER_COUNT = int(os.environ.get("GATEWAY_WORKER_COUNT") or 1000) | |
# Define a constant for the default gateway worker count | |
DEFAULT_GATEWAY_WORKER_COUNT = 1000 | |
GATEWAY_WORKER_COUNT = int(os.environ.get("GATEWAY_WORKER_COUNT", DEFAULT_GATEWAY_WORKER_COUNT)) |
Motivation
On very high (burst) load scenarios, we can get connection refused errors quite quickly, if there are no worker threads available.
We should make it configurable, to see if we can bypass some of them.
Changes
GATEWAY_WORKER_COUNT
to control maximum number of worker threadsTODO
What's left to do: