Add an unused implementation NoOpTransportSocketCallbacks #4181

danzh2010 · 2018-08-16T15:23:22Z

Signed-off-by: Dan Zhang [email protected]

Description:
Add an implementation of TransportSocketCallbacks which does nothing when setReadBufferReady() or raiseEvent() is call. This implementation is supposed to be used in TransportSocket implementation which wraps another socket object. In this case, the underlying socket's callbacks should be suppressed.

Also add callbacks() in TransportSocket interface which is used to get the callbacks passed to setTransportSocketCallbacks().

Risk Level: Low (not enabled in main)
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: N/A

setTransportSocketCallbacks(). Add a new class NoOpTransportSocketCallbacks. Add a utility class NoOpTransportSocketCallbacks to hold back callbacks from underlying socket in a TransportSocket implementation which wraps another socket. move files around Signed-off-by: Dan Zhang <[email protected]> add some implementation to mock class Signed-off-by: Dan Zhang <[email protected]> format Signed-off-by: Dan Zhang <[email protected]> header order Signed-off-by: Dan Zhang <[email protected]> format Signed-off-by: Dan Zhang <[email protected]>

Signed-off-by: Dan Zhang <[email protected]>

lizan · 2018-08-16T20:34:14Z

source/extensions/transport_sockets/alts/noop_transport_socket_callbacks.h

+ */
+class NoOpTransportSocketCallbacks : public Network::TransportSocketCallbacks {
+public:
+  explicit NoOpTransportSocketCallbacks(Network::TransportSocket* parent) : parent_(parent) {}


#4172 (comment)

Yeah if you delay creating TransportSocketCallbacks, i.e. in parent setTransportSocketCallbacks, you should be able to just take TransportSocketCallbacks& here.

That's a good point! Done.
I still keep the callbacks() in TransportSocket interface, as it's convenient to access to callbacks_ for underlying socket.

Signed-off-by: Dan Zhang <[email protected]>

lizan · 2018-08-16T23:25:19Z

test/mocks/network/mocks.h

@@ -428,7 +428,9 @@ class MockTransportSocket : public TransportSocket {
  MockTransportSocket();
  ~MockTransportSocket();

-  MOCK_METHOD1(setTransportSocketCallbacks, void(TransportSocketCallbacks& callbacks));
+  void setTransportSocketCallbacks(TransportSocketCallbacks& callbacks) override {


Keep this (and callbacks below) as a MOCK and do ON_CALL(...).WillByDefault in constructor like:
https://github.com/envoyproxy/envoy/pull/4153/files#diff-f1d609046babf8223367556fbdda1721R206

This will allow test to do EXPECT_CALL etc.

lizan · 2018-08-16T23:28:06Z

include/envoy/network/transport_socket.h

+  /**
+   * @return the TransportSocketCallbacks object passed in through setTransportSocketCallbacks().
+   */
+  virtual TransportSocketCallbacks* callbacks() PURE;


I would remove this as I think where you need get callbacks is in the scope you set it. No strong opinion on it though. If this is needed internally, ping me on hangout.

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2018-08-20T17:27:55Z

Ping?

lizan

LGTM modulo one comment.

lizan · 2018-08-20T21:06:43Z

test/extensions/transport_sockets/alts/noop_transport_socket_callbacks_test.cc

+
+class TestTransportSocketCallbacks : public Network::TransportSocketCallbacks {
+public:
+  TestTransportSocketCallbacks(Network::Connection* connection) : connection_(connection) {}


if connection_ couldn't be null, make it a reference. also make constructor explicit

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk · 2018-08-21T14:04:57Z

source/extensions/transport_sockets/alts/noop_transport_socket_callbacks.h

+   * No-op for these two methods to hold back the callbacks.
+   */
+  void setReadBufferReady() override {}
+  void raiseEvent(Network::ConnectionEvent) override {}


I'm fine with this landing as is, but can you please loop me in when it is used? I have some concerns about these particular events being swallowed. Maybe we can add ASSERTs that they're not used so we'll at least catch it in debug builds if a mistake is made? Hopefully seeing where the code is used will allay my concerns :-)

@alyssawilk I will merge master and use this in my PR #4153, ASSERT cannot be added here. In #4153, it have to swallow Connected event from RawBufferSocket, and raise it when handshake is done.

danzh1989 added 2 commits August 16, 2018 10:36

format again

dffd2d1

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 requested a review from lizan as a code owner August 16, 2018 15:23

mattklein123 assigned lizan Aug 16, 2018

danzh1989 added 2 commits August 16, 2018 12:00

add override

d1a9c25

Signed-off-by: Dan Zhang <[email protected]>

add back BUILD file

a85ec49

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 force-pushed the final branch from 7eb18fe to a85ec49 Compare August 16, 2018 16:05

danzh1989 added 2 commits August 16, 2018 12:38

add override

b8605bd

Signed-off-by: Dan Zhang <[email protected]>

fix connection_impl_test

41f9978

Signed-off-by: Dan Zhang <[email protected]>

lizan reviewed Aug 16, 2018

View reviewed changes

danzh1989 added 2 commits August 16, 2018 17:57

remove socket pointer

d97d11f

Signed-off-by: Dan Zhang <[email protected]>

format

dd1f97a

Signed-off-by: Dan Zhang <[email protected]>

lizan reviewed Aug 16, 2018

View reviewed changes

danzh1989 added 3 commits August 17, 2018 10:50

mock callbacks()

2bb3bbe

Signed-off-by: Dan Zhang <[email protected]>

remove callback from interface

d2755dc

Signed-off-by: Dan Zhang <[email protected]>

add back constructor

de8caf3

Signed-off-by: Dan Zhang <[email protected]>

lizan reviewed Aug 20, 2018

View reviewed changes

use reference

f0aeb4a

Signed-off-by: Dan Zhang <[email protected]>

lizan approved these changes Aug 20, 2018

View reviewed changes

lizan requested a review from alyssawilk August 20, 2018 22:34

alyssawilk reviewed Aug 21, 2018

View reviewed changes

alyssawilk approved these changes Aug 21, 2018

View reviewed changes

alyssawilk merged commit 91558cd into envoyproxy:master Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an unused implementation NoOpTransportSocketCallbacks #4181

Add an unused implementation NoOpTransportSocketCallbacks #4181

danzh2010 commented Aug 16, 2018 •

edited

Loading

lizan Aug 16, 2018

danzh2010 Aug 16, 2018

lizan Aug 16, 2018 •

edited

Loading

danzh2010 Aug 17, 2018

lizan Aug 16, 2018

danzh2010 Aug 17, 2018

danzh2010 commented Aug 20, 2018

lizan left a comment

lizan Aug 20, 2018

danzh2010 Aug 20, 2018

alyssawilk Aug 21, 2018

danzh2010 Aug 21, 2018

lizan Aug 21, 2018

Add an unused implementation NoOpTransportSocketCallbacks #4181

Add an unused implementation NoOpTransportSocketCallbacks #4181

Conversation

danzh2010 commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan Aug 16, 2018 • edited Loading

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

danzh2010 commented Aug 20, 2018

lizan left a 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

danzh2010 commented Aug 16, 2018 •

edited

Loading

lizan Aug 16, 2018 •

edited

Loading