-
Notifications
You must be signed in to change notification settings - Fork 5.4k
H2 encode flow control #1328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
H2 encode flow control #1328
Changes from all commits
7e2fec9
c3629cb
0437eca
2d86666
f0cb2d0
ebc2760
7bf8073
d6dbeda
1f3d7cc
0449209
76384ce
46fd713
09975b0
f3346f7
e46933e
6a6774a
f3a4908
e6b5236
46d12c5
44d9ede
79ff9b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,22 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| * @param trailers supplies the trailers to encode. | ||
| */ | ||
| virtual void encodeTrailers(HeaderMapPtr&& trailers) PURE; | ||
|
|
||
| /** | ||
| * Called when the buffer for a decoder filter or any buffers the filter sends data to go over | ||
| * their high watermark. | ||
| * | ||
| * In the case of a filter such as the router filter, which spills into multiple buffers (codec, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you share this comment with the other variants below? I'm thinking we might want to take the design doc and digest it down to a Markdown file explaining what is actually implemented, and example sequences of going above and below to convey the intuition of when callbacks are fired etc. Maybe you can figure out a better way to convey the intuition in comment form, up to you.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having read more of the PR, I think a MD file explaining the connection + stream level design stuff, and then real examples of callback sequences on different events/overflows would be super helpful. Looking through the PR locally, it looks very solid, but I'm having a hard time connecting all the dots staring at the code, some worked examples or diagrams if you prefer would be great. |
||
| * connection etc.) this may be called multiple times. Any such filter is responsible for calling | ||
| * the low watermark callbacks an equal number of times as the respective buffers are drained. | ||
| */ | ||
| virtual void onDecoderFilterAboveWriteBufferHighWatermark() PURE; | ||
|
|
||
| /** | ||
| * Called when a decoder filter or any buffers the filter sends data to go from over its high | ||
| * watermark to under its low watermark. | ||
| */ | ||
| virtual void onDecoderFilterBelowWriteBufferLowWatermark() PURE; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -297,6 +313,16 @@ class StreamEncoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| * It is an error to call this method in any other case. | ||
| */ | ||
| virtual void addEncodedData(Buffer::Instance& data) PURE; | ||
|
|
||
| /** | ||
| * Called when an encoder filter goes over its high watermark. | ||
| */ | ||
| virtual void onEncoderFilterAboveWriteBufferHighWatermark() PURE; | ||
|
|
||
| /** | ||
| * Called when a encoder filter goes from over its high watermark to under its low watermark. | ||
| */ | ||
| virtual void onEncoderFilterBelowWriteBufferLowWatermark() PURE; | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -988,6 +988,20 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::encodeTrailers(HeaderMapP | |
| parent_.encodeTrailers(nullptr, *parent_.response_trailers_); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter:: | ||
| onDecoderFilterAboveWriteBufferHighWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Read-disabling downstream stream due to filter callbacks.", parent_); | ||
| parent_.response_encoder_->getStream().readDisable(true); | ||
| parent_.connection_manager_.stats_.named_.downstream_flow_control_paused_reading_total_.inc(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattklein123 Do you think it's also worth adding the source-cluster of the flow control backup or is that overkill?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have this in upstream stats, so I think this is overkill (see my comment about router stats). Router should be hitting the cluster stats in this case IMO. |
||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter:: | ||
| onDecoderFilterBelowWriteBufferLowWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Read-enabling downstream stream due to filter callbacks.", parent_); | ||
| parent_.response_encoder_->getStream().readDisable(false); | ||
| parent_.connection_manager_.stats_.named_.downstream_flow_control_resumed_reading_total_.inc(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamEncoderFilter::addEncodedData(Buffer::Instance& data) { | ||
| return parent_.addEncodedData(*this, data); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a later implementation, have you thought about using distance from watermark as a way to size the H2 stream flow control window?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would avoid the double buffer and given the proximity between buffer and data source I think it'd be a nice clean thing to do, but it would make this one buffer + data source combination special and unusual so I'm not convinced it'd be worth doing unless I get to do the full flow control redesign we've talked about in person where everything seemlessly avoids the double buffering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean by rejiggering everything so that encode* returns the number of bytes accepted, etc.? That's certainly doable, though, buffering still will need to happen since whoever fails to write will need to buffer locally until they can write. This pushes more complexity out to the leaves. Not sure it's worth it, but I guess we can see how things go with this approach.