Only apply conn_max_requests to HTTP/1 connections - #4777
Open
alco wants to merge 1 commit into
Open
Conversation
`ELECTRIC_TWEAKS_CONN_MAX_REQUESTS` was passed to both Bandit's `http_1_options` and `http_2_options`, but the option behaves very differently under each protocol. For HTTP/1, requests are served sequentially by a long-lived handler process and reaching `max_requests` closes the connection at a request boundary, recycling that process and its accumulated heap. That is the reason the tweak was introduced. For HTTP/2, requests already run in short-lived per-stream processes, so the memory concern doesn't apply. Once the cumulative stream count on a connection reaches `max_requests`, Bandit raises a connection-level error and tears down the whole multiplexed connection with GOAWAY/REFUSED_STREAM, disrupting concurrent streams. With the default of 50 this routinely cycled HTTP/2 connections and could produce synchronized reconnect bursts. Leave `http_2_options[:max_requests]` unset so Bandit's default of no limit applies, and add a regression test for the generated options. Fixes #4771 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zNLCRpP2XfBci3xmyTEGn
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELECTRIC_TWEAKS_CONN_MAX_REQUESTSwas being applied to both HTTP/1 and HTTP/2 Bandit connections, but the option means very different things under each protocol.Under HTTP/1, reaching
max_requestscloses the connection at a request boundary, recycling the long-lived handler process and its accumulated heap — the reason the tweak exists. Under HTTP/2, requests already run in short-lived per-stream processes, and once the cumulative stream count reachesmax_requestsBandit raises a connection-level error and tears down the whole multiplexed connection with GOAWAY/REFUSED_STREAM. With the default of 50, this routinely cycled HTTP/2 connections, disrupted in-flight streams, and could produce synchronized reconnect bursts.The limit is now only passed in
http_1_options; HTTP/2 connections keep Bandit's default of no request limit.Fixes #4771
🤖 Generated with Claude Code
https://claude.ai/code/session_012zNLCRpP2XfBci3xmyTEGn