Refactoring the connection class to create buffers via factory by alyssawilk · Pull Request #1234 · envoyproxy/envoy

alyssawilk · 2017-07-10T19:35:25Z

This plus substantial refactors to the connection tests for code reuse, and some new tests using the new factories.

pulling this out from #1217 since it's well over 700 lines on its own.

Also did the read buffer along with the write buffer for consistency. Someone may need it some day and they shouldn't have to rewrite all the existing connection tests when they do.

Adding buffer factories for connection read/write buffers

mattklein123

overall very nice. Some quick comments. @dnoe can you also take a look at this one?

mattklein123 · 2017-07-10T23:27:58Z

include/envoy/buffer/buffer.h

+  virtual ~Factory() {}
+
+  /**
+   * Creates and returns a unique pointer to a new buffer.


nit: @return ... (trying to use doxygen for new stuff).

mattklein123 · 2017-07-10T23:29:46Z

include/envoy/event/dispatcher.h

+   * Optionally sets a custom buffer factory for the dispatcher.  This factory may be used by new
+   * connections on connection creation.
+   */
+  virtual void setBufferFactory(Buffer::FactoryPtr&& factory) PURE;


Do you think it's worth having a setter on the interface? Seems like the factory could just be passed to the constructor of DispatcherImpl and then we avoid leaking the abstraction for changing it?

This would allow changing the factory to one with different behavior at runtime, but I'm not sure if that would ever be useful.

Ah, I thought it'd be useful for consistency but I'm happy to remove it - with a bit of test cleanup I don't think it needs to be set at runtime.

mattklein123 · 2017-07-10T23:30:29Z

source/common/buffer/buffer_impl.h


+class OwnedImplFactory : public Factory {
+public:
+  InstancePtr create() override;


nit: // Buffer::Factory above this line.

mattklein123 · 2017-07-10T23:30:48Z

include/envoy/event/dispatcher.h

+  virtual void setBufferFactory(Buffer::FactoryPtr&& factory) PURE;
+
+  /**
+   * Returns the buffer factory for this dispatcher.


same @return doxygen comment here

mattklein123 · 2017-07-10T23:31:44Z

test/common/ssl/connection_impl_test.cc

-
-    Network::ListenerPtr listener = dispatcher.createSslListener(
-        connection_handler, *server_ctx, socket, listener_callbacks, stats_store,
+  void Initialize(uint32_t read_buffer_limit) {


nit: initialize (lower case)

mattklein123 · 2017-07-10T23:32:52Z

test/mocks/buffer/mocks.h

+  MOCK_METHOD2(move, void(Instance& rhs, uint64_t length));
+  MOCK_METHOD1(drain, void(uint64_t size));
+
+  void BaseMove(Instance& rhs) { Buffer::OwnedImpl::move(rhs); }


nit: lower case for method name start all through this class.

Ironically I reread the style guide before sending this one out but Google3 style habits are so hard to break :-P

mattklein123 · 2017-07-10T23:33:41Z

test/mocks/buffer/mocks.h


 namespace Envoy {
+
+class MockBuffer : public Buffer::OwnedImpl {


nice addition, thanks.

mattklein123 · 2017-07-10T23:37:38Z

test/mocks/buffer/mocks.h

+    return bytes_written;
+  }
+
+  int FailWrite(int) {


Is this called? Not quite sure how it works?

It's used in the other CL to force writes to "fail" and data to be buffered in the connection (eventually going over watermarks).

I'll remove it from this one since it's not currently used.

If it's meant for testing only the usual convention is to name the method something like failWriteForTest(). This doesn't appear to be in the style doc, I'll create a PR that mentions this convention.

dnoe · 2017-07-11T02:13:16Z

include/envoy/event/dispatcher.h

+   * Optionally sets a custom buffer factory for the dispatcher.  This factory may be used by new
+   * connections on connection creation.
+   */
+  virtual void setBufferFactory(Buffer::FactoryPtr&& factory) PURE;


This would allow changing the factory to one with different behavior at runtime, but I'm not sure if that would ever be useful.

dnoe · 2017-07-11T02:23:02Z

source/common/network/connection_impl.h

  // Network::BufferSource
-  Buffer::Instance& getReadBuffer() override { return read_buffer_; }
+  Buffer::Instance& getReadBuffer() override { return *read_buffer_; }
  Buffer::Instance& getWriteBuffer() override { return *current_write_buffer_; }


There's both a write_buffer_ and a current_write_buffer_. write_buffer_ is initialized via factory, but current_write_buffer_ is not (in fact it looks to me like it's always nullptr, so this line seems especially odd). This isn't a line changed in this PR, but it would be good to clarify...

@mattklein123 any idea what current_write_buffer_ was supposed to be for?

It's set in the cc file right before fitler_manager_.onWrite and cleared right after. There's a note that it's a bit of a hack to provide access under the stack of that call :-)

Yeah, that whole situation is not great but per @alyssawilk the comment should have the details.

dnoe · 2017-07-11T02:33:13Z

test/mocks/event/mocks.h

  MOCK_METHOD1(post, void(std::function<void()> callback));
  MOCK_METHOD1(run, void(RunType type));
+  Buffer::Factory& getBufferFactory() override { return *buffer_factory_; }
+  void setBufferFactory(Buffer::FactoryPtr&& factory) override {


Is this only needed for the tests?

dnoe

LGTM if @mattklein123 also approves

mattklein123

very nice, thanks.

Automatic merge from submit-queue. Add notification for duplication **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note ``` Add notification to match istio/istio#4358

Description: This tool was meant to be a bazel implementation detail so its public accessor was removed in bazelbuild/bazel@f8b51ff Using the cc API from bazel we can dynamically get this. This is still a bit of a hack because we're writing our own args for this. We might be able to fetch the args, based on bazel's crosstool, dynamically as well, but ideally we remove this rule instead. Fixes envoyproxy/envoy-mobile#1233 Signed-off-by: Keith Smiley <keithbsmiley@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This updates the Go version used here as well as some Go dependencies. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

dnoe and others added 5 commits July 10, 2017 15:23

Moving the test rewrites for pr/1217 out of the change.

48f6ecd

Adding buffer factories for connection read/write buffers

adding in the client connection

5494dac

Merge branch 'refs/heads/master' into buffer-management

b408a89

actually removing replaceWriteBufferForTest from both tests!

a48a52c

Fixing move semantics per style guide.

9da7330

alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Jul 10, 2017

Addressing the rest of the comments not handled by envoyproxy#1234

8fd62ca

alyssawilk mentioned this pull request Jul 10, 2017

Tcp flow control #1217

Merged

mattklein123 reviewed Jul 10, 2017

View reviewed changes

mattklein123 requested a review from dnoe July 10, 2017 23:41

dnoe reviewed Jul 11, 2017

View reviewed changes

alyssawilk added 3 commits July 11, 2017 09:25

review cleanup

591b76d

removing an unused function

59362e9

Merge branch 'refs/heads/master' into buffer-management

61276bb

dnoe approved these changes Jul 12, 2017

View reviewed changes

mattklein123 approved these changes Jul 12, 2017

View reviewed changes

mattklein123 merged commit e0c1d7d into envoyproxy:master Jul 12, 2017

alyssawilk deleted the buffer-management branch July 20, 2017 14:58

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

deps: use Go 1.25.1 & update libs (#1234)

68d0203

**Description** This updates the Go version used here as well as some Go dependencies. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>


		namespace Envoy {

		class MockBuffer : public Buffer::OwnedImpl {

Conversation

alyssawilk commented Jul 10, 2017

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!

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!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants