Add config option to control maximum gateway worker thread count#5
Conversation
|
This pull request was cloned from Note: the URL is not a link to avoid triggering a notification on the original pull request. |
Hellebore
left a comment
There was a problem hiding this comment.
Hey @Hellebore - I've reviewed your changes and they look great!
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.
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, | |
| # 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)) |
|
|
||
| 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.
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") |
| 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.
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) | |
| threads: int = os.cpu_count() or 4, |
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_COUNTto control maximum number of worker threadsTODO
What's left to do: