trio icon indicating copy to clipboard operation
trio copied to clipboard

Implement highlevel unix socket listeners

Open CoolCat467 opened this issue 10 months ago • 6 comments

In this pull request, we implement highlevel unix socket support. This is an alternate implementation of and resolves #1433.

I will say, at the moment, only unix SOCK_STREAM (TCP) sockets are supported with this change, but I could pretty easily implement unix datagram socket listeners as well.

Closes #279

CoolCat467 avatar Jan 15 '25 04:01 CoolCat467

Codecov Report

Attention: Patch coverage is 89.55224% with 14 lines in your changes missing coverage. Please review.

Project coverage is 99.92700%. Comparing base (d65d3f6) to head (f1b13fa). Report is 60 commits behind head on main.

Files with missing lines Patch % Lines
src/trio/_highlevel_open_unix_listeners.py 71.05263% 8 Missing and 3 partials :warning:
.../trio/_tests/test_highlevel_open_unix_listeners.py 96.20253% 2 Missing and 1 partial :warning:

:x: Your patch check has failed because the patch coverage (89.55224%) is below the target coverage (100.00000%). You can increase the patch coverage or adjust the target coverage. :x: Your project check has failed because the head coverage (99.92700%) is below the target coverage (100.00000%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@                 Coverage Diff                  @@
##                 main       #3187         +/-   ##
====================================================
- Coverage   100.00000%   99.92700%   -0.07300%     
====================================================
  Files             124         126          +2     
  Lines           19047       19177        +130     
  Branches         1287        1299         +12     
====================================================
+ Hits            19047       19163        +116     
- Misses              0          10         +10     
- Partials            0           4          +4     
Files with missing lines Coverage Δ
src/trio/__init__.py 100.00000% <100.00000%> (ø)
src/trio/_highlevel_socket.py 100.00000% <100.00000%> (ø)
src/trio/_tests/test_exports.py 100.00000% <ø> (ø)
.../trio/_tests/test_highlevel_open_unix_listeners.py 96.20253% <96.20253%> (ø)
src/trio/_highlevel_open_unix_listeners.py 71.05263% <71.05263%> (ø)
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jan 15 '25 07:01 codecov[bot]

Rereading https://github.com/python-trio/trio/issues/279, now I get why the original PR has the atomic replace. I believe what this PR does is fine, because no atomic replace -> atomic replace is not a breaking change (I think?), compared to no unlinking -> unlinking.

I think it would be better to not support no downtime redeploys and explicitly document this (at least for now!). Given the complexity of the issue I think @njsmith should look over this of course!

A5rocks avatar Jan 15 '25 07:01 A5rocks

I believe what this PR does is fine, because no atomic replace -> atomic replace is not a breaking change (I think?), compared to no unlinking -> unlinking.

It's still compat breaking. I haven't read the code, but I'm assuming in the "before" for both scenarios you error on the path already existing (Address in use, I'd guess). If you start overwriting (via a rename) that's still a change in functionality almost equivalent to unlinking before use.

I think you should figure out the expected pre and post functionality before going stable with it.

  • Do you ignore existing socket files and reuse the path (via unlinking or atomic rename)?
  • Do you ignore existing files and reuse the path (via unlinking or atomic rename)?
  • Do you unlink the socket at the end of the program?
    • PR says yes, but I think it's more common not to? Not really sure honestly. Either way, I think devs have to account for the path existing because the cleanup may not happen on a particularly hard crash.
    • Also not compatible with the atomic rename. Since the PR is taking the option to unlink at the end we find ourselves with another potential functionality change.

Sxderp avatar Jan 15 '25 18:01 Sxderp

CI is failing because of jedi and I am unsure how to go about fixing it. That particular test is incredibly confusing and tells me nothing about what the error means.

CoolCat467 avatar Jan 15 '25 18:01 CoolCat467

CI is failing because of jedi and I am unsure how to go about fixing it. That particular test is incredibly confusing and tells me nothing about what the error means.

it doesn't seem to be just jedi (and mypy), e.g. macOS 3.10 has a bunch of other tests failing. But yeah test_exports is not terribly friendly for developers. But tl;dr the test checks what symbols are available with inspect.getmembers, and then diffs that against what jedi/mypy/etc sees. These symbol lists do differ in a bunch of cases, and if possible you figure out why they differ, if that's a problem, and either ignore it in the test (by adding some logic that skips those symbols) or fix it elsewhere.

What seems to be happening here in particular is that skipping trio.SocketListener.socket and trio.SocketStream.socket are not necessary https://github.com/python-trio/trio/blob/75cc5dfcb3735cc5c1419900cd01e1fc1cb3008d/src/trio/_tests/test_exports.py#L446-L456

That probably also fixes the mypy error (which definitely should have some additional logic/printing what went wrong)

jakkdl avatar Jan 16 '25 15:01 jakkdl

As the author of the original PR, I see most of the comments are about socket creation. It was carefully crafted precisely for zero downtime restarts, and considering Windows has its own definition of REUSEADDR (as opposed to other OSes).

The code in the original PR has been in use by Sanic Framework for many years, and found to function correctly. I would advice against deviating from it, unless you have a very good reason.

I'll not dig deeper into it now, but if you want comments on the new one, tag me again.

Tronic avatar Jan 20 '25 14:01 Tronic