Limit initial window size increases and per-stream window delta #26342

yashykt · 2021-05-21T20:58:30Z

Internal bug reference b/179093825

gRPC Core at present does not consider the flow control windows of existing active streams when sending initial window updates. This can cause flow control windows to go beyond the max limit of 2^31-1 as determined by the HTTP2 spec. This PR fixes that by adding -

an upper limit of 1 GB on the maximum initial window size
an upper limit of 1 MB on per-stream window deltas

… of active streams

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

ctiller · 2021-06-18T19:24:52Z

src/core/ext/transport/chttp2/transport/flow_control.h

@@ -309,7 +309,7 @@ class TransportFlowControl final : public TransportFlowControlBase {
    return action;
  }

-  const grpc_chttp2_transport* const t_;


Iirc it's because grpc_chttp2_stream_map_for_each requires a non-const field. I can probably write a const version of it too though

I think that would be preferable... just to limit the scope of potential mutations.

Maybe we can track some kind of crude approximation and recheck iff that approximation says we'd overflow?

Is this really that much though? Internally, I've seen 100 as the number being thrown around as the configuration for maximum number of concurrent streams?

If it is an issue, we could probably say something like - the initial window size can only grow as much as half the maximum flow control window and a similar restriction on the window delta.

ctiller · 2021-06-18T19:25:56Z

src/core/ext/transport/chttp2/transport/flow_control.cc

+// delta. Min has a ceiling of 0, while max has a floor of 0.
+WindowDelta ComputeLocalWindowDeltaBoundaries(grpc_chttp2_transport* t) {
+  WindowDelta delta;
+  grpc_chttp2_stream_map_for_each(&t->stream_map,


does this change the update from O(1) to O(N)?

Yes it does unfortunately. I wonder if there is a way to do this without adding an O(N) loop

I wonder too... as it is this feels a little risky in terms of CPU usage.

yashykt

Also, there is a remaining change that I'm trying to push but running into proxy issues. The change is to do with also limiting stream flow control window updates.

(PS: Thanks for taking a look :))

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

yashykt · 2021-06-18T21:11:30Z

src/core/ext/transport/chttp2/transport/flow_control.cc

+// delta. Min has a ceiling of 0, while max has a floor of 0.
+WindowDelta ComputeLocalWindowDeltaBoundaries(grpc_chttp2_transport* t) {
+  WindowDelta delta;
+  grpc_chttp2_stream_map_for_each(&t->stream_map,


Yes it does unfortunately. I wonder if there is a way to do this without adding an O(N) loop

yashykt · 2021-06-18T21:12:32Z

src/core/ext/transport/chttp2/transport/flow_control.h

@@ -309,7 +309,7 @@ class TransportFlowControl final : public TransportFlowControlBase {
    return action;
  }

-  const grpc_chttp2_transport* const t_;


Iirc it's because grpc_chttp2_stream_map_for_each requires a non-const field. I can probably write a const version of it too though

ctiller · 2021-07-27T00:32:53Z

I'm super uncomfortable with expanding this mechanism to become O(n) - especially since doing so will make the flow control mechanism that we use when under duress more expensive.

Instead, can I propose we split the flow control windows into two budgets: one for initial window, the other per stream, and we bound each to something extreme but sensible (order 100MB springs to mind, 10MB may be a better default).

At that point, we disallow advertising over budget, eliminating the possibility of this condition occuring.

yashykt · 2021-08-04T00:24:42Z

@ctiller PTALA

ctiller

This is fairly difficult to reason through whether it's correct.
Can you please write a unit test demonstrating that it has the desired behaviors?

yashykt · 2021-08-04T18:51:22Z

Tests added. (There is an internal stress test that helps to verify the behavior too.)

ctiller · 2021-08-04T19:08:46Z

My memory was that we had an isolated unit test for flow_control.cc, but searching history reveals no such test - perhaps it was started and never completed. I don't think there's any sense in blocking this work on that right now, but it seems this code is ripe for an explicit test that says "if this state changes, this action is performed" so that we can track changes in behavior here.

yashykt · 2021-08-09T16:05:25Z

Known issues: #26595

…#26342) * Limit initial window size increases/decreases by flow control windows of active streams * Limit stream flow control window updates to maximum allowable * Reviewer comments * Alternative way * Clean-up * Add tests * Remove unnecessary cq_verifier * Generate projects * Initialize recv_message * Get around compilation issue * Test size large

…ta (grpc#26342)" This reverts commit 3e197ac.

…#26342) * Limit initial window size increases/decreases by flow control windows of active streams * Limit stream flow control window updates to maximum allowable * Reviewer comments * Alternative way * Clean-up * Add tests * Remove unnecessary cq_verifier * Generate projects * Initialize recv_message * Get around compilation issue * Test size large

yashykt added release notes: yes Indicates if PR needs to be in release notes lang/core labels May 21, 2021

yashykt force-pushed the chttp2windowbug branch from 51e2ae4 to e1907ff Compare May 21, 2021 21:28

Limit initial window size increases/decreases by flow control windows…

a1bdfeb

… of active streams

yashykt force-pushed the chttp2windowbug branch from e1907ff to a1bdfeb Compare June 18, 2021 19:12

ctiller reviewed Jun 18, 2021

View reviewed changes

Limit stream flow control window updates to maximum allowable

adc9df5

yashykt commented Jun 18, 2021

View reviewed changes

yashykt added 2 commits July 23, 2021 12:42

Merge branch 'master' into chttp2windowbug

eaef5a0

Reviewer comments

411b6e1

yashykt added 2 commits August 3, 2021 12:31

Merge branch 'master' into chttp2windowbug

6f813bc

Alternative way

2433674

yashykt force-pushed the chttp2windowbug branch from e9772cb to 2433674 Compare August 3, 2021 23:23

Clean-up

249c97f

ctiller reviewed Aug 4, 2021

View reviewed changes

yashykt force-pushed the chttp2windowbug branch from 86e52a4 to 72022d0 Compare August 4, 2021 18:24

Add tests

f916c41

yashykt force-pushed the chttp2windowbug branch from 72022d0 to f916c41 Compare August 4, 2021 18:26

yashykt changed the title ~~Limit initial window size increases by flow control windows of active streams~~ Limit initial window size increases and per-stream window delta Aug 4, 2021

ctiller approved these changes Aug 4, 2021

View reviewed changes

yashykt added 5 commits August 4, 2021 22:57

Remove unnecessary cq_verifier

2f404d1

Generate projects

aa6c7c2

Initialize recv_message

9dea229

Get around compilation issue

1cf4ff4

Test size large

45f8498

yashykt merged commit 3e197ac into grpc:master Aug 9, 2021

veblush added a commit to veblush/grpc that referenced this pull request Sep 9, 2021

Revert "Limit initial window size increases and per-stream window del…

250d9c7

…ta (grpc#26342)" This reverts commit 3e197ac.

yashykt mentioned this pull request Sep 9, 2021

HTTP2: Set per-stream flow control window delta limit to 1MB #27298

Merged

yashykt deleted the chttp2windowbug branch May 18, 2023 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit initial window size increases and per-stream window delta #26342

Limit initial window size increases and per-stream window delta #26342

yashykt commented May 21, 2021 •

edited

ctiller Jun 18, 2021

yashykt Jun 18, 2021

ctiller Jun 18, 2021

ctiller Jun 18, 2021

yashykt Jul 23, 2021

ctiller Jun 18, 2021

yashykt Jun 18, 2021

ctiller Jun 18, 2021

yashykt left a comment

yashykt Jun 18, 2021

yashykt Jun 18, 2021

ctiller commented Jul 27, 2021

yashykt commented Aug 4, 2021

ctiller left a comment

yashykt commented Aug 4, 2021

ctiller commented Aug 4, 2021

yashykt commented Aug 9, 2021

Limit initial window size increases and per-stream window delta #26342

Limit initial window size increases and per-stream window delta #26342

Conversation

yashykt commented May 21, 2021 • edited

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

yashykt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctiller commented Jul 27, 2021

yashykt commented Aug 4, 2021

ctiller left a comment

Choose a reason for hiding this comment

yashykt commented Aug 4, 2021

ctiller commented Aug 4, 2021

yashykt commented Aug 9, 2021

yashykt commented May 21, 2021 •

edited