quiche: implement stream idle timeout in codec by danzh2010 · Pull Request #17674 · envoyproxy/envoy

danzh2010 · 2021-08-11T15:33:01Z

Additional Description: refactor out MultiplexedStreamImplBase from http2 codec for code sharing with quic codec
Risk Level: low
Testing: enabled Http2IntegrationTest::CodecStreamIdleTimeout for Http3
Fixes #17654
Part of #16652

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk · 2021-08-12T16:46:23Z

@yanavlasov would you be willing to do first pass on this one?

yanavlasov · 2021-08-12T18:53:27Z

test/integration/multiplexed_integration_test.cc

      ::Envoy::Http2::Utility::initializeAndValidateOptions(
          envoy::config::core::v3::Http2ProtocolOptions());
-  http2_options.mutable_initial_stream_window_size()->set_value(65535);
+  http2_options.mutable_initial_stream_window_size()->set_value(32 * 1024);


I think for H/2 the 64K is the minimum. The 32K will be ignored in this case and 64K is applied.

Good point! In fact, the test still pass with H2. After looking to the log, it seems that before 64k flow control limit kicks in, socket already couldn't send all the data after sending ~32kB. I guess it passes because H2 hit congestion control window with a 32k payload. This is about the same for H3. If the test runs H3 with 64k flow control window, it will be blocked at ~32kB because of CC. As QUICHE needs to ACK packets by itself, not running the event loop of client connection in this test will cause the connection stuck. That's why I uses a smaller flow control window for H3.

To rule out the impact of CC window, I alter the test to still use 64k flow control window for H2 but use 32k for Quic.

yanavlasov · 2021-08-12T18:54:53Z

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

yanavlasov · 2021-08-16T19:24:57Z

LGTM from me, @alyssawilk for any additional comments

alyssawilk

Nice! So good to see this gap closed :-)
/wait

alyssawilk · 2021-08-16T19:41:30Z

source/common/http/http2/codec_impl.h

-        stream_idle_timer_.reset();
-      }
-    }
+    void destroy() override;


generally for overrides we comment the class it's from

// Http::MultiplexedStreamImplBase

alyssawilk · 2021-08-16T19:41:52Z

source/common/http/http2/codec_impl.h

    virtual HeaderMapPtr cloneTrailers(const HeaderMap& trailers) PURE;
-    virtual void createPendingFlushTimer() PURE;
-    void onPendingFlushTimer();
+    void onPendingFlushTimer() override;


this would then be moved up with destroy()

alyssawilk · 2021-08-16T19:42:21Z

source/common/http/http2/codec_impl.h

-    std::chrono::milliseconds stream_idle_timeout_{};
-    Event::TimerPtr stream_idle_timer_;
+
+  protected:


ditto on commenting the override.
alternately I don't see a reason to not have it public and group it - your call.

Done. It is only used by base class. So I prefer keep it as protected.

alyssawilk · 2021-08-16T19:45:30Z

source/common/quic/envoy_quic_client_stream.cc

  }
 }

+bool EnvoyQuicClientStream::hasPendingData() { return BufferedDataBytes() > 0; }


Maybe comment how this plays with trailers?

done. I added comment in server stream to which this interface actually matters.

alyssawilk · 2021-08-16T19:46:15Z

source/common/quic/envoy_quic_client_stream.h

                                 const quic::QuicHeaderList& header_list) override;

+  // Http::MultiplexedStreamImplBase
+  bool hasPendingData() override;


should we disable flush timer for client streams as we do for HTTP/2? or maybe since we have separate classes only inherit for the server stream?

It is disabled via overriding setFlushTimeout() to no-op a few lines above.
hasPendingData() is implemented here just because it is pure method in the base class.

ah, I missed it as it wasn't part of the diff, my bad!

Signed-off-by: Dan Zhang <danzh@google.com>

* main: Fix for fuzz tests failing due to invalid corpus paths (envoyproxy#17767) kafka: fix integration test (envoyproxy#17764) Fix typo in cluster.proto (envoyproxy#17755) cluster manager: add drainConnections() API (envoyproxy#17747) kafka broker filter: move to contrib (envoyproxy#17750) quiche: switch external dependency to github (envoyproxy#17732) quiche: implement stream idle timeout in codec (envoyproxy#17674) Update c-ares to 1.17.2 (envoyproxy#17704) Fix dns resolve fuzz bug (envoyproxy#17107) Remove members that shadow members of the base class (envoyproxy#17713) thrift proxy: missing parts from the previous PR (envoyproxy#17668) thrift-proxy: cleanup ConnectionManager::ActiveRpc (envoyproxy#17734) listener: extra warning for deprecated use_proxy_proto field (envoyproxy#17736) kafka: add support for metadata request in mesh-filter (envoyproxy#17597) upstream: add all host map to priority set for fast host searching (envoyproxy#17290) Remove the support for `hidden_envoy_deprecated_per_filter_config` (envoyproxy#17725) tls: SDS support for private key providers (envoyproxy#16737) bazel: update rules_foreign_cc (envoyproxy#17445) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

danzh1989 added 13 commits July 29, 2021 18:14

create unidirectional read stream

c275f94

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into pendingstream

531cfbc

Signed-off-by: Dan Zhang <danzh@google.com>

format

38774bb

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into pendingstream

dcf9fef

Signed-off-by: Dan Zhang <danzh@google.com>

not create server initiated stream

4392432

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into idlestream

635ee68

Signed-off-by: Dan Zhang <danzh@google.com>

test fail

c160b86

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into idlestream

ccf179d

Signed-off-by: Dan Zhang <danzh@google.com>

flush timeout for server stream

e378df5

Signed-off-by: Dan Zhang <danzh@google.com>

remove unnecessary change

dc817ee

Signed-off-by: Dan Zhang <danzh@google.com>

fix build

231b14e

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into idlestream

c7d1627

Signed-off-by: Dan Zhang <danzh@google.com>

wire up client stream

c72a1f4

Signed-off-by: Dan Zhang <danzh@google.com>

ggreenway assigned alyssawilk Aug 11, 2021

alyssawilk assigned yanavlasov and antoniovicente Aug 11, 2021

fix clang

77791de

Signed-off-by: Dan Zhang <danzh@google.com>

yanavlasov requested changes Aug 12, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 12, 2021

fix test flow control window

b65c64a

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Aug 13, 2021

format

b7b9049

Signed-off-by: Dan Zhang <danzh@google.com>

yanavlasov previously approved these changes Aug 16, 2021

View reviewed changes

alyssawilk reviewed Aug 16, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 16, 2021

comments

050631f

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed yanavlasov’s stale review via 050631f August 16, 2021 21:10

repokitteh-read-only bot removed the waiting label Aug 16, 2021

alyssawilk approved these changes Aug 17, 2021

View reviewed changes

alyssawilk merged commit 744b7bf into envoyproxy:main Aug 17, 2021

Conversation

danzh2010 commented Aug 11, 2021

Uh oh!

alyssawilk commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Aug 12, 2021

Uh oh!

yanavlasov commented Aug 16, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants