quic: ameliorating stream limit bug

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(),

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.

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()) {

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<quic::QuicCryptoClientStreamBase> 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.

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

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<Http::RequestEncoder*> encoders;
   std::vector<IntegrationStreamDecoderPtr> responses;
   std::vector<int> 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);

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};
 

tools/spelling/spelling_dictionary.txt

@@ -128,6 +128,7 @@ FQDN
 FREEBIND
 FUZZER
 FUZZERS
+dereferencing
 dnsresolvers
 guarddog
 GC