Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
cc @mum4k thanks again for the pointers w.r.t. nighthawk! |
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
LGTM, modulo two nits. Nice catch!
| ASSERT(connecting_stream_capacity_ >= delta); | ||
| connecting_stream_capacity_ -= delta; | ||
| } | ||
| void incrConnectingAndConnectedStreamCapacity(uint32_t delta) { |
There was a problem hiding this comment.
nit: newline before
| } | ||
|
|
||
| TEST_P(Http2UpstreamIntegrationTest, ManySimultaneousRequestsLaxUpstreamLimits) { | ||
| if (upstreamProtocol() == Http::CodecType::HTTP2) { |
There was a problem hiding this comment.
Does this imply that this test will run for HTTP/1 and HTTP/3? Or is this only for HTTP/3 (since the file is multiplexed_upstream and HTTP/1 is not multiplexed?). If it's only HTTP/3, then maybe:
if (upstreamProtocol() != Http::CodecType::HTTP3) {
return;
}
(and skip the conditional at line 283).
Alternatively, if it runs for HTTP/1, maybe add a comment?
There was a problem hiding this comment.
was cargo culted from prior test - it actually works for both so updating.
|
Thank you for your help in addressing this @alyssawilk. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| parent_.decrClusterStreamCapacity(old_capacity - new_capacity); | ||
| } else if (old_capacity < new_capacity) { | ||
| parent_.incrClusterStreamCapacity(new_capacity - old_capacity); | ||
| if (connect_timer_) { |
There was a problem hiding this comment.
Question: Why doesn't this just call MultiplexedActiveClientBase::onSettings()? In other words, why is this handled completely differently than http/2?
There was a problem hiding this comment.
I'm not sure if this answers your question but, I think the crux of the issue is that with HTTP/2 the setting SETTINGS_MAX_CONCURRENT_STREAMS specifies a limit on "open" streams. This means that a client may open a new stream any time it closes a stream. But HTTP/3 stream limits are managed at the QUIC layer. With QUIC, the limit is (effectively) the largest stream ID that can be opened. When a client closes a stream, that has no effect on the limit and the client may not be able to open a new stream. Only when the server sends a new MAX_STREAMS frame will the limit increase. This difference between HTTP/2 and HTTP/3 is pretty fundamental, albeit rather annoying.
There was a problem hiding this comment.
Got it; that makes sense. Thanks!
- as of envoyproxy/envoy#18668 `Envoy::Config::Utility::createTagProducer` now takes a new parameter `Stats::TagVector`. - removed workaround that allowed the HTTP3 integration test to pass while Envoy assumed that `--max-concurrent-streams` is always 100. This was fixed in envoyproxy/envoy#18879. - no changes in `.bazelrc`, `.bazelversion` or `ci/run_envoy_docker.sh`. Signed-off-by: Jakub Sobon <mumak@google.com>
- as of envoyproxy/envoy#18668 `Envoy::Config::Utility::createTagProducer` now takes a new parameter `Stats::TagVector`. - removed workaround that allowed the HTTP3 integration test to pass while Envoy assumed that `--max-concurrent-streams` is always 100. This was fixed in envoyproxy/envoy#18879. - updated `tools/update_cli_readme_documentation.py` to match on custom markers rather than the backtick character, since one of the flags now contains backticks in its description. Marked sections for replacement with the custom edit markers in our documents. - executed `update_cli_readme_documentation --mode fix`. - no changes in `.bazelrc`, `.bazelversion` or `ci/run_envoy_docker.sh`. Signed-off-by: Jakub Sobon <mumak@google.com>
4 participants
Fixing a bug where the new quic limits weren't respected on connection creation, because accounting used effectiveConcurrentStreamLimit() which wasn't aware of quic stream limitations.
Risk Level: Medium
Testing: new integration test
Docs Changes: n/a
Release Notes: n/a