Implement highlevel unix socket listeners by CoolCat467 · Pull Request #3187 · python-trio/trio

CoolCat467 · 2025-01-15T04:21:17Z

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

A5rocks

Preliminary comments but I'd like to see some tests!

@Tronic if you see this then maybe you can check these changes seem sane? (alternatively, feel free to move them all to your PR and we can switch to merging that)

src/trio/_highlevel_open_unix_listeners.py

A5rocks · 2025-01-15T04:45:18Z

src/trio/_highlevel_open_unix_listeners.py

+        if sys.platform != "win32":
+            sock.setsockopt(tsocket.SOL_SOCKET, tsocket.SO_REUSEADDR, 1)
+
+        await sock.bind(str(fspath))


I'm concerned about this because the original PR does some fun stuff here. Specifically, what if there are two calls to this trying to make a file, does this work? Could we add some tests for this? (along with other tests -- just noticed there are none.)

Also the original PR seems to handle this path being e.g. a directory. Does this PR? (yet another test!)

src/trio/_highlevel_open_unix_listeners.py

A5rocks · 2025-01-15T04:50:53Z

src/trio/_highlevel_socket.py

    errno.ENOTSOCK,
 }

+HAS_UNIX: Final = hasattr(tsocket, "AF_UNIX")


IMO this is cleaner than the definition in _highlevel_open_unix_listeners.py

src/trio/_highlevel_socket.py

newsfragments/3187.feature.rst

…ketListener`

A5rocks · 2025-01-15T05:34:00Z

src/trio/_highlevel_socket.py

+    def __init__(
+        self,
+        socket: SocketType,
+        path: str | bytes | PathLike[str] | PathLike[bytes] | None = None,


I think I preferred how the old version didn't require this path argument. Is this really required? I assume we can just check whether a socket is unix type with socket.family == getattr(tsocket, "AF_UNIX", None) and have the old logic to get the path.

Was looking deeper into things noted in #279 and saw this comment: #279 (comment)

Ah, but I missed something, we only have to care about that if we are renaming socket files, which this implementation does not do, so it would be fine to do the original thing.

codecov · 2025-01-15T07:25:13Z

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 ⚠️
.../trio/_tests/test_highlevel_open_unix_listeners.py	96.20253%	2 Missing and 1 partial ⚠️

❌ 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.
❌ 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%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

A5rocks · 2025-01-15T07:27:52Z

Rereading #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!

Sxderp · 2025-01-15T18:16:13Z

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.

CoolCat467 · 2025-01-15T18:24:08Z

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.

jakkdl · 2025-01-16T15:23:34Z

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

trio/src/trio/_tests/test_exports.py

Lines 446 to 456 in 75cc5df

    
           # These are (mostly? solely?) *runtime* attributes, often set in 
        
           # __init__, which doesn't show up with dir() or inspect.getmembers, 
        
           # but we get them in the way we query mypy & jedi 
        
           EXTRAS = { 
        
               trio.DTLSChannel: {"peer_address", "endpoint"}, 
        
               trio.DTLSEndpoint: {"socket", "incoming_packets_buffer"}, 
        
               trio.Process: {"args", "pid", "stderr", "stdin", "stdio", "stdout"}, 
        
               trio.SSLListener: {"transport_listener"}, 
        
               trio.SSLStream: {"transport_stream"}, 
        
               trio.SocketListener: {"socket"}, 
        
               trio.SocketStream: {"socket"},

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

…failure if exists

Tronic · 2025-01-20T14:31:49Z

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.

CoolCat467 added 2 commits January 14, 2025 22:15

Implement highlevel unix socket listeners

b60728d

Add newsfragment

b916c3f

A5rocks requested changes Jan 15, 2025

View reviewed changes

A5rocks reviewed Jan 15, 2025

View reviewed changes

newsfragments/3187.feature.rst Show resolved Hide resolved

Add path functionality to SocketListener instead of making `UnixSoc…

c922a52

…ketListener`

A5rocks reviewed Jan 15, 2025

View reviewed changes

CoolCat467 added 2 commits January 15, 2025 00:37

Add tests for _highlevel_open_unix_listeners

fe7ef60

Revert SocketListener holding on to path, just ask unix socket.

ec76454

Fix pyright issue

75cc5df

CoolCat467 added 3 commits January 16, 2025 18:34

Add comment to exports test, fix test issue, and only unlink file on …

705bd96

…failure if exists

Help macos socket paths not be too long

e8232b4

Fix macos mkstemp

87e77d0

Merge branch 'python-trio:main' into unix-socket-server

f1b13fa

Uh oh!

Conversation

CoolCat467 commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

A5rocks Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

A5rocks Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

A5rocks Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

A5rocks Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

CoolCat467 Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

CoolCat467 Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

A5rocks commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sxderp commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CoolCat467 commented Jan 15, 2025

Uh oh!

jakkdl commented Jan 16, 2025

Uh oh!

Tronic commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CoolCat467 commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

A5rocks commented Jan 15, 2025 •

edited

Loading

Sxderp commented Jan 15, 2025 •

edited

Loading