Skip to content

Add config option to control maximum gateway worker thread count#1

Open
Hellebore wants to merge 4 commits into
base-sha/17fa5ee48109ff6c245bd30d18e9910d3e81e3b1from
head-sha/24278fae889d7cb720bb1c961a3d4d2f190abb90/2024-03-20T16-04-05/c61dca
Open

Add config option to control maximum gateway worker thread count#1
Hellebore wants to merge 4 commits into
base-sha/17fa5ee48109ff6c245bd30d18e9910d3e81e3b1from
head-sha/24278fae889d7cb720bb1c961a3d4d2f190abb90/2024-03-20T16-04-05/c61dca

Conversation

@Hellebore

Copy link
Copy Markdown

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

  • Add config option GATEWAY_WORKER_COUNT to control maximum number of worker threads

TODO

What's left to do:

  • Test real life impact of this for certain high connection count scenarios
  • Discuss naming of the variable
  • Discuss where to set the variable.. We might want to opt to set it in the GatewayServer (in http/hypercorn.py) instead, or even in the serve_gateway variable, to limit the variables use to the "main gateway".

@Hellebore

Copy link
Copy Markdown
Author

This pull request was cloned from https://github.com/localstack/localstack/pull/10016

Note: the URL is not a link to avoid triggering a notification on the original pull request.

@Hellebore Hellebore left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sourcery logo Sourcery Review:

Hey @Hellebore - I've reviewed your changes and they look great!

General suggestions:

  • Consider scaling the default worker thread count based on system capabilities to optimize for different environments.
  • Centralize the default value for the worker thread count to improve code maintainability and readability.
  • Ensure consistency in handling environment variable fallback values across the codebase.
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

LangSmith trace

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,

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sourcery logo 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.

Suggested change
threads: int | None = None,
threads: int = os.cpu_count() or 4,


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")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sourcery logo 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.

Suggested change
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")

Comment thread localstack/config.py
GATEWAY_LISTEN,
) = populate_edge_configuration(os.environ)

GATEWAY_WORKER_COUNT = int(os.environ.get("GATEWAY_WORKER_COUNT") or 1000)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sourcery logo 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.

Suggested change
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))

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants