quiche: handle stream blockage during decodeHeaders and decodeTrailers #16128

danzh2010 · 2021-04-22T18:43:27Z

Commit Message: use posted callback to block/unblock quic stream in EnvoyQuicStream::readDisable() with weak_ptr to stream to handle stream life time issue.
Additional Description: currently if readDisabled() is called in decodeHeaders|Trailers(), the stream will be blocked right away which breaks the assumption QUICHE has that a stream shouldn't be blocked during OnInitialHeadersComplete() and OnTrailingHeadersComplete().
This change makes the stream state change completely outside of QUICHE call stack. (The unblocking is already outside of QUICHE call stack in existing implementation.) Also simplify the blockage state change logic.

Risk Level: low
Testing: added more unit tests and enabled Http2UpstreamIntegrationTest::ManyLargeSimultaneousRequestWithBufferLimits which was flaky.

Part of #2557 #14829

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-04-22T19:06:14Z

/assign @antoniovicente

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-04-25T19:59:23Z

/assign @alyssawilk

alyssawilk

Thanks for tracking this one down!

alyssawilk · 2021-04-26T15:57:23Z

source/common/quic/envoy_quic_stream.h

+    // stream can trigger another decoding inside the callstack which messes up stream state, and
+    // blocking a quic stream in a QUICHE callstack is not expected by QUICHE.
+    *unblock_posted_ = true;
+    connection()->dispatcher().post(


optional: If this is happening with regularity would it make more sense to have an alarm registered at now? It'd handle the deletion of the session by unregistering on destruction so be a bit cleaner and you could check alarm registration rather than have a boolean here.

+1

Please consider use of a SchedulableCallback owned by the stream. Call scheduleCallbackNextIteration() to schedule the callback; call cancel() or delete the SchedulableCallback to avoid getting called once you have entered the stream termination sequence.

Switched to use SchedulableCallback. Instead of scheduleCallbackNextIteration(), I used scheduleCallbackInCurrentIteration() because all we need is to not call stream SetUnblocked() in place.

alyssawilk · 2021-04-26T16:06:48Z

source/common/quic/envoy_quic_client_stream.cc

    sequencer()->SetBlockedUntilFlush();
  } else {
-    ASSERT(read_disable_counter_ == 0, "readDisable called in between.");
    sequencer()->SetUnblocked();


looking at SetBlockedUntilFlush it's pretty clear calling it extra times won't waste CPU or be unsafe. If we read disable then read enable, we may end up calling some spurious SetUnblocked. That looks ok but can you confirm, and comment in code if it's fine?

Yes, an extra OnDataAvailable() won't cause another push as data should have already been pushed in QUICHE stack or remain blocked by other condition.

SG, can you please comment that in code then? The other Envoy read enable/disable need parity (one unblock per block) so I think it's worth calling out that this is different.

Commented at scheduling async_stream_blockage_change_ in readDisable() about the transient block and unblock.

ah, that's not quite what I was looking for.
Can we do something like comment in switchStreamBlockState that unlike the envoy readDisable calls, that if the underlying QUIC code has received N block calls, it will still unblock on one block call (so parity between block and unblock calls is not necessary)?

Signed-off-by: Dan Zhang <[email protected]>

antoniovicente · 2021-04-27T22:54:13Z

source/common/quic/envoy_quic_stream.h

+    // stream will be spuriously unblocked and call OnDataAvailable(). This call shouldn't take any
+    // effect because any available data should have been processed already upon arrival or they
+    // were blocked by some condition other than flow control, i.e. Qpack decoding.
+    async_stream_blockage_change_->scheduleCallbackCurrentIteration();


Please use scheduleCallbackNextIteration

Use of scheduleCallbackCurrentIteration can result in an infinite chain of callbacks which starve every other connection in the proxy.

Signed-off-by: Dan Zhang <[email protected]>

DavidSchinazi

Nice! Feel free to ignore the nit if that's the only change before submission to avoid having to rerun CI just for that.

DavidSchinazi · 2021-04-28T15:41:04Z

test/common/quic/envoy_quic_client_stream_test.cc

+        EXPECT_EQ("200", headers->getStatusValue());
+        quic_stream_->readDisable(true);
+      }));
+  if (quic::VersionUsesHttp3(quic_version_.transport_version)) {


nit: if (quic_version_.UsesHttp3()) {

alyssawilk

LGTM modulo one comment nit

alyssawilk · 2021-04-28T15:53:27Z

source/common/quic/envoy_quic_client_stream.cc

    sequencer()->SetBlockedUntilFlush();
  } else {
-    ASSERT(read_disable_counter_ == 0, "readDisable called in between.");
    sequencer()->SetUnblocked();


SG, can you please comment that in code then? The other Envoy read enable/disable need parity (one unblock per block) so I think it's worth calling out that this is different.

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk

Awesome, thanks for your patience here :-)

envoyproxy#16128) Commit Message: use posted callback to block/unblock quic stream in EnvoyQuicStream::readDisable() with weak_ptr to stream to handle stream life time issue. Additional Description: currently if readDisabled() is called in decodeHeaders|Trailers(), the stream will be blocked right away which breaks the assumption QUICHE has that a stream shouldn't be blocked during OnInitialHeadersComplete() and OnTrailingHeadersComplete(). This change makes the stream state change completely outside of QUICHE call stack. (The unblocking is already outside of QUICHE call stack in existing implementation.) Also simplify the blockage state change logic. Risk Level: low Testing: added more unit tests and enabled Http2UpstreamIntegrationTest::ManyLargeSimultaneousRequestWithBufferLimits which was flaky. Part of envoyproxy#2557 envoyproxy#14829 Signed-off-by: Dan Zhang <[email protected]> Co-authored-by: Dan Zhang <[email protected]> Signed-off-by: Gokul Nair <[email protected]>

danzh1989 added 3 commits April 22, 2021 11:04

add tests for header and trailer cause read disable

24c30d2

Signed-off-by: Dan Zhang <[email protected]>

fix tests

c2c283a

Signed-off-by: Dan Zhang <[email protected]>

update doc

a1a1a85

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot assigned antoniovicente Apr 22, 2021

danzh1989 added 2 commits April 23, 2021 14:32

more time for TSAN

5bbeb54

Signed-off-by: Dan Zhang <[email protected]>

format

0a1fdab

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot assigned alyssawilk Apr 25, 2021

alyssawilk assigned DavidSchinazi Apr 26, 2021

alyssawilk reviewed Apr 26, 2021

View reviewed changes

alyssawilk added the waiting label Apr 26, 2021

convert to use SchedulableCallback

3ed42c4

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 27, 2021

antoniovicente reviewed Apr 27, 2021

View reviewed changes

use scheduleCallbackNextIteration

e9765bf

Signed-off-by: Dan Zhang <[email protected]>

DavidSchinazi previously approved these changes Apr 28, 2021

View reviewed changes

alyssawilk reviewed Apr 28, 2021

View reviewed changes

fix comments

fb2f88b

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 dismissed DavidSchinazi’s stale review via fb2f88b April 28, 2021 17:08

danzh1989 added 2 commits April 28, 2021 15:17

more comments

3fb743c

Signed-off-by: Dan Zhang <[email protected]>

more comments

0479a23

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk approved these changes Apr 29, 2021

View reviewed changes

alyssawilk merged commit 9e263d8 into envoyproxy:main Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quiche: handle stream blockage during decodeHeaders and decodeTrailers #16128

quiche: handle stream blockage during decodeHeaders and decodeTrailers #16128

danzh2010 commented Apr 22, 2021 •

edited

Loading

danzh2010 commented Apr 22, 2021

danzh2010 commented Apr 25, 2021

alyssawilk left a comment

alyssawilk Apr 26, 2021

antoniovicente Apr 26, 2021

danzh2010 Apr 27, 2021

alyssawilk Apr 26, 2021

danzh2010 Apr 27, 2021

alyssawilk Apr 28, 2021

danzh2010 Apr 28, 2021

alyssawilk Apr 28, 2021

danzh2010 Apr 28, 2021

antoniovicente Apr 27, 2021

danzh2010 Apr 28, 2021

DavidSchinazi left a comment

DavidSchinazi Apr 28, 2021

danzh2010 Apr 28, 2021

alyssawilk left a comment

alyssawilk Apr 28, 2021

alyssawilk left a comment

quiche: handle stream blockage during decodeHeaders and decodeTrailers #16128

quiche: handle stream blockage during decodeHeaders and decodeTrailers #16128

Conversation

danzh2010 commented Apr 22, 2021 • edited Loading

danzh2010 commented Apr 22, 2021

danzh2010 commented Apr 25, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidSchinazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

danzh2010 commented Apr 22, 2021 •

edited

Loading