Detect broken connections using socket peeking

[florimondmanca]

Fixes#182

Attempts implementing #185 (comment)

Attempt peeking data from the socket to detect whether the connection has reached EOF ("broken connection" / "dropped connection").

Better cross-platform support than the selectors alternative here: #185.

Follow-ups:

• Rename is_connection_dropped() to is_at_eof().

[florimondmanca]

@cdeler@JayH5 Okay, I've revisited the approach for is_socket_at_eof(), simply passing explicitly the fd, family and type so we can properly duplicate the socket in both the TCP and UDS cases. I've done some testing against a Uvicorn server over TCP and UDS, in a standard and "server has disconnected early" scenarios, and everything seems to work well. How is this looking?

[cdeler]

It's brilliant

[JayH5]

LGTM. Kinda curious whether a library like Trio would want to include this kind of low-level function.

[florimondmanca]

Hi @njsmith! Pinging you here just in case you'd have any thoughts on this approach. Since that's not the way trio is doing things (I think?), I'm curious if there's some hidden socket wizardry subtelty we're missing, or if this approach seems legit? For more context feel free to read through #185.

[njsmith]

Trio sockets expose a special is_readable method, exactly for this use case, so on Trio you should just use that :-).

The implementation is pretty simple, and AFAIK there aren't any compatibility issues or anything:

https://github.com/python-trio/trio/blob/20ee2b1b7376db637435d80e266212a35837ddcc/trio/_socket.py#L471-L478

I skimmed #185 but I couldn't really follow what you all were worrying about. If you want to steal that code for non-Trio backends, go for it. Otherwise, I'm not sure what to say b/c I don't know what problem you're trying to solve :-)

On Trio though please do call sock.is_readable instead of pulling out the fileno and doing something custom, because that will allow mock networking for testing to work.

[florimondmanca]

@njsmith Woops — meant to point you at #182, the original issue. Yeah it's a bit fuzzy, here's a quick run-through… (also helpful so I can validate this is all well-posed…)

The problem exposed in #182 is something that came up in urllib3 years ago, which we're vulnerable to as of today: when many files are open and we hit the 1024 fd limit on Unix (512 on Windows), select.select() starts failing.

In #185 I explored using the selectors module, which solves the Unix case by using a suitable selector based on the platform (eg poll, kqueue). But @agronholm mentioned that it wouldn't solve the Windows case because only select.select() is available there (as the trio is_readable() method also mentions).

So this PR takes a different approach, peeking onto the socket rather than using any selector. Then since we're able to get a raw socket object from async backends too, we're now using a generic cross-platform helper (the is_socket_at_eof() function) to do the same "has the socket reached EOF?" logic in both the sync and async cases. (This goes a bit beyond the scope of #182 which only reported an issue in the sync case, but at least we're now consistent everywhere.)

I guess I'm not asking anything here, was just curious what your thoughts would be given your experience with low-level networking via trio. If this still sounds nebulous, then, fair enough. 😄

[njsmith]

The problem exposed in #182 is something that came up in urllib3 years ago, which we're vulnerable to as of today: when many files are open and we hit the 1024 fd limit on Unix (512 on Windows), select.select() starts failing.

select is a very different beast on Unix and Windows.

On Unix, select uses a bitmask of all possible fds up to some numeric limit. So it isn't even the number of fds that you pass in that matters, what matters is the numeric value of your highest-numbered fd. Obviously in modern times this is terrible, so they invented poll, which takes an array of integer fds, so now the numeric values don't matter, just how many of them you're passing in.

On Windows, select has always used an array of socket handles. So Windows select has exactly the same performance characteristics as Unix poll.

There are also the fancier even-more-modern syscalls like epoll and IOCP and all that, which the selectors module tries to use if it can. But those are designed to optimize for when you're watching a large number of sockets at once. In this case we're only trying to query a single socket at a time, so the fancy modern options are actually less efficient than good ol' poll/select.

So I think that six lines of code in Trio will solve all your problems everywhere.

Then since we're able to get a raw socket object from async backends too

Again, please don't try to pull out raw socket objects on Trio, b/c it breaks testing helpers. If there's something you need that's not available on Trio socket objects, lmk and we'll figure out how to add it.

[cdeler]

@florimondmanca

Original issue fixing could be checked if you add the following test:

# conftest.py@pytest.fixture(scope="function")defmany_files() ->typing.Iterator[None]:
files= []
real_file=tempfile.NamedTemporaryFile(delete=False)
try:
real_file.write(b"some")
real_file.flush()
files.append(real_file.fileno())
for_inrange(1023):
fd=os.dup(real_file.fileno())
files.append(fd)
yieldfinally:
forfdinfiles:
os.close(fd)
os.remove(real_file.name)
# test_interface.py@pytest.mark.anyioasyncdeftest_dropped_connection_detection_bug(
backend: str, server: Server, many_files
) ->None:
asyncwithhttpcore.AsyncConnectionPool(backend=backend) ashttp:
method=b"GET"url= (b"http", *server.netloc, b"/")
headers= [server.host_header]
for_inrange(2):
status_code, response_headers, stream, ext=awaithttp.arequest(
method, url, headers
)
awaitread_body(stream)
assertstatus_code==200assertext== {"http_version": "HTTP/1.1", "reason": "OK"}
assertlen(http._connections[url[:3]]) ==1# type: ignore

May be you can add the test to the suite?

As an alternative, you can add the many_files fixture to keep-alive tests

[florimondmanca]

Closing, in favor of #185… :-)