network: refactor tcp listener to use file events by florincoras · Pull Request #12547 · envoyproxy/envoy

florincoras · 2020-08-07T18:19:28Z

IoHandle specific, implicitly, socket interface specific,
implementations of listen and accept are now used to accept new
IoHandles, instead of the os specific sycalls libevent leverages
internally for listeners.

Signed-off-by: Florin Coras fcoras@cisco.com

Risk Level: Medium
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a

IoHandle specific, implicitly, socket interface specific, implementations of listen and accept are now used to accept new IoHandles, instead of the os specific sycalls libevent leverages internally for listeners. Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras · 2020-08-07T18:22:09Z

@mattklein123 this is practically point 4 here.

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente

Looks like a nice cleanup. Thanks for doing this!

antoniovicente · 2020-08-10T18:31:01Z

source/common/api/posix/os_sys_calls_impl.cc

+  os_fd_t rc;
+
+#if defined(__linux__)
+  rc = ::accept4(sockfd, addr, addrlen, flags);


What other flags can be passed in to this version of accept?

SOCK_CLOEXEC, but as far as I understood, we're not currently using that.

antoniovicente · 2020-08-10T18:34:46Z

source/common/network/listener_impl.cc

+void ListenerImpl::onSocketEvent(short flags) {
+  ASSERT(flags & (Event::FileReadyType::Read));
+
+  while (1) {


It would be good to have a limit on the number of accept calls done per wakeup and warn if we had to do too many accepts in a row as it indicates that the proxy may have fallen behind for some reason.

As far as I know, libevent is not doing anything more complicated than this. But we could definitely improve it going forward.

Please add a TODO here

antoniovicente · 2020-08-10T18:40:27Z

source/common/network/listener_impl.cc

+#ifdef WIN32
+  auto trigger = Event::FileTriggerType::Level;
+#else
+  auto trigger = Event::FileTriggerType::Edge;


I think listeners should always operate on Level mode.

Operating in level mode allows us to safely accept only the first N connections available and return without having to set explicit resumption conditions.

This is related to the comment above. We currently consume all the accepted sessions, before returning. If we plan to add a more sophisticated admissions mechanism, then we might want to switch.

Use of level mode here always feels slightly safer. I agree that use of edge trigger should mostly be ok since we drain to completion. I worry about some transient error on accept resulting in us losing the trigger and then never recovering. Plus, doing the same on all platforms is simpler.

antoniovicente · 2020-08-10T18:42:11Z

source/common/network/listener_impl.cc

+ListenerImpl::~ListenerImpl() {
+  if (file_event_.get()) {
+    file_event_->setEnabled(0);
+    file_event_.reset();


I'm fairly sure that you can just delete the file_event_, no need to setEnabled(0) first. You should be able to remove this explicit destructor.

Fair point. Let me run the whole test suite to see if it hits any problems.

Out of the 687 tests, //test/integration:h1_capture_fuzz_test crashed at source/common/http/conn_manager_impl.cc:639 on first suite run but always passed afterwards (if run as part of the suite or independently). Not sure what to make of it as it could be unrelated ...

antoniovicente · 2020-08-10T18:44:16Z

source/common/network/listener_impl.cc

+    socklen_t remote_addr_len = sizeof(remote_addr);
+
+    IoHandlePtr io_handle = socket_->ioHandle().accept(reinterpret_cast<sockaddr*>(&remote_addr),
+                                                       &remote_addr_len, ENVOY_SOCK_NONBLOCK);


ENVOY_SOCK_NONBLOCK seems to be hard coded.

Do we need the flags argument? Should the implementations set non-block unconditionally on their own?

ENVOY_SOCK_NONBLOCK is just an os independent flag that we can pass to accept since SOCK_NONBLOCK does not exist on windows and darwin. The traditional POSIX accept syscall, on linux at least, does not inherit the O_NONBLOCK flag (see SOCK_NONBLOCK here).

The question is if you should hard code use of SOCK_NONBLOCK in OsSysCallsImpl::accept and drop the flags argument to the accept interface method.

Apologies, I thought you meant the underlying syscalls, not our accept implementations. I guess we can do that and then if at a later point the need for flags arises, we can re-add the parameter. Reasonable?

antoniovicente · 2020-08-10T18:45:19Z

test/mocks/network/io_handle.h

  MOCK_METHOD(bool, supportsUdpGro, (), (const));
  MOCK_METHOD(Api::SysCallIntResult, bind, (Address::InstanceConstSharedPtr address));
  MOCK_METHOD(Api::SysCallIntResult, listen, (int backlog));
+  MOCK_METHOD(IoHandlePtr, accept, (struct sockaddr * addr, socklen_t* addrlen, int flags));


Is there some additional testing that we should add for your changes? Are there specific reasons why additional testing is not needed?

Up until now, this accept was hidden within libevent. So, whatever we tested until now should still be tested.

antoniovicente · 2020-08-10T18:47:23Z

source/common/network/listener_impl.cc

+#else
+  auto trigger = Event::FileTriggerType::Edge;
+#endif
+  socket.ioHandle().listen(128);


Use a constant for the 128 backlog.

What was the listen backlog prior to these changes? Should the backlog be configurable?

See #9501. @florincoras can you fix that as part of a follow up change? Would really appreciate it. It should be much easier now. In the interim we should replicate what libevent does which IIRC is 128 as you did here. Please add a TODO to follow up and fix this.

Actually, that's what I found by digging into libevent.

@mattklein123 just realized I missed your comment. Adding the TODO and yes, this should be pretty easy to customize now, although I'll need a bit of guidance wrt to where we should be configuring this from. Later edit: I can go with the suggestion from the issue, i.e., tcp_backlog_size and listener.proto if that's okay with you.

A TODO should be good enough for now.

tcp_backlog_size would be a nice future addition in a followup change by either you or someone else.

Yup, added it to my TODO list.

Can you flesh out the comment more to explain where the magic 128 comes from (libevent)?

mattklein123

Thanks for working on this. Just flushing a few quick comments.

/wait

mattklein123 · 2020-08-10T20:11:38Z

source/common/network/listener_impl.cc

+void ListenerImpl::onSocketEvent(short flags) {
+  ASSERT(flags & (Event::FileReadyType::Read));
+
+  while (1) {


Please add a TODO here

mattklein123 · 2020-08-10T20:13:01Z

source/common/network/listener_impl.cc

+#else
+  auto trigger = Event::FileTriggerType::Edge;
+#endif
+  socket.ioHandle().listen(128);


See #9501. @florincoras can you fix that as part of a follow up change? Would really appreciate it. It should be much easier now. In the interim we should replicate what libevent does which IIRC is 128 as you did here. Please add a TODO to follow up and fix this.

mattklein123 · 2020-08-10T20:13:44Z

source/common/network/listener_impl.h

  // rejected/closed. If the accepted socket is to be admitted, false is returned.
  static bool rejectCxOverGlobalLimit();

-  Event::Libevent::ListenerPtr listener_;


This type I think can be deleted now wherever it is defined.

Ack! Will remove!

- added TODOs for accept admission and listen backlog config - removed explicit Listener destructor - removed ListenerPtr type - removed accept flags, i.e., SOCK_NONBLOCK is now implicit on accepted fds Signed-off-by: Florin Coras <fcoras@cisco.com>

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente · 2020-08-11T03:16:55Z

source/common/network/listener_impl.cc

+#else
+  auto trigger = Event::FileTriggerType::Edge;
+#endif
+  socket.ioHandle().listen(128);


A TODO should be good enough for now.

tcp_backlog_size would be a nice future addition in a followup change by either you or someone else.

antoniovicente · 2020-08-11T03:25:29Z

source/common/network/listener_impl.cc

+#ifdef WIN32
+  auto trigger = Event::FileTriggerType::Level;
+#else
+  auto trigger = Event::FileTriggerType::Edge;


Use of level mode here always feels slightly safer. I agree that use of edge trigger should mostly be ok since we drain to completion. I worry about some transient error on accept resulting in us losing the trigger and then never recovering. Plus, doing the same on all platforms is simpler.

antoniovicente · 2020-08-11T03:31:43Z

include/envoy/api/os_sys_calls.h

  virtual SysCallSizeResult write(os_fd_t socket, const void* buffer, size_t length) PURE;
+
+  /**
+   * @see man 2 accept


We may want to say something about it also marking the socket as non-blocking.

Good point. Added!

- listener FileEvent uses level trigger mode - comment about non-blocking accepted fds Signed-off-by: Florin Coras <fcoras@cisco.com>

mattklein123

Thanks LGTM with small comments.

/wait

mattklein123 · 2020-08-11T18:19:49Z

source/common/network/listener_impl.cc

+#else
+  auto trigger = Event::FileTriggerType::Edge;
+#endif
+  socket.ioHandle().listen(128);


Can you flesh out the comment more to explain where the magic 128 comes from (libevent)?

mattklein123 · 2020-08-11T18:20:12Z

source/common/network/listener_impl.cc

+  socket.ioHandle().listen(128);
+  file_event_ = dispatcher.createFileEvent(
+      socket.ioHandle().fd(), [this](uint32_t events) -> void { onSocketEvent(events); },
+      Event::FileTriggerType::Level, Event::FileReadyType::Read);


Can you add a comment specific to why we are using level trigger here until if/when we change it?

mattklein123 · 2020-08-11T18:20:44Z

source/common/network/listener_impl.cc

-    throw CreateListenerException(
-        fmt::format("cannot listen on socket: {}", socket.localAddress()->asString()));
-  }
+  RELEASE_ASSERT(file_event_, "file event must be valid");


nit: remove, we don't check for OOM type errors like this.

- more context for 128 listener backlog - explain why we use level trigger, as opposed to edge trigger, for listener file event - removed OOM release assert Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras · 2020-08-11T21:54:45Z

Is there any way to retry only Linux-x64 release as it failed with:
ERROR: /source/test/mocks/server/BUILD:120:14: C++ compilation of rule '//test/mocks/server:server_lifecycle_notifier_mocks' failed (Exit 34): java.io.IOException: com.google.devtools.build.lib.remote.ExecutionStatusException: ABORTED: the bot running the task appears to be lost

mattklein123

Thanks!

florincoras requested review from alyssawilk and mattklein123 as code owners August 7, 2020 18:19

dio assigned antoniovicente Aug 7, 2020

mattklein123 assigned yanavlasov Aug 7, 2020

fix win and macos builds

266dc4e

Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras force-pushed the listener branch from 08ee28c to 266dc4e Compare August 8, 2020 01:30

fix windows invalid socket test

6f27463

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente reviewed Aug 10, 2020

View reviewed changes

mattklein123 self-assigned this Aug 10, 2020

mattklein123 requested changes Aug 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 10, 2020

address comments

4053e39

- added TODOs for accept admission and listen backlog config - removed explicit Listener destructor - removed ListenerPtr type - removed accept flags, i.e., SOCK_NONBLOCK is now implicit on accepted fds Signed-off-by: Florin Coras <fcoras@cisco.com>

repokitteh-read-only bot removed the waiting label Aug 10, 2020

fix windows typo

739dba8

Signed-off-by: Florin Coras <fcoras@cisco.com>

antoniovicente reviewed Aug 11, 2020

View reviewed changes

more comments

95acc45

- listener FileEvent uses level trigger mode - comment about non-blocking accepted fds Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras mentioned this pull request Aug 11, 2020

Customizing backlog size #9501

Closed

mattklein123 requested changes Aug 11, 2020

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Aug 11, 2020

additional fixes

dea7ef5

- more context for 128 listener backlog - explain why we use level trigger, as opposed to edge trigger, for listener file event - removed OOM release assert Signed-off-by: Florin Coras <fcoras@cisco.com>

mattklein123 approved these changes Aug 12, 2020

View reviewed changes

yanavlasov approved these changes Aug 12, 2020

View reviewed changes

mattklein123 merged commit bcccbfa into envoyproxy:master Aug 12, 2020

florincoras deleted the listener branch August 28, 2020 20:02

Conversation

florincoras commented Aug 7, 2020

Uh oh!

florincoras commented Aug 7, 2020

Uh oh!

antoniovicente 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!

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!

florincoras Aug 10, 2020 • 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!

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!

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

florincoras Aug 10, 2020 •

edited

Loading

florincoras commented Aug 11, 2020 •

edited

Loading