quiche: prevent nested buffer watermark accounting by danzh2010 · Pull Request #14259 · envoyproxy/envoy

danzh2010 · 2020-12-03T16:50:16Z

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

Commit Message: Refactor watermark accounting logic during encodeHeaders|Data|Trailers() by wrapping the code counting buffer size before and after into a scoped object.

Additional Description: The above change fix 2problems:

When the stream gets reset or connection gets closed during the scope of updating watermark buffer, the watermark accounting clean-up was nested in the another buffer updating;
In gQUIC, QuicConnection::OnCanWrite() counts buffered bytes change twice against connection buffer watermark if OnCanWrite() triggers encodeHeaders().

Risk Level: low
Testing: added stream unit tests to test connection close during WriteHeaders() and WriteBodySlices() and session unit tests to test OnCanWrite() triggers encodeHeaders();

Part of #2557

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

alyssawilk · 2020-12-14T14:29:59Z

Hey Dan, I think this didn't get a reviewer because it's tagged as a draft. Want to ping when it's ready for review?

github-actions · 2021-01-14T00:40:18Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-01-23T00:37:03Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

danzh2010 · 2021-01-25T18:49:58Z

Hey Dan, I think this didn't get a reviewer because it's tagged as a draft. Want to ping when it's ready for review?

I need to wait for #14246 to get in as this PR branches from it.

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

danzh2010 · 2021-01-27T20:36:55Z

Can someone reopen this PR?

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

danzh2010 · 2021-03-24T02:40:29Z

/retest

repokitteh-read-only · 2021-03-24T02:40:33Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14259 (comment) was created by @danzh2010.

see: more, trace.

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

danzh2010 · 2021-03-24T05:11:46Z

/retest

repokitteh-read-only · 2021-03-24T05:11:50Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14259 (comment) was created by @danzh2010.

see: more, trace.

alyssawilk · 2021-03-24T13:53:59Z

still needs main merge?

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

danzh2010 · 2021-03-24T15:30:25Z

still needs main merge?

sync'ed with main

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

danzh2010 · 2021-03-25T14:16:04Z

Fixed a bug in quic pause filter. PTAL

alyssawilk · 2021-03-25T14:54:35Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

can we just have a test isTsan() utilitiy?

I added a macro TSAN_TIMEOUT_FACTOR for these 3 places.

alyssawilk · 2021-03-25T14:55:12Z

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

We can add this as a function and use in both places like setRouteUsingWebsocket()

bazel/external/quiche.BUILD

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

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

danzh2010 · 2021-03-29T15:14:49Z

Ping?

alyssawilk

Please do remember I'm out Fridays so don't do reviews that day :-)

mattklein123 · 2021-03-29T20:01:42Z

Sorry can you merge main?

/wait

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

danzh2010 · 2021-03-30T15:00:33Z

/retest

repokitteh-read-only · 2021-03-30T15:00:38Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14259 (comment) was created by @danzh2010.

see: more, trace.

mattklein123

Thanks!

danzh1989 added 5 commits December 1, 2020 15:17

add 100 continue

00c5734

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

add tests

5bafa4a

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

watermark buffer

c47f1e3

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

change OnCanWrite

d76a34d

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

spell

9858629

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

alyssawilk self-assigned this Dec 14, 2020

alyssawilk added the waiting:any label Dec 14, 2020

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 14, 2021

Base automatically changed from master to main January 15, 2021 23:02

github-actions bot closed this Jan 23, 2021

repokitteh-read-only bot removed the waiting:any label Jan 25, 2021

danzh1989 added 3 commits January 27, 2021 14:59

Merge branch 'master' into headertoolarge

57dc790

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

resolve conflict

a418bce

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

dedup tests

b4a71b8

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

alyssawilk reopened this Jan 28, 2021

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 28, 2021

danzh1989 added 4 commits February 1, 2021 23:50

send buffer monitor

40bff0d

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

runResetCallbacks before close

26643a7

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

watermark test

a7ac709

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

typo

123fe04

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

danzh2010 marked this pull request as ready for review February 3, 2021 05:44

danzh2010 requested review from alyssawilk, ggreenway and mattklein123 as code owners February 3, 2021 05:44

typo

9ce374c

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

increase upstream_response_timout

6427445

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

danzh2010 dismissed alyssawilk’s stale review via 6427445 March 24, 2021 01:29

Merge branch 'master' into headertoolarge

60acf3f

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

Merge branch 'master' into headertoolarge

dcfb8dc

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

danzh1989 added 2 commits March 24, 2021 15:06

adjust buffer limit for protocol_integration_test to pass

772903a

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

fix pause filter

e7cdb6e

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

alyssawilk reviewed Mar 25, 2021

View reviewed changes

alyssawilk added the waiting label Mar 25, 2021

tsan common macro

ae80ff2

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

repokitteh-read-only bot removed the waiting label Mar 25, 2021

alyssawilk previously approved these changes Mar 29, 2021

View reviewed changes

alyssawilk assigned mattklein123 Mar 29, 2021

mattklein123 added the waiting label Mar 29, 2021

Merge branch 'master' into headertoolarge

88112fd

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

danzh2010 dismissed alyssawilk’s stale review via 88112fd March 30, 2021 03:52

repokitteh-read-only bot removed the waiting label Mar 30, 2021

fix format

cb054e6

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

mattklein123 approved these changes Mar 30, 2021

View reviewed changes

mattklein123 merged commit 964e2a0 into envoyproxy:main Mar 30, 2021

Conversation

danzh2010 commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Dec 14, 2020

Uh oh!

github-actions bot commented Jan 14, 2021

Uh oh!

github-actions bot commented Jan 23, 2021

Uh oh!

danzh2010 commented Jan 25, 2021

Uh oh!

danzh2010 commented Jan 27, 2021

Uh oh!

danzh2010 commented Mar 24, 2021

Uh oh!

repokitteh-read-only bot commented Mar 24, 2021

Uh oh!

danzh2010 commented Mar 24, 2021

Uh oh!

repokitteh-read-only bot commented Mar 24, 2021

Uh oh!

alyssawilk commented Mar 24, 2021

Uh oh!

danzh2010 commented Mar 24, 2021

Uh oh!

danzh2010 commented Mar 25, 2021

Uh oh!

alyssawilk Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

danzh2010 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

danzh2010 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danzh2010 commented Mar 29, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Mar 29, 2021

Uh oh!

danzh2010 commented Mar 30, 2021

Uh oh!

repokitteh-read-only bot commented Mar 30, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danzh2010 commented Dec 3, 2020 •

edited

Loading