listener: fix sharedSocket by lambdai · Pull Request #17090 · envoyproxy/envoy

lambdai · 2021-06-22T18:39:04Z

Commit Message:
The background is that a tcp listener which doesn't bind to port does not own io_handle. There is a code path that using ListenSocketFactory::sharedSocket() to obtain the socket and call io handle without checking if io handle is needed.

SharedSocket now returns nullopt when the tcp listener does not bind_to_port even if reuse_port set to true.

Additional Description:
Risk Level: LOW
Testing: added test that crashes on existing code. Also fixed a ListenerManagerImplTest
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

antoniovicente · 2021-06-23T00:19:34Z

source/server/listener_impl.h

  Network::SocketOptRef sharedSocket() const override {
+    // If a tcp listener socket doesn't bind to port, there is no listen socket so there is no
+    // shared socket.
+    if (socketType() == Network::Socket::Type::Stream && !bind_to_port_) {


Does this apply only to Stream sockets or should it also apply to other socket types?

That's a good question!
I think the sharedSocket() matters only if my imagined bind_to_port is on.
It seems udp listener's bind_to_port has a different semantic
@danzh2010 Could you add some comment on the bind_to_port usage in udp/quic listener?

Regardless the semantic of bind_to_port in UDP world, the implementation of UDP on bind_to_port = false is that a socket is created so sharedSocket() must returns it.

So the answer to the original question:
As per the current impl of udp listener, sharedSocket() should return it as in this PR.

However, if udp listener turns to not create socket on bind_to_port==false, this sharedSocket() need to align with that change.

It would be good to have the behaviors in sync. bind_to_port == false doesn't make much sense to me for UDP, I wonder if it should be disallowed.

That being said, I'm wondering if returning nullopt in sharedSocket() based on bind_to_port is the right approach? According to the contract sharedSocket() returns nullopt when the sockets among worker threads are not shared, so it is called in ListenerManagerImpl::drainListener() to decide whether to close shared socket or not. This PR seems to change that contract. Should we also change the corresponding logic of closing socket?

I agree this PR is somewhat changing the contract of sharedSocket(). Essentially we have two approaches, one is to provide a dedicate socket impl for listener doesn't bind to port. That specialized socket can be returned by sharedSocket(). The other approach in this PR is not to allow sharedSocket() from the beginning. Close is not an issue as per implementation: no underlying BSD socket is created so we are not leaking fd.

As I mentioned elsewhere, I'm currently working on simplifying all of this code, so I would prefer to not make major changes here which I'm going to undo anyway. Per my above comment, why can't we just check if io_handle_ is null in various places?

I would prefer to not make major changes here which I'm going to undo anyway.

I feel this PR has two part: sharedSocket() and the the ListenerSocketImpl.

Your changes will delete sharedSocket(). However, the latter ListenerSocketImpl is needed. The reason is that your commits seems create number of concurrency listen socket regardless it's bind_to_port, for simplicity. I don't know how is that listenersocket closed correctly... I could be wrong though.

Your changes will delete sharedSocket(). However, the latter ListenerSocketImpl is needed. The reason is that your commits seems create number of concurrency listen socket regardless it's bind_to_port, for simplicity. I don't know how is that listenersocket closed correctly... I could be wrong though.

Sorry I can't parse this. We can review my other change when I post it.

Oh, i had the wrong impression of udp listen socket always need a not-nullptr io_handle even when bind_to_port is false. I discovered this "fact" and I now incline to believe the fact is b/c a test case is improperly using listener socket.

lambdai · 2021-06-24T00:15:10Z

/retest

repokitteh-read-only · 2021-06-24T00:15:13Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17090 (comment) was created by @lambdai.

see: more, trace.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-06-25T19:31:27Z

Do you have any idea why the CI is not completed in time? @ggreenway
Also, what are the changes needed other than CI?

mattklein123 · 2021-06-28T04:18:02Z

I haven't reviewed this PR yet, but if it helps you in any way, I'm in the process of getting rid of the sharedSocket() function entirely. You can see my WIP branch here (this is about changing the default of reuse_port as well as making almost all listener errors happen on the main thread vs. workers which vastly simplifies error handling): 22361bc

lambdai · 2021-06-30T21:50:58Z

I haven't reviewed this PR yet, but if it helps you in any way, I'm in the process of getting rid of the sharedSocket() function entirely. You can see my WIP branch here (this is about changing the default of reuse_port as well as making almost all listener errors happen on the main thread vs. workers which vastly simplifies error handling): 22361bc

+1 for killing sharedSocket() or mutate reuse_port. I recall the changes (was a PR IIRC).
The blocker to me is not the sharedSocket() nor the default value. It's the crash at the rare combination listed in the added test case in this PR.
Can we merge this to fix the crash?

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

/wait

source/common/network/listen_socket_impl.h

mattklein123 · 2021-07-02T15:23:39Z

source/server/listener_impl.h

  Network::SocketOptRef sharedSocket() const override {
+    // If a tcp listener socket doesn't bind to port, there is no listen socket so there is no
+    // shared socket.
+    if (socketType() == Network::Socket::Type::Stream && !bind_to_port_) {


As I mentioned elsewhere, I'm currently working on simplifying all of this code, so I would prefer to not make major changes here which I'm going to undo anyway. Per my above comment, why can't we just check if io_handle_ is null in various places?

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-07-02T22:15:39Z

test/common/quic/active_quic_listener_test.cc


  void sendCHLO(quic::QuicConnectionId connection_id) {
-    client_sockets_.push_back(std::make_unique<Socket>(local_address_, nullptr, /*bind*/ false));
+    client_sockets_.push_back(std::make_unique<Network::SocketImpl>(Network::Socket::Type::Datagram,


@mattklein123 With this change, the io_handle == nullptr iff bind_to_port for listensocket, both tcp and udp.

Now UDP and TCP listener behavior is now unified including what you pointed out

mattklein123

Thanks this makes sense to me with small comment.

/wait

mattklein123 · 2021-07-06T15:20:35Z

source/common/network/listen_socket_impl.h

+  // These four overrides are introduced to perform check. A null io handle is possible only if the
+  // io handle is listener side.


What does "is listener side" mean? Clarify?

Updated to

A null io handle is possible only if the the owner socket is a listen socket that does not bind to port.

Is it better?

mattklein123 · 2021-07-06T16:35:08Z

source/common/network/listen_socket_impl.h

+    return *io_handle_;
+  }
+  void close() override {
+    if (io_handle_ != nullptr) {


In working through my other change, I'm hitting a similar issue on setSocketOption (now that reuse port is the default). I think there is a similar bug there also. It makes me think we are going about the no bind sockets incorrectly in general if we need all these guards.

(i.e. why do no bind listeners even have sockets. I realize this is a much larger change and I'm trying to get through my current mess first, but this seems like something we might want to fix in the future.)

SetSocketOption should check bind_to_port w/ or w/o PR. At least ListenSocketImpl::setupSocket has a bind_to_port arg.

why do no bind listeners even have sockets

Good point! I was thinking about create a derived not-bind-listen-socket and you think deeper

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Thanks this makes sense. I will merge this change into my other branch which will take a lot longer to review.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai added 4 commits June 21, 2021 15:45

fix prod

759a38b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix test

f142345

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

format

f55e317

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add extra io handle check

a12492f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

alyssawilk assigned yanavlasov and ggreenway Jun 22, 2021

antoniovicente reviewed Jun 23, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into tcpbindtoport

06d434b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 self-assigned this Jun 28, 2021

mattklein123 added the waiting label Jun 30, 2021

mattklein123 removed the waiting label Jul 1, 2021

Merge branch 'main' into tcpbindtoport

14b244a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 requested changes Jul 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 2, 2021

unify udp and tcp listener socket constructor

34e6c6f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Jul 2, 2021

clean up

df2d945

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Jul 2, 2021

View reviewed changes

mattklein123 requested changes Jul 6, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 6, 2021

mattklein123 reviewed Jul 6, 2021

View reviewed changes

update comment

60cdee7

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Jul 7, 2021

mattklein123 approved these changes Jul 7, 2021

View reviewed changes

mattklein123 merged commit b2cd50b into envoyproxy:main Jul 7, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

listener: fix sharedSocket (envoyproxy#17090)

ea95481

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

		// These four overrides are introduced to perform check. A null io handle is possible only if the
		// io handle is listener side.

Conversation

lambdai commented Jun 22, 2021

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!

lambdai Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai commented Jun 24, 2021

Uh oh!

repokitteh-read-only bot commented Jun 24, 2021

Uh oh!

lambdai commented Jun 25, 2021

Uh oh!

mattklein123 commented Jun 28, 2021

Uh oh!

lambdai commented Jun 30, 2021 • 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!

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lambdai Jul 2, 2021 •

edited

Loading

lambdai commented Jun 30, 2021 •

edited

Loading