fix: respect both local and remote HTTP/2 concurrent stream limits#141
Conversation
Introduce effective_max_concurrent_streams using min(remote, local default) to prevent StreamLimitExceeded when remote limit exceeds http-2's local default of 100. Adds max_concurrent_streams: option for explicit override. Fixes ostinelli#140
Reproduces the bug from ostinelli#140 by sending 101 async pushes against a server advertising 1000 streams. Also makes dummy server max_concurrent_streams configurable and processes stream responses concurrently.
|
Hi @katpadi, Thanks for the PR — I tested this on my side and confirmed that the original issue is resolved with your changes. Previously, we were able to reproduce The added spec also makes sense to me:
This matches the production scenario we observed quite well. The changes to the dummy server (allowing configurable From my perspective, this fix looks correct and addresses the issue. Thanks again for working on this! |
|
Hi @katpadi, Amazing, thank you! Is it necessary to expose
|
The connection already negotiates correctly by taking the minimum of the server-advertised limit and the http-2 local default. Exposing this as a user option adds no value and could mask stream limit errors if misconfigured.
|
@benubois - yeah, I agree. I removed |
|
Thanks again! |
Fixes #140
Problem
delayed_push_asyncraisesHTTP2::Error::StreamLimitExceededon the 101st concurrent push. The http-2 gem enforces 2 independent limits remote (APNs, typically 1000) and local (http-2 default, 100) but only the remote was checked, allowing stream creation past the local limit.Solution
effective_max_concurrent_streamsusesmin(remote, local)so backpressure triggers before either limit is hitmax_concurrent_streams:option onConnection.newfor explicit overrideStreamLimitExceededindelayed_push_asyncas a safety netTests