diff --git a/source/common/http/http3/conn_pool.cc b/source/common/http/http3/conn_pool.cc index 9bba3fa0221c7..fbc3e45036078 100644 --- a/source/common/http/http3/conn_pool.cc +++ b/source/common/http/http3/conn_pool.cc @@ -15,14 +15,28 @@ namespace Envoy { namespace Http { namespace Http3 { +namespace { + +uint32_t getMaxStreams(const Upstream::ClusterInfo& cluster) { + return PROTOBUF_GET_WRAPPED_OR_DEFAULT(cluster.http3Options().quic_protocol_options(), + max_concurrent_streams, 100); +} + +} // namespace + +ActiveClient::ActiveClient(Envoy::Http::HttpConnPoolImplBase& parent, + Upstream::Host::CreateConnectionData& data) + : MultiplexedActiveClientBase(parent, getMaxStreams(parent.host()->cluster()), + parent.host()->cluster().stats().upstream_cx_http3_total_, data) { +} void Http3ConnPoolImpl::setQuicConfigFromClusterConfig(const Upstream::ClusterInfo& cluster, quic::QuicConfig& quic_config) { + // TODO(alyssawilk) use and test other defaults. quic::QuicTime::Delta crypto_timeout = quic::QuicTime::Delta::FromMilliseconds(cluster.connectTimeout().count()); quic_config.set_max_time_before_crypto_handshake(crypto_timeout); - int32_t max_streams = - cluster.http3Options().quic_protocol_options().max_concurrent_streams().value(); + int32_t max_streams = getMaxStreams(cluster); quic_config.SetMaxBidirectionalStreamsToSend(max_streams); quic_config.SetMaxUnidirectionalStreamsToSend(max_streams); Quic::configQuicInitialFlowControlWindow(cluster.http3Options().quic_protocol_options(), diff --git a/source/common/http/http3/conn_pool.h b/source/common/http/http3/conn_pool.h index 0886d4ddaf7ae..6ae87420207e0 100644 --- a/source/common/http/http3/conn_pool.h +++ b/source/common/http/http3/conn_pool.h @@ -22,16 +22,7 @@ namespace Http3 { class ActiveClient : public MultiplexedActiveClientBase { public: ActiveClient(Envoy::Http::HttpConnPoolImplBase& parent, - Upstream::Host::CreateConnectionData& data) - : MultiplexedActiveClientBase(parent, - parent.host() - ->cluster() - .http3Options() - .quic_protocol_options() - .max_concurrent_streams() - .value(), - parent.host()->cluster().stats().upstream_cx_http3_total_, - data) {} + Upstream::Host::CreateConnectionData& data); }; // Http3 subclass of FixedHttpConnPoolImpl which exists to store quic data. diff --git a/source/common/quic/codec_impl.cc b/source/common/quic/codec_impl.cc index 21100bb788825..5ac2bab02dc02 100644 --- a/source/common/quic/codec_impl.cc +++ b/source/common/quic/codec_impl.cc @@ -79,9 +79,6 @@ Http::RequestEncoder& QuicHttpClientConnectionImpl::newStream(Http::ResponseDecoder& response_decoder) { EnvoyQuicClientStream* stream = quicStreamToEnvoyClientStream(quic_client_session_.CreateOutgoingBidirectionalStream()); - // TODO(danzh) handle stream creation failure gracefully. This can happen when - // there are already 100 open streams. In such case, caller should hold back - // the stream creation till an existing stream is closed. ASSERT(stream != nullptr, "Fail to create QUIC stream."); stream->setResponseDecoder(response_decoder); if (quic_client_session_.aboveHighWatermark()) { diff --git a/source/common/quic/envoy_quic_client_session.h b/source/common/quic/envoy_quic_client_session.h index 7b5e1c9b079c2..30462fa77feed 100644 --- a/source/common/quic/envoy_quic_client_session.h +++ b/source/common/quic/envoy_quic_client_session.h @@ -80,7 +80,12 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, quic::QuicSpdyStream* CreateIncomingStream(quic::QuicStreamId id) override; quic::QuicSpdyStream* CreateIncomingStream(quic::PendingStream* pending) override; std::unique_ptr CreateQuicCryptoStream() override; - + bool ShouldCreateOutgoingBidirectionalStream() override { + ASSERT(quic::QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream()); + // Prefer creating an "invalid" stream outside of current stream bounds to + // crashing when dereferencing a nullptr in QuicHttpClientConnectionImpl::newStream + return true; + } // QuicFilterManagerConnectionImpl bool hasDataToWrite() override; // Used by base class to access quic connection after initialization. diff --git a/test/integration/BUILD b/test/integration/BUILD index dd9a77c51b755..12d76944fe332 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -368,7 +368,7 @@ envoy_cc_test( "multiplexed_integration_test.cc", "multiplexed_integration_test.h", ], - shard_count = 4, + shard_count = 8, deps = [ ":http_protocol_integration_lib", "//source/common/buffer:buffer_lib", diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index 2a785467bfd07..d47abf5f0ea20 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -202,9 +202,11 @@ TEST_P(Http2UpstreamIntegrationTest, LargeSimultaneousRequestWithBufferLimits) { simultaneousRequest(1024 * 20, 1024 * 14 + 2, 1024 * 10 + 5, 1024 * 16); } -void Http2UpstreamIntegrationTest::manySimultaneousRequests(uint32_t request_bytes, uint32_t) { +void Http2UpstreamIntegrationTest::manySimultaneousRequests(uint32_t request_bytes, + uint32_t max_response_bytes, + uint32_t num_requests) { + autonomous_allow_incomplete_streams_ = true; TestRandomGenerator rand; - const uint32_t num_requests = 50; std::vector encoders; std::vector responses; std::vector response_bytes; @@ -213,7 +215,7 @@ void Http2UpstreamIntegrationTest::manySimultaneousRequests(uint32_t request_byt codec_client_ = makeHttpConnection(lookupPort("http")); for (uint32_t i = 0; i < num_requests; ++i) { - response_bytes.push_back(rand.random() % (1024 * 2)); + response_bytes.push_back(rand.random() % (max_response_bytes)); auto headers = Http::TestRequestHeaderMapImpl{ {":method", "POST"}, {":path", "/test/long/url"}, @@ -234,8 +236,11 @@ void Http2UpstreamIntegrationTest::manySimultaneousRequests(uint32_t request_byt ASSERT_TRUE(responses[i]->waitForEndStream()); if (i % 2 != 0) { EXPECT_TRUE(responses[i]->complete()); - EXPECT_EQ("200", responses[i]->headers().getStatusValue()); - EXPECT_EQ(response_bytes[i], responses[i]->body().length()); + // TODO(18160) remove this if and always check for 200 and body length. + if (num_requests <= 100 || upstreamProtocol() != Http::CodecType::HTTP3) { + EXPECT_EQ("200", responses[i]->headers().getStatusValue()); + EXPECT_EQ(response_bytes[i], responses[i]->body().length()); + } } else { // Upstream stream reset. EXPECT_EQ("503", responses[i]->headers().getStatusValue()); @@ -247,9 +252,17 @@ void Http2UpstreamIntegrationTest::manySimultaneousRequests(uint32_t request_byt } TEST_P(Http2UpstreamIntegrationTest, ManySimultaneousRequest) { - manySimultaneousRequests(1024, 1024); + manySimultaneousRequests(1024, 1024, 100); } +#ifdef NDEBUG +// TODO(alyssawilk) this causes crashes in debug mode for QUIC due to a race +// condition between Envoy's stream accounting and QUICE's. Debug and fix. +TEST_P(Http2UpstreamIntegrationTest, TooManySimultaneousRequests) { + manySimultaneousRequests(1024, 1024, 200); +} +#endif + TEST_P(Http2UpstreamIntegrationTest, ManyLargeSimultaneousRequestWithBufferLimits) { config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream. manySimultaneousRequests(1024 * 20, 1024 * 20); diff --git a/test/integration/multiplexed_upstream_integration_test.h b/test/integration/multiplexed_upstream_integration_test.h index 0b2eecc318c44..14aeb56a49c0b 100644 --- a/test/integration/multiplexed_upstream_integration_test.h +++ b/test/integration/multiplexed_upstream_integration_test.h @@ -14,7 +14,8 @@ class Http2UpstreamIntegrationTest : public HttpProtocolIntegrationTest { } void bidirectionalStreaming(uint32_t bytes); - void manySimultaneousRequests(uint32_t request_bytes, uint32_t response_bytes); + void manySimultaneousRequests(uint32_t request_bytes, uint32_t max_response_bytes, + uint32_t num_streams = 50); bool use_alpn_{false}; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index bfc60ecaec0dc..49df254f0c663 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -128,6 +128,7 @@ FQDN FREEBIND FUZZER FUZZERS +dereferencing dnsresolvers guarddog GC