http/1 and http/2: end to end flow control

[mattklein123]

Take over nghttp2 window updates and some stream/filter callbacks to apply back pressure when needed. For http/1 just stop reading from the socket.

[htuch]

@mattklein123 I'm interested in getting started on this issue. I'm wondering if you have any any context (beyond the need to avoid infinite buffering) or initial thoughts.

[mattklein123]

Here is my rough thinking on how to go about this (these are rough thoughts, feel free to propose alternatives once you start investigating). This is going to be a fairly complicated change. I think it would be good to break this up into small pieces just so that we can test things incrementally (that should not be that hard to do).

Changes to codec.h

https://github.com/lyft/envoy/blob/master/include/envoy/http/codec.h

IMO we should try to hide as much of the windowing stuff as possible from higher layers.

• On Stream interface, we will need a disableRead() and enableRead() (or variant). On HTTP/1 this will directly interface with the connection by disabling/enabling socket reads. On HTTP/2, this will work with the nghttp2 nghttp2_session_consume API. (As part of this change, we will need to switch to using nghttp2_option_set_no_auto_window_update). Roughly, for http/2, when enabled, we should just consume() data as we get it. When disabled, we keep track of how much data needs to be consumed, and then only consume it when we become enabled. When calling consume(), we need to flush pending frames to release window updates also. From an API perspective at this payer, disable/enable of read is eventually consistent. It might not happen right away and some number of reads might still happen.
• On the Stream interface, I would add: setWriteBufferHighWatermark() (or variant). In StreamCallbacks, we will need a callback for when we go above the high watermark as well as back below it.
• The default HTTP/2 window sizes need to be configurable. Right now we just generically open up to 256MiB. I don't have a strong opinion in v1 of this change as to whether we make this configurable on a global basis or per listener basis. We should probably keep the connection window large and do flow control at stream level. The default write high buffer watermark could also be set in the same default way.

Changes to connection.h

https://github.com/lyft/envoy/blob/master/include/envoy/network/connection.h

We are going to need a setWriteBufferHighWatermark() here as well as a set of callbacks. This will end up being used in the HTTP/1 case so that we can get notified of writes when we are above the high watermark.

Changes to connection_impl

We need to support real socket disable/enable for the HTTP/1 case. Right now we do a soft disable, mainly because it's much faster to get notified of remote close. If we upgrade to libevent 2.1, we can switch this behavior out for #391 so that we can use EPOLLRDHUP, which would be a lot better (there are other solutions also, can talk about this separately).

Changes to filter.h

https://github.com/lyft/envoy/blob/master/include/envoy/http/filter.h

Filters need to be notified of when the downstream connection goes above/below the high watermark. Additionally, filters will need to be able to enable/disable reading from the downstream stream.

• In StreamFilterCallbacks, much like reset, I would add the ability to both get notifications of above/below watermark events, as well as enable/disable read. NOTE: It would be technically more correct if enable/disable read actually propagated through the filter stack with the ability for a filter to pause (consume) this event, and then restart, but I do not think this extra complexity is worthwhile. I would just hit the underlying stream directly.

Other random changes

There will be various other changes required to router and a few other places to plumb through the callbacks and then wire it up back and forth. For this change we will also want end-to-end integration tests.

[htuch]

Thinking about this API a little, I'm wondering if we have a good way to determine what the per-stream memory use will actually be. Using a watermark is a little loosey-goosey as the actual memory use might exceed the watermark by an arbitrary amount due to the last send prior to when the send pipeline decides to stop sending due to the high watermark exceeded callback.

An alternative API would involve not always consuming the entire buffer in encodeData(), but only up to the available buffer space, where we add an upper bound to each buffer. This might be a bit more invasive based on various comments about the expectation that buffer slices are movable in their entirety.

Thoughts?

[mattklein123]

The problem with modifying encodeData() to do partial writes is not only that it will be an incredibly invasive change (I'm guessing many weeks to get it correct), but that I don't think it helps. In the case of a partial write, unless you reset the stream, you are going to have to buffer the data somewhere (in the router, etc.). So you are still at the whim of when window updates go out.

IMO, a rough memory maximum is sufficient for almost all use cases (potentially coupled with a safeguard where we stop handling new connections / fail HC when we go above some high memory watermark).

If a real memory guarantee is needed, I think we could do the following:

• Do the above API suggestions
• Do not consume read data until we actually write the data to the other side. Thus, memory for a stream is bounded by the receive window on the other side.
• I don't think this would be too much of a change to the above, and we could potentially support both models. (Pre-consume would be higher perf in some cases).

[htuch]

Yeah, partial writes don't help, but if we knew how much buffer space was available on the send path we could only consume from the kernel up to this amount and/or set the receive window appropriately in H2. I think this is something like what you're getting at with the last points above. In any case, this still wouldn't provide precise bounding, since any filter might change the size of data as it passes thru (e.g. a gzip filter).

I'll continue with your original API suggestion for now, but it would be good to have some way to know the number of streams that can be handled for a given amount of memory in the future.

[mattklein123]

One option that I considered is having encodeData (and write() on the connection) return the amount of data currently buffered. This would be a larger change, but could be used to drive dynamically changing window sizes... Something to consider in a follow up change? Not sure if we want to do that in v1.

[alyssawilk]

Commenting late in the game, the other advantage of the partial write over the current callback/notification driven design is only the parties which are actively contributing to buffer bloat are actually notified. My main concern with the existing design is if you have 100 streams on a connection, pushing the high and low watermark to all 100 is a bunch of extra work if a small subset of streams are the primary bandwidth hogs. It seems more efficient and elegant to have the just-in-time notification where writes are simply not accepted. But I'll buy Matt's explanation that we can potentially support both models and start with the one he thinks is more sane :-)