Introduce listener TCP connection buffer configuration and implement … by htuch · Pull Request #558 · envoyproxy/envoy

htuch · 2017-03-10T01:43:49Z

…read buffer limits (#150).

Baby steps towards flow control. This patch plumbs the TCP connection buffer limits to the listener
and enforces it on the read buffer only. Later patches will also plumb to the upstream cluster
connection and cover the write buffer (which requires the watermark API).

…read buffer limits (envoyproxy#150). Baby steps towards flow control. This patch plumbs the TCP connection buffer limits to the listener and enforces it on the read buffer only. Later patches will also plumb to the upstream cluster connection and cover the write buffer (which requires the watermark API).

mattklein123 · 2017-03-10T21:16:06Z

I'm trying to unbury myself. I will look at this later today or this weekend.

ccaraman · 2017-03-10T21:23:19Z

docs/configuration/listeners/listeners.rst

  connection is handled by the listener that receives it. Default is false.

+per_connection_buffer_limit_bytes
+  *(optional, integer)* Soft limit on size of the listener's new connection read and write buffers. If unspecified, an implementation defined default is applied.


Please specify the default value here.

ccaraman · 2017-03-10T21:24:12Z

source/common/network/connection_impl.h

+    return read_buffer_limit_ > 0 && read_buffer_.length() >= read_buffer_limit_;
+  }
+  // Mark read buffer ready to read in the event loop. This is used when yielding following
+  // shouldDrainReadBuffer(). TODO(htuch): While this is the basis for also yielding to other


nit: put todo on new line.

mattklein123

Looks good. A few (annoying, sorry) comments.

mattklein123 · 2017-03-11T03:29:56Z

docs/configuration/listeners/listeners.rst

  connection is handled by the listener that receives it. Default is false.

+per_connection_buffer_limit_bytes
+  *(optional, integer)* Soft limit on size of the listener's new connection read and write buffers. If unspecified, an implementation defined default is applied (1MB).


nit: 100 col line break

mattklein123 · 2017-03-11T03:31:29Z

include/envoy/event/dispatcher.h

                                              Network::ListenerCallbacks& cb,
                                              Stats::Store& stats_store, bool bind_to_port,
-                                              bool use_proxy_proto, bool use_orig_dst) PURE;
+                                              bool use_proxy_proto, bool use_orig_dst,


Sorry, to ask you to do this, but I have a feeling we are going to keep adding params/options here. (I know soon we will split use_original_dst into use_orginal_port/use_original_address, etc.). Can we define a struct ListenerOptions and just pass that. That way we don't have to change a gazillion mocks and callsites the next time we add a param.

mattklein123 · 2017-03-11T03:32:28Z

include/envoy/event/dispatcher.h

                                              Stats::Store& stats_store, bool bind_to_port,
-                                              bool use_proxy_proto, bool use_orig_dst) PURE;
+                                              bool use_proxy_proto, bool use_orig_dst,
+                                              size_t per_connection_buffer_limit_bytes) PURE;


Instead of size_t, can we be specific with either uint32_t or uint64_t (probably uint32_t). Same applies all the other places we reference this.

mattklein123 · 2017-03-11T03:33:56Z

source/common/json/config_schemas.cc

       "use_proxy_proto" : {"type" : "boolean"},
-       "use_original_dst" : {"type" : "boolean"}
+       "use_original_dst" : {"type" : "boolean"},
+       "per_connection_buffer_limit_bytes" : {


"per_connection_read_buffer_limit_bytes" ? Next we will have write high/low watermark so might get confusing?

I was planning on using per_connection_buffer_limit_bytes to drive both the watermark for the write buffer (automatically setting low watermark at a fraction like 0.5) and the read buffer, under the assumption that we want to keep config simple and that read/write buffer limits should be somewhat symmetric in general. I know there are scenarios where this isn't true, but this could be reasonable for v1.

OK sounds good.

mattklein123 · 2017-03-13T19:14:10Z

include/envoy/network/connection.h

+   * Set a soft limit on the size of the read buffer prior to flushing to further stages in the
+   * processing pipeline.
+   */
+  virtual void setReadBufferLimit(size_t limit) PURE;


mattklein123 · 2017-03-13T19:14:28Z

include/envoy/network/listener.h

+  static ListenerOptions listenerOptionsWithBindToPort() {
+    return {.bind_to_port_ = true,
+            .use_proxy_proto_ = false,
+            .use_original_dst_ = false,


mattklein123 · 2017-03-13T19:15:27Z

source/common/network/connection_impl.h

  Address::InstancePtr local_address_;
  Buffer::OwnedImpl read_buffer_;
  Buffer::OwnedImpl write_buffer_;
+  size_t read_buffer_limit_ = 0;


mattklein123 · 2017-03-13T19:16:04Z

source/common/network/connection_impl.h

  Ssl::Connection* ssl() override { return nullptr; }
  State state() override;
  void write(Buffer::Instance& data) override;
+  void setReadBufferLimit(size_t limit) override { read_buffer_limit_ = limit; }


mattklein123 · 2017-03-13T19:16:55Z

source/common/network/listener_impl.h

  const bool use_proxy_proto_;
  ProxyProtocol proxy_protocol_;
  const bool use_original_dst_;
+  const size_t per_connection_buffer_limit_bytes_;


Just have a ListenerOptions as a member var and then you can just copy it in via passed in options.

Signed-off-by: Shikugawa <Shikugawa@gmail.com>

Because we shared the `release` bazel config between both iOS and Android, Android builds were incorrectly having llvm bitcode enabled for them. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

…lang-org group across 1 directory (#558)

ccaraman reviewed Mar 10, 2017

View reviewed changes

htuch added 2 commits March 10, 2017 17:10

@ccaraman review feedback.

a7b3478

Some configuration tests for the per connection buffer limit.

5df5456

mattklein123 reviewed Mar 11, 2017

View reviewed changes

htuch added 2 commits March 13, 2017 14:03

@mattklein123 review feedback.

9c66fb6

Merge remote-tracking branch 'upstream/master' into flow-control-config

0b27b80

mattklein123 reviewed Mar 13, 2017

View reviewed changes

Further @mattklein123 review feedback.

0f14891

mattklein123 approved these changes Mar 13, 2017

View reviewed changes

mattklein123 merged commit fd58242 into envoyproxy:master Mar 13, 2017

htuch deleted the flow-control-config branch March 14, 2017 15:09

mattklein123 mentioned this pull request Mar 18, 2017

SSL connection write event crash #585

Closed

lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 21, 2020

bazel: wasm to wasm32 (envoyproxy#558)

1fc841d

Signed-off-by: Shikugawa <Shikugawa@gmail.com>

mathetake pushed a commit that referenced this pull request Mar 3, 2026

build(deps): bump golang.org/x/oauth2 from 0.28.0 to 0.29.0 in the go…

67c9c55

…lang-org group across 1 directory (#558)

Conversation

htuch commented Mar 10, 2017

Uh oh!

mattklein123 commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants