network: various fixes for OS X socket behavior by zuercher · Pull Request #1381 · envoyproxy/envoy

zuercher · 2017-08-02T22:52:11Z

Some network socket changes for OS X support:

getpeername returns a larger size in its addrlen parameter for an anonymous peer than on Linux.
OS X doesn't have a mechanism to create a socket in non-blocking mode. It require a call to fcntl to set O_NONBLOCK. While making this change I made all the socket creation paths RELEASE_ASSERT on the fd being valid.
OS X returns an error on setsockopt if it's invoked on an already-closed socket.
Fixes a race between artificially generated write events and the write event that signals connection completed in ConnectionImpl.
OS X doesn't support getsockopt(,,SOL_ORIGINAL_DST,) -- use getsockname instead and work around the fact that the latter works for IPv6 where the Linux getsockopt call does not. Added direct testing of Network::Utility::getOriginalDst.

(Split out from #1348, in support of #128.)

zuercher · 2017-08-02T22:53:21Z

source/common/network/connection_impl.cc

+        return;
+      }
+    }
+#endif


@PiotrSikora you mentioned kq_errno but I'm not familiar with that and google only seems to be showing me references to nginx source code. It looks like there the event flags from libevent are preserved. Does that enable it to detect this condition?

Ooops, sorry! kq_errno is indeed just NGINX's field name for that.

What you want is to check flags (if it's EV_EOF) and fflags (for errno) fields from struct kevent, see kqueue(2).

zuercher · 2017-08-02T23:01:54Z

test/common/network/utility_test.cc

+INSTANTIATE_TEST_CASE_P(IpVersions, NetworkUtilityGetOriginalDst,
+                        testing::ValuesIn(TestEnvironment::getIpVersionsForTest()));
+
+TEST_P(NetworkUtilityGetOriginalDst, GetOriginalDst) {


This test is a bit disingenuous since the connection isn't really being forwarded from another port.

However, I don't think it's even possible to do that kind of port redirection in OS X, so perhaps I should change getOriginalDst to always return nullptr in OS X and instead consider disabling the use_original_dst feature instead.

I think it's reasonable to disable this feature altogether on OS X for now, and bring it back via separate PR at some point in the future, if it's possible at all (I think it should be possible to redirect traffic via either divert(2) or pf.conf on OS X, but I didn't verify it).

I agree. I would just revert all of this IMO and only do the SOL_IP call on linux, and on apple just set error to ENOTSUPPORTED (or whatever) so that the subsequence if check fails with a TODO.

mattklein123 · 2017-08-03T04:54:17Z

source/common/network/connection_impl.cc

    UNREFERENCED_PARAMETER(rc);

+#ifdef __APPLE__
+    // When a connect() is performed on a non-blocking socket, it'll return -1/EINPROGRESS


This whole thing is absolutely nutty. Is there some documentation or stack overflow or something we can link to?

This PR LGTM with this exception as well. Just some better citations would be sufficient if there is no way to improve on this.

mattklein123 · 2017-08-03T04:56:01Z

test/common/network/utility_test.cc

+  signal();
+  thread->join();
+
+#if !defined(__APPLE__)


I don't quite understand what this test is doing. Just making sure the calls actually work? And we don't support it on IPv6 basically? Per above I would just disable all of this on OSX and just leave the code as is before. If we keep this test I have some other comments but I can hold off for now until we decide.

htuch · 2017-08-03T16:58:33Z

source/common/network/address_impl.cc

+  RELEASE_ASSERT(fd != -1);
+
+#ifdef __APPLE__
+  // Cannot set SOCK_NONBLOCK as a ::socket flag


Can we make Linux do what is needed for OS X here, and reduce the special casing?

There are perf implications for this (multiple system calls vs. 1), so in this case I would be in favor of the ifdef. If there is a way we can make it less visually annoying that's fine.

I switched flagsFromSocketType to be socketFromSocketType and do all the flag setting, fcntl, and socket creation there. I think this is better, since it's all contained in one block.

PiotrSikora · 2017-08-03T19:11:39Z

source/common/network/address_impl.cc

+
+#ifdef __APPLE__
+  // Cannot set SOCK_NONBLOCK as a ::socket flag
+  RELEASE_ASSERT(fcntl(fd, F_SETFL, O_NONBLOCK) != -1);


This overwrites existing flags... It's probably safe, since we've just created the socket, but for my sanity, could you verify that fcntl(fd, F_GETFL) returns 0 at this point? Just one-off experiment, not in the PR.

Hmm... I'm confused. Apparently, it doesn't overwrite flags on OS X.

fcntl(fd, F_GETFL) on a new socket returns 0x0002 (O_RDWR), but it returns 0x0006 (O_RDWR|O_NONBLOCK) after fcntl(fd, F_SETFL, O_NONBLOCK), which is not something I'd expect.

From http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html:

F_SETFL
Set the file status flags, defined in <fcntl.h>, for the file description associated with fildes from the corresponding bits in the third argument, arg, taken as type int. Bits corresponding to the file access mode and the file creation flags, as defined in <fcntl.h>, that are set in arg shall be ignored. If any bits in arg other than those mentioned here are changed by the application, the result is unspecified. If fildes does not support non-blocking operations, it is unspecified whether the O_NONBLOCK flag will be ignored.

which explains why O_RDWR isn't cleared.

mattklein123

This LGTM beyond my ask for some additional comments on how you knew about the crazy SO_ERROR stuff on OS X. @htuch @PiotrSikora ?

PiotrSikora · 2017-08-04T20:55:47Z

@zuercher ping on the flags & EV_EOF instead of getpeername() hack.

mattklein123 · 2017-08-04T20:58:46Z

Ah maybe @PiotrSikora knows how to get rid of the crazy hack. Nice.

zuercher · 2017-08-05T02:32:56Z

Sorry, I was deep in bazel internals all day trying to figure out a better solution for my other open PR.

I plan to revisit this and get rid of that ifdef if possible. I'm off Monday, so not much will happen till Tuesday.

Empirically, that block does get triggered but when it does the read ends up returning EAGAIN. Then the next call to onWriteReady gets no errors. Taking the block out crashes some integration tests. It seems like the socket isn't fully connected somehow, but that seems crazy as well. I'm hopeful that I can inspect the event earlier and detect the EAGAIN case before we ever call onWriteReady.

zuercher · 2017-08-08T22:53:52Z

So, I can't directly look at EV_EOF (as @PiotrSikora suggested) because the kqueue event is hidden inside libevent.

I believe the problem occurs when ConnectionImpl::write triggers a FileTypeReady::Write event before a connection has been formed. The eventual call to writev within libevent returns an error, but instead of errno == EAGAIN we see errno == ENOTCONN. If we treat that result the same as EAGAIN, things seem to work. This is basically the same condition as the original #ifdef, but without the extra system calls. It also seems to change the sequence of events in some of the unit tests, which I'm working through.

…ckets and set flags as needed

zuercher · 2017-08-09T19:30:36Z

I eventually realized that ENOTCONN in both the original #ifdef and the one I described above was caused by this sequence in tests:

Construct client-side ConnectionImpl
Invoke ConnectionImpl::write. This buffers the write data and activates an artificial write event because the ConnectionImpl is not in the connecting state.
Invoke ConnectionImpl::connect. The connect call sees EINPROGRESS. Connecting state is flagged.
When dispatcher's run loop is triggered, the artificial write event tricks ConnectionImpl::onWriteReady into thinking the connection completed and it attempts to write to the socket, which fails with ENOTCONN. (Or in the #ifdef code, getpeername returns ENOTCONN and read returns EAGAIN.)

The original ifdef code worked by ignoring the ENOTCONN error and waiting for the real write event that occurs when the connection completes. You can get a similar effect by ignoring the ENOTCONN from write, however things become tricky in the tests, because there's no way to tell when the connection is actually completed (the ConnectionEvent::Connected is triggered spuriously by the artificial write event).

Instead, I added an Unconnected state to ConnectionImpl::InternalState and skip generating the artificial write event if either that or Connecting are set. Now the ConnectionEvent::Connected event is triggered only when the connection actually completes, and subsequent errors from write can be trusted. This then allows tests to invoke the dispatcher's run loop in blocking mode to wait for the connection to complete before attempting to write.

mattklein123 · 2017-08-09T20:23:17Z

source/common/network/connection_impl.h

+    static const uint32_t Connecting               = 0x02;
+    static const uint32_t CloseWithFlush           = 0x04;
+    static const uint32_t ImmediateConnectionError = 0x08;
+    static const uint32_t Unconnected              = 0x10;


What if we just start client connections in the Connecting state? Is there any downside to that? Do we need Unconnected state for anything?

That should work. Just me being pedantic.

mattklein123 · 2017-08-09T21:42:09Z

source/common/network/connection_impl.cc

  int new_value = enable;
  rc = setsockopt(fd_, IPPROTO_TCP, TCP_NODELAY, &new_value, sizeof(new_value));
+#ifdef __APPLE__
+  if (-1 == rc && errno == EINVAL) {


When does this happen? Is this needed with the connected fixes?

Hm. This still happens and I think it's basically the same reason.

We call ConnectionImpl::connect() and then immediately ConnectionImpl::noDelay(true), but the socket isn't connected yet, so OS X returns an error.

I added some code to read the value of TCP_NODELAY and it does get set despite the error.

I think the way to get rid of this #ifdef is to only set TCP_NODELAY after the socket is connected. That means either changing all the call sites to invoke this from a callback or else tracking the desired setting and applying in onWriteReady.

This seems like an OS X bug to me, but I digress. Hmm. In this case I would be in favor of just putting in more of a comment on why this is done/breaks and maybe a TODO?

Added a comment.

I would also file an issue on this and reference it here. I'm imagining some point in the future where we're looking at performance on OS X, and the TCP_NODELAY fail ends up being due to some reason that does not actually set the option. We will then have non-deterministic performance characteristics. This is only slightly hypothetical - we've recently been chasing down a TCP proxy overhead issue that relates somewhat to TCP_NODELAY on envoy-users.

mattklein123 · 2017-08-09T21:42:45Z

test/common/network/proxy_protocol_test.cc

          server_connection_ = std::move(conn);
          server_connection_->addConnectionCallbacks(server_callbacks_);
          server_connection_->addReadFilter(read_filter_);
-          printf("argh!\n");


oops nevermind. You did del. :)

I did. It snuck in via a different PR of mine. 😬

mattklein123 · 2017-08-09T21:43:36Z

source/common/network/connection_impl.cc

    : ConnectionImpl(dispatcher, address->socket(Address::SocketType::Stream), address,
-                     getNullLocalAddress(*address), false) {}
+                     getNullLocalAddress(*address), false) {
+  markConnecting();


nit: IMO it would be better to just pass a boolean to ConnectionImpl constructor saying whether to mark connecting or not vs. having a method.

mattklein123

Sorry two more nits. LGTM. @PiotrSikora @dnoe @htuch ?

mattklein123 · 2017-08-10T02:55:56Z

source/common/network/address_impl.cc

+    if (version == IpVersion::v6) {
+      domain = AF_INET6;
+    } else {
+      RELEASE_ASSERT(version == IpVersion::v4);


nit: I would make this normal ASSERT

mattklein123 · 2017-08-10T02:56:05Z

source/common/network/address_impl.cc

+      domain = AF_INET;
+    }
+  } else {
+    RELEASE_ASSERT(type() == Type::Pipe);


nit: I would make this normal ASSERT

htuch

LGTM if you can file that issue on TCP_NODELAY. Thanks.

danmcd · 2020-01-30T15:53:48Z

Seeing #1446 also reference illumos, I'm curious if at least some of the fixes here may need to apply to illumos (and possibly Oracle Solaris) as well?

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This PR adds the Anthropic logo to the LLM providers section and sorts all providers alphabetically for better organization. **Changes:** - Added Anthropic SVG logo to static assets - Updated provider list in both README.md and website component - Implemented alphabetical sorting for the provider list on homepage - Updated website description to clarify that providers are displayed alphabetically **Related Issues/PRs:** Related PR: #1369 --------- Signed-off-by: Erica Hughberg <erica.sundberg.90@gmail.com>

network: various fixes for OS X socket behavior

71476ed

zuercher commented Aug 2, 2017

View reviewed changes

fix format

2270972

mattklein123 reviewed Aug 3, 2017

View reviewed changes

htuch reviewed Aug 3, 2017

View reviewed changes

PiotrSikora reviewed Aug 3, 2017

View reviewed changes

revert SO_ORIGINAL_DST change

3995df1

mattklein123 previously approved these changes Aug 4, 2017

View reviewed changes

Merge branch 'master' into osx-7-socket-fixes

7551e41

zuercher added 2 commits August 9, 2017 12:10

remove #ifdef __APPLE__, fix connect/write race

7af5182

network: provide a single Address::InstanceBase function to create so…

45317b4

…ckets and set flags as needed

zuercher dismissed mattklein123’s stale review via 45317b4 August 9, 2017 19:17

Merge branch 'master' into osx-7-socket-fixes

65dca33

singleton: add missing #include

45bca1e

mattklein123 reviewed Aug 9, 2017

View reviewed changes

remove InternalState::Unconnected and just set Connecting

887eef3

mattklein123 reviewed Aug 9, 2017

View reviewed changes

remove markConnecting in favor of constructor param

cd4b87e

mattklein123 reviewed Aug 10, 2017

View reviewed changes

plain ASSERT for address types

73108cc

htuch approved these changes Aug 10, 2017

View reviewed changes

mattklein123 approved these changes Aug 10, 2017

View reviewed changes

mattklein123 merged commit 2f180a6 into envoyproxy:master Aug 10, 2017

zuercher deleted the osx-7-socket-fixes branch August 10, 2017 16:07

zuercher mentioned this pull request Aug 11, 2017

OSX: setting TCP_NODELAY sometimes produces an error #1446

Closed

jpsim pushed a commit that referenced this pull request Nov 28, 2022

docs: update swift tests instructions (#1381)

7adbf0c

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

docs: update swift tests instructions (#1381)

2cba2dd

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

zuercher commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher Aug 2, 2017 • 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!

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Aug 4, 2017

Uh oh!

mattklein123 commented Aug 4, 2017

Uh oh!

zuercher commented Aug 5, 2017

Uh oh!

zuercher commented Aug 8, 2017

Uh oh!

zuercher commented Aug 9, 2017

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

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!

zuercher commented Aug 2, 2017 •

edited

Loading

zuercher Aug 2, 2017 •

edited

Loading

mattklein123 Aug 10, 2017 •

edited

Loading