network: implement early close detection for OS X #4299
Conversation
Implements early close detection on OS X by inspecting the TCP state using getsockopt. *Risk Level*: low (conditional compilation) *Testing*: integration tests pass (tested against envoyproxy#4232) *Docs Changes*: n/a *Release Notes*: n/a *Fixes*: envoyproxy#4294 Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
zuercher
force-pushed
the
stephan/early-close-detection
branch
from
dd2ff02 to
f4fb622
Compare
|
N.B. I'll be out of town until Tuesday. This change makes it possible to avoid disabling tests in #4232. |
|
@alyssawilk do you have any thoughts? |
There was a problem hiding this comment.
Unless there's a compelling reason to support this I'd be inclined to just note this as something which is unsupported in Envoy rather than try to implement in userspace something which just isn't supported by the OS. I don't think it's worth the extra complexity.
| events |= Event::FileReadyType::Closed; | ||
| } else { | ||
| ENVOY_CONN_LOG(trace, "pending read in early close detection", *this); | ||
| disabled_read_pending_ = true; |
There was a problem hiding this comment.
Why isn't this set true under readDisable? I'm not sure why it would differ from !read_enabled_
There was a problem hiding this comment.
It indicates that a read event occurred while !read_enabled_. I'll try to think of a better name.
| socklen_t bytes_size = sizeof(int); | ||
| const Api::SysCallIntResult result = | ||
| os_syscalls.getsockopt(fd(), SOL_SOCKET, SO_NREAD, &bytes, &bytes_size); | ||
| ASSERT(0 == result.rc_); |
There was a problem hiding this comment.
can you not get EAGAIN for this?
There was a problem hiding this comment.
Perhaps, but as you noted, we don't need to attempt to support this for UDS, so this call can be removed.
| if (detectEarlyClose()) { | ||
| // No longer connected. Convert to a closed event. | ||
| ENVOY_CONN_LOG(trace, "early close detection triggered", *this); | ||
| events |= Event::FileReadyType::Closed; |
There was a problem hiding this comment.
shouldn't you remove the read regardless?
There was a problem hiding this comment.
That's done further down, outside the if/else.
| bool ConnectionImpl::detectEarlyClose() { | ||
| auto& os_syscalls = Api::OsSysCallsSingleton::get(); | ||
|
|
||
| if (is_uds_) { |
There was a problem hiding this comment.
does uds support this on non-apple? I thought we already discovered that was broken?
There was a problem hiding this comment.
Fair. It can just return false for this case.
|
The benefit this provides is not having to special case a bunch of tests. Not sure if that's sufficiently compelling. |
|
My personal preference would be to skip it - I think now that we know macos doesn't have early error detection it'll be easy to notice and disable these tests for apple and as our main deflaker I'm up for taking that on over making the connection class harder to read :-). I'm also OK being overruled or picking an arbitrary tiebreaker - I can take another pass tomorrow if you decide to go forward. |
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
|
I went ahead and updated this and made the test changes simpler. I'll close it once the CI checks complete and update the issue in case anyone ever wants to resurrect this (e.g. for the Windows port, provided there's a similar trick available there). |
Implements early close detection on OS X by inspecting the TCP
state using getsockopt.
This PR is a bit bigger than it strictly needs to be. In order to detect closure on both TCP and unix sockets under OSX, ConnectionImpl needs to know which type of socket it has. I elected to move the UDS check into the constructor from the noDelay method (to avoid retesting this on every call to detectEarlyClose()). Because of that change, I ended up modifying ConnectionImpl to use the Api::OsSysCalls singleton so that I could mock the getsockopt call in connection_test_impl and then had to mock getsockopt calls in ProxyProtocolTest. If all that seems like too much, I think we could probably revert ConnectionImpl to use direct system calls and modify connection_impl_test to create a socket that would allow the UDS check to work.
Risk Level: low (conditional compilation for OSX)
Testing: integration tests pass (tested against #4232)
Docs Changes: n/a
Release Notes: n/a
Fixes: #4294
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io