IoSocketHandleImpl::close() by danzh2010 · Pull Request #5171 · envoyproxy/envoy

danzh2010 · 2018-11-30T21:28:00Z

Implement IoSocketHandleImpl::close(). And replace the use of ::close(int fd) with it everywhere.
Risk Level: low.
Test: Rely on existing tests. If there are ::close(fd) left out, ~IoHandle() should have caused double close on same fd in tests.
Part of #4546 #2557

Signed-off-by: Dan Zhang <danzh@google.com>

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Dan Zhang <danzh@google.com>

venilnoronha · 2018-12-01T04:59:24Z

/retest

repokitteh-read-only · 2018-12-01T04:59:26Z

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #5171 (comment) was created by @venilnoronha.

see: more, trace.

venilnoronha · 2018-12-01T05:58:10Z

include/envoy/api/os_sys_calls.h

+   * @see man 2 connect
+   */
+  virtual SysCallIntResult connect(int sockfd, const struct sockaddr* serv_addr,
+                                   socklen_t addrlen) PURE;


Do you plan to send in a follow-up PR that updates existing calls to ::getsockname, ::getpeername, ::connect, etc.?

Either me or @sbelair2 will do, but yes, there will be follow-up pr's to use IoHandle interface instead of operating on fd directly.

venilnoronha · 2018-12-01T06:03:42Z

source/common/network/io_handle_impl.cc

+}
+
+SysCallIntResult FdIoHandleImpl::getSocketName(sockaddr* addr, socklen_t* addr_len) {
+


nit: remove extra line break.

done in io_socket_handle_impl.cc

venilnoronha · 2018-12-01T06:04:07Z

source/common/network/io_handle_impl.h

+namespace Envoy {
+namespace Network {
+
+class FdIoHandleImpl : public virtual IoHandle {


nit: I think we can skip the virtual keyword.

done in io_socket_handle_impl.cc

mattklein123 · 2018-12-01T18:43:16Z

@danzh2010 how does this relate to #5128? Is this a separate implementation? Can you please sync up with @sbelair2 to reconcile? I would like to only review a single sequence of changes on this topic. Thank you!

danzh2010 · 2018-12-03T20:25:51Z

I originally mean to only send this PR to sbelair2 for discussion about the interface and the fd specific implementation. I think he can either merge mine into his current PR or submitting his as is and I can extend it with this one. We will definitely sync up on slack. Sorry for the confusion.

jmarantz · 2018-12-05T17:37:29Z

/sub

danzh2010 · 2018-12-12T17:37:57Z

I'm going on vacation for 4 weeks. How can I prevent this PR from becoming stale and closed?

dnoe · 2018-12-12T17:41:58Z

You can just re-open it when you return, come back to this page and comment and click "Reopen and comment".

sbelair2 · 2018-12-12T17:46:33Z

My apologies for taking so long. As I said in our private communication, I have converted IoHandlePtr to a unique_ptr but cannot get things to work: 59 tests all fail quickly with an illegal instruction: 4. I’ve been going through my code and don’t see it, and it is very difficult to debug- am in the process of setting up a vm and trying to catch it in gdb. When are you going on vacation? And can’t the PR be re-opened? Btw, the PR will have moved on past this point by then, no matter what. From: danzh <notifications@github.com> Reply-To: envoyproxy/envoy <reply@reply.github.com> Date: Wednesday, December 12, 2018 at 9:38 AM To: envoyproxy/envoy <envoy@noreply.github.com> Cc: "Stephen Belair (sbelair)" <sbelair@cisco.com>, Mention <mention@noreply.github.com> Subject: Re: [envoyproxy/envoy] Add an IoHandle interface (#5171) I'm going on vacation for 4 weeks. How can I prevent this PR from becoming stale and closed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#5171 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADxdgc3cFava-xPw93TdYwwjy3CD7IKxks5u4T8BgaJpZM4Y8azp>.

jmarantz · 2018-12-12T17:53:16Z

I think only maintainers get a "re-open" button. But one of us can do that for you.

One concern I have with this family of PRs is that I feel a holistic design incorporating abstraction of libevent is needed. It is hard for me to evaluate any of these PRs if ultimately they still have to interface with libevent, which exposes FDs at the interface.

mattklein123 · 2018-12-12T18:03:17Z

@jmarantz I think we have a good handle on how to proceed re: libevent, at least in the short term. Let's chat offline. @sbelair2 I'm sorry you are having a hard time with unique_ptr. If you can't figure it out we can have someone take over the PR. Thanks!

jrajahalme · 2018-12-12T19:18:47Z

source/common/network/io_handle_impl.cc

+IoHandlePtr FdIoHandleImpl::socket(int domain, int type, int protocol) {
+  SysCallIntResult result = Api::OsSysCallsSingleton::get().socket(domain, type, protocol);
+  RELEASE_ASSERT(result.rc_ != -1, "");
+  return IoHandlePtr(new FdIoHandleImpl(result.rc_));


make_unique<>() would be easier on the eye.

done in io_socket_handle_impl.cc

jrajahalme · 2018-12-12T20:12:07Z

source/common/network/io_handle_impl.h

+
+  Api::SysCallIntResult close() override;
+
+  bool isClosed() override;


count be bool isClosed() const override;

Technically, most methods (all that do not change the fd_) could be marked const, but maybe should do so only on methods that do not modify the underlying socket.

Obviously this change should be made on the base class first.

done in io_socket_handle_impl.* and io_handle.h

stale · 2018-12-19T21:31:44Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-12-27T00:22:01Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

danzh2010 · 2019-02-13T23:12:26Z

/assign @sbelair2

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Thanks I think this approach is great. A few small comments.

/wait

mattklein123 · 2019-02-17T02:19:41Z

include/envoy/network/io_handle.h

+// frequent memory allocation of IoError instance.
+// If this is used, IoHandleCallResult has to be instantiated with a deleter that does not
+// deallocate memory for this error.
+#define ENVOY_ERROR_AGAIN reinterpret_cast<IoError*>(1)


nit: Is it possible to use a constexpr here vs. a define?

not done. constexpr doesn't like reinterpret_cast.

mattklein123 · 2019-02-17T02:20:11Z

include/envoy/network/io_handle.h

+  // Map platform specific error into IoErrorCode.
+  // Needed to hide errorCode() in case of ENVOY_ERROR_AGAIN.
+  static IoErrorCode getErrorCode(const IoError* err) {
+    ASSERT(err != nullptr);


nit: Can we just make this function take a reference to avoid this assert? Same elsewhere in similar cases?

I think *ENVOY_ERROR_AGAIN will cause crash.

It won't crash actually, from the computer perspective a pointer and reference are the same thing, it won't crash until you actually try to access the memory (I'm 99% sure). I would go ahead and switch to a reference assuming it works.

Yeah, you're right. I switched to use reference here.
If the caller try to dereference a nullptr, we will have seg fault. Is this more preferred than ASSERT?

Yeah in general we prefer references for things that can't be null. In this case, I don't consider ENVOY_ERROR_AGAIN to be null.

mattklein123 · 2019-02-17T02:21:11Z

source/common/network/io_socket_handle_impl.cc

+IoError::IoErrorCode IoSocketError::errorCode() const {
+  switch (errno_) {
+  case EAGAIN:
+    RELEASE_ASSERT(false, "EAGAIN should use specific error ENVOY_ERROR_AGAIN");


Can we just swap with NOT_REACHED?

mattklein123 · 2019-02-17T02:23:15Z

source/common/network/io_socket_handle_impl.cc

+  const int rc = ::close(fd_);
+  fd_ = -1;
+  IoErrorPtr err(nullptr, deleteIoError);
+  if (rc == -1) {


Can this ever actually fail? It definitely can't return EAGAIN I don't think. Can we just RELEASE_ASSERT that is doesn't fail instead of having this code?

Yes, it couldn't be EAGAIN for sure. But it can fail according t linux man.

I think it can only fail if passed a bad handle though, which should never happen. I would just RELEASE_ASSERT it personally.

RELEASE_ASSERT will fail test/common/network/connection_impl_test.cc because most of the io_handle objects in the test are initialized manually with invalid fd like 0 or 1.
I removed the if (rc == -1 ) block and return rc as is.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-02-21T15:21:20Z

/retest

repokitteh-read-only · 2019-02-21T15:21:25Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5171 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-02-21T17:10:51Z

mac build failure: ERROR: /Users/distiller/project/test/common/upstream/BUILD:167:1: Linking of rule '//test/common/upstream:load_stats_reporter_test' failed (Exit 1) cc_wrapper.sh failed: error executing command ...
ld: not enough disk space for writing 'bazel-out/darwin-fastbuild/bin/test/common/upstream/load_stats_reporter_test' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation).

This seems unrelated to this PR

mattklein123 · 2019-02-21T17:12:42Z

@danzh2010 don't worry about it, but if you need more changes merge master and I think it will go away.

mattklein123

LGTM, let's see if we can shift pointers to references, if not we can ship! Thank you!

mattklein123 · 2019-02-21T17:17:05Z

include/envoy/network/io_handle.h

+  // Map platform specific error into IoErrorCode.
+  // Needed to hide errorCode() in case of ENVOY_ERROR_AGAIN.
+  static IoErrorCode getErrorCode(const IoError* err) {
+    ASSERT(err != nullptr);


It won't crash actually, from the computer perspective a pointer and reference are the same thing, it won't crash until you actually try to access the memory (I'm 99% sure). I would go ahead and switch to a reference assuming it works.

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Nice!

danzh2010 · 2019-02-25T16:02:26Z

I'm wondering if it might make sense to skip routing everything through os_sys_calls here, and just have the IoHandle implementations make the necessary syscalls directly. While it's fine here where the FdIoHandleImpl is wrapping an integer file descriptor, I think it may cause issues with a Windows implementation where the IoHandle is wrapping a SOCKET. Either os_sys_calls would have to be updated such that close / bind / connect etc. take the appropriate socket descriptor on each platform (int vs SOCKET), or the Windows IoHandle would just not use os_sys_calls, which could lead to drift down the line

This is hard to achieve in #6037, given that os_sys_calls is used by the caller, and the readv/writev interface already diverged from syscall's interface.

sesmith177 · 2019-02-25T17:19:47Z

I'm wondering if it might make sense to skip routing everything through os_sys_calls here, and just have the IoHandle implementations make the necessary syscalls directly. While it's fine here where the FdIoHandleImpl is wrapping an integer file descriptor, I think it may cause issues with a Windows implementation where the IoHandle is wrapping a SOCKET. Either os_sys_calls would have to be updated such that close / bind / connect etc. take the appropriate socket descriptor on each platform (int vs SOCKET), or the Windows IoHandle would just not use os_sys_calls, which could lead to drift down the line

This is hard to achieve in #6037, given that os_sys_calls is used by the caller, and the readv/writev interface already diverged from syscall's interface.

thanks, I'll take a look

cc @yaelharel

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

danzh1989 added 3 commits November 30, 2018 15:00

Add new interface IoHandle to replace direct operations on fd.

a6de84a

Signed-off-by: Dan Zhang <danzh@google.com>

remove debug output

16db4f1

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Dan Zhang <danzh@google.com>

fix format

b696bc2

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the fdwrapperinterface branch from 5503f68 to f26fc1e Compare November 30, 2018 21:29

fix format

4b1c922

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the fdwrapperinterface branch 2 times, most recently from c9a3af8 to 4b1c922 Compare November 30, 2018 21:47

danzh2010 mentioned this pull request Nov 30, 2018

Issue 4546: Refactor FD with a general I/O Handle #5128

Closed

venilnoronha reviewed Dec 1, 2018

View reviewed changes

mattklein123 self-assigned this Dec 1, 2018

danzh2010 mentioned this pull request Dec 5, 2018

Refactor fd() callback in transport sockets to a generalized I/O handle. #4546

Closed

mattklein123 added the waiting label Dec 5, 2018

jrajahalme reviewed Dec 12, 2018

View reviewed changes

jrajahalme mentioned this pull request Dec 14, 2018

Minimal support for multiplexed transport connections #5301

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 19, 2018

stale bot closed this Dec 27, 2018

danzh2010 changed the title ~~Add an IoHandle interface~~ Extend IoHandle interface Jan 28, 2019

repokitteh-read-only bot assigned sbelair2 Feb 13, 2019

mattklein123 added the waiting label Feb 14, 2019

ENVOY_ERROR_AGAIN

6c638e7

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Feb 15, 2019

danzh1989 added 4 commits February 15, 2019 15:40

fix test

8dd36d6

Signed-off-by: Dan Zhang <danzh@google.com>

fix mis spell

18a5a33

Signed-off-by: Dan Zhang <danzh@google.com>

add deleter to dictionary

daa010b

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into fdwrapperinterface

6e85489

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 requested changes Feb 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 17, 2019

use NOT_REACHED

a443ab7

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Feb 19, 2019

mattklein123 added the waiting label Feb 20, 2019

danzh1989 added 3 commits February 20, 2019 16:53

Merge branch 'master' into fdwrapperinterface

28beeda

Signed-off-by: Dan Zhang <danzh@google.com>

modify close

b1a640a

Signed-off-by: Dan Zhang <danzh@google.com>

format

dcd7068

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Feb 20, 2019

mattklein123 reviewed Feb 21, 2019

View reviewed changes

danzh1989 added 2 commits February 21, 2019 13:17

passing reference

c19433f

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into fdwrapperinterface

6cc83ad

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 approved these changes Feb 21, 2019

View reviewed changes

mattklein123 merged commit b2149d1 into envoyproxy:master Feb 21, 2019

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019

IoSocketHandleImpl::close() (envoyproxy#5171)

3f20b8c

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

		}

		SysCallIntResult FdIoHandleImpl::getSocketName(sockaddr* addr, socklen_t* addr_len) {


		Api::SysCallIntResult close() override;

		bool isClosed() override;

Conversation

danzh2010 commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

venilnoronha commented Dec 1, 2018

Uh oh!

repokitteh-read-only bot commented Dec 1, 2018

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!

mattklein123 commented Dec 1, 2018

Uh oh!

danzh2010 commented Dec 3, 2018

Uh oh!

jmarantz commented Dec 5, 2018

Uh oh!

danzh2010 commented Dec 12, 2018

Uh oh!

dnoe commented Dec 12, 2018

Uh oh!

sbelair2 commented Dec 12, 2018 via email

Uh oh!

jmarantz commented Dec 12, 2018

Uh oh!

mattklein123 commented Dec 12, 2018

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!

stale bot commented Dec 19, 2018

Uh oh!

stale bot commented Dec 27, 2018

Uh oh!

danzh2010 commented Feb 13, 2019

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

danzh2010 commented Nov 30, 2018 •

edited

Loading