ClientConnectionImpl: Delay setTransportSocketCallbacks() until end of the constructor. by jrajahalme · Pull Request #5307 · envoyproxy/envoy

jrajahalme · 2018-12-14T22:26:27Z

ConnectionImpl already calls 'setTransportSocketCallbacks()' at the
end of construction, so that when called the transport socket sees the
Connection in a fully constructed state.

In the case of the ClientConnectionImpl the semantics are less clear,
as the setTransportSocketCallbacks() is called in the middle of the
construction process. This commit changes this so that for derived
classes the setTransportSocketCallbacks() is called at the end of the
derived class construction.

The motivation for this change comes from a TransportSocket extension
that relies on all socket options on client connections being set at
the time when setTransportSocketCallbacks() is called.

Risk Level: Low
Signed-off-by: Jarno Rajahalme jarno@covalent.io

…f the constructor. ConnectionImpl already calls 'setTransportSocketCallbacks()' at the end of construction, so that when called the transport socket sees the Connection in a fully constructed state. In the case of the ClientConnectionImpl the semantics are less clear, as the setTransportSocketCallbacks() is called in the middle of the construction process. This commit changes this so that for derived classes the setTransportSocketCallbacks() is called at the end of the derived class construction. Currently this is accomplished by only calling the setTransportSocketCallbacks() in ConnectionImpl constructor if the 'connected' parameter is passed in as 'true', as this is always the case for server connections, while the ClientConnectionImpl constructor passes this parameter as 'false'. The motivation for this change comes from a TransportSocket extension that relies on all socket options on client connections being set at the time when setTransportSocketCallbacks() is called. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

One question and also need to fix format. Thank you!

/wait

mattklein123 · 2018-12-14T22:39:06Z

source/common/network/connection_impl.cc

This is pretty fragile IMO. WDYT about making the constructor private and then have a static factory function that does the right thing for the various places that need to construct a connection? Would that be possible?

That would be a bit more "boilerplate" but would be not fragile at all, I'll give it a go. And sorry about the format thing, getting rusty...

repokitteh-read-only · 2018-12-14T23:51:57Z

🙀 Error while processing event:

evaluation error
error: context deadline exceeded

🐱

Caused by: #5307 was unlabeled by repokitteh[bot].

see: more, trace.

jrajahalme · 2018-12-14T23:51:58Z

Took the liberty to force push as the implementation is now completely different.

…nstructor being static. Now that Network::ClientConnectionImpl constructor is protected, ConfigValidateConnection also needs a static create() function. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Use Network::ClientConnectionImpl::createClientConnection() instead of the protected constructor. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

repokitteh-read-only · 2018-12-16T01:14:22Z

🙀 Error while processing event:

evaluation error
error: context deadline exceeded

🐱

Caused by: #5307 was unlabeled by repokitteh[bot].

see: more, trace.

mattklein123

Thanks a ton, this is much cleaner now. Few comments.

/wait

mattklein123 · 2018-12-16T05:30:14Z

source/common/network/connection_impl.cc

+std::unique_ptr<ConnectionImpl>
+ConnectionImpl::createServerConnection(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket,
+                                       TransportSocketPtr&& transport_socket) {
+  // make_shared<> requires a public constructor so we can't use it here.


nit: You mean make_unique?

Yes, will fix.

mattklein123 · 2018-12-16T05:30:30Z

source/common/network/connection_impl.cc

+    const Address::InstanceConstSharedPtr& source_address,
+    Network::TransportSocketPtr&& transport_socket,
+    const Network::ConnectionSocket::OptionsSharedPtr& options) {
+  // make_shared<> requires a public constructor so we can't use it here.


make_unique?

mattklein123 · 2018-12-16T05:31:00Z

source/common/network/connection_impl.cc

+  // make_shared<> requires a public constructor so we can't use it here.
+  std::unique_ptr<Network::ConnectionImpl> conn(new Network::ConnectionImpl(
+      dispatcher, std::move(socket), std::move(transport_socket), true));
+  conn->transport_socket_->setTransportSocketCallbacks(*conn);


Can you add a comment here and below (or just refer one to the other) of why we need to do this for future readers?

mattklein123 · 2018-12-16T05:31:43Z

source/common/network/connection_impl.h

                       public TransportSocketCallbacks,
                       protected Logger::Loggable<Logger::Id::connection> {
-public:
+protected:


nit: unless it can't be done, please move so the class is defined from public -> protected -> private. Same below.

Should be doable.

mattklein123 · 2018-12-16T05:31:57Z

source/server/config_validation/connection.cc

+                                 const Address::InstanceConstSharedPtr& source_address,
+                                 Network::TransportSocketPtr&& transport_socket,
+                                 const Network::ConnectionSocket::OptionsSharedPtr& options) {
+  // make_shared<> requires a public constructor so we can't use it here.


make_unique?

mattklein123 · 2018-12-16T05:32:07Z

source/server/config_validation/connection.h

 */
 class ConfigValidateConnection : public Network::ClientConnectionImpl {
-public:
+private:


lizan · 2018-12-16T20:37:44Z

The motivation for this change comes from a TransportSocket extension
that relies on all socket options on client connections being set at
the time when setTransportSocketCallbacks() is called.

This doesn't sound right really, from interface level setTransportSocketCallbacks() isn't expected to do anything else than keeping/propagating the reference of transport socket callbacks. If onConnected doesn't fit your use case, I think we can add another hook point for that.

mattklein123 · 2018-12-17T00:50:04Z

This doesn't sound right really, from interface level setTransportSocketCallbacks() isn't expected to do anything else than keeping/propagating the reference of transport socket callbacks. If onConnected doesn't fit your use case, I think we can add another hook point for that.

This change seems fine to me, but fine to defer to @lizan and @jrajahalme to discuss an alternate solution.

jrajahalme · 2018-12-17T18:38:26Z

@lizan It seems reasonable to me to expect that, in general, the callbacks, when called after being set, will get the results as if the underlying objects have been fully constructed. However, it may be that what I need should be done by some other means when the IoHandle and TransportSocket abstractions are fully developed. Specifically, in my case, a specialized TransportSocket needs to know when the ClientConnectionImpl has been fully constructed (including any socket options being set) before the client connection can get to the connected state. I.e., my transport socket implementation determines when a connection has been made in an I/O model where each connection does not have a unique file descriptor, but a virtual "I/O handle".

I don't really care how the "construction notification" is done, but piggy-backing it to the setTransportSocketCallbacks() felt like a natural extension of the semantics of it. It was already true for ConnectionImpl and seemed like a reasonable thing to do also for ClientConnectionImpl. Would you want so see a connectionConstructed() callback being added instead?

jrajahalme · 2018-12-17T18:47:46Z

@lizan @mattklein123 I just realized that depending on how IP_TRANSPARENT PR (#5255) lands, I might not need this, so I'm OK parking this PR for a while.

mattklein123 · 2018-12-17T18:56:47Z

@jrajahalme OK I will just close this for now. We can reopen if needed. I'm going to try to review all of the transparent stuff some time today.

mattklein123 reviewed Dec 14, 2018

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 14, 2018

mattklein123 self-assigned this Dec 14, 2018

jrajahalme force-pushed the fully-construct-connection-before-callbacks branch from 614e40e to 737c794 Compare December 14, 2018 23:51

repokitteh-read-only bot removed the waiting label Dec 14, 2018

mattklein123 added the waiting label Dec 15, 2018

jrajahalme added 2 commits December 15, 2018 16:28

config_validation: Add static create() due to ClientConnectionImpl co…

08c0e2a

…nstructor being static. Now that Network::ClientConnectionImpl constructor is protected, ConfigValidateConnection also needs a static create() function. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

test: Use createClientConnection()

7b6e210

Use Network::ClientConnectionImpl::createClientConnection() instead of the protected constructor. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the fully-construct-connection-before-callbacks branch from 239f56f to 7b6e210 Compare December 16, 2018 01:13

repokitteh-read-only bot removed the waiting label Dec 16, 2018

mattklein123 requested changes Dec 16, 2018

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 16, 2018

mattklein123 closed this Dec 17, 2018

Conversation

jrajahalme commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

repokitteh-read-only bot commented Dec 14, 2018

Uh oh!

jrajahalme commented Dec 14, 2018

Uh oh!

repokitteh-read-only bot commented Dec 16, 2018

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!

lizan commented Dec 16, 2018

Uh oh!

mattklein123 commented Dec 17, 2018

Uh oh!

jrajahalme commented Dec 17, 2018

Uh oh!

jrajahalme commented Dec 17, 2018

Uh oh!

mattklein123 commented Dec 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jrajahalme commented Dec 14, 2018 •

edited

Loading