Implement highlevel unix socket listeners #3187
Conversation
| if sys.platform != "win32": | ||
| sock.setsockopt(tsocket.SOL_SOCKET, tsocket.SO_REUSEADDR, 1) | ||
|
|
||
| await sock.bind(str(fspath)) |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Also the original PR seems to handle this path being e.g. a directory. Does this PR? (yet another test!)
| @@ -31,6 +32,8 @@ | |||
| errno.ENOTSOCK, | |||
| } | |||
|
|
|||
| HAS_UNIX: Final = hasattr(tsocket, "AF_UNIX") | |||
There was a problem hiding this comment.
IMO this is cleaner than the definition in _highlevel_open_unix_listeners.py
src/trio/_highlevel_socket.py
Outdated
| def __init__( | ||
| self, | ||
| socket: SocketType, | ||
| path: str | bytes | PathLike[str] | PathLike[bytes] | None = None, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Was looking deeper into things noted in #279 and saw this comment: #279 (comment)
There was a problem hiding this 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3187 +/- ##
====================================================
- Coverage 100.00000% 99.93542% -0.06459%
====================================================
Files 124 126 +2
Lines 18460 18581 +121
Branches 1216 1225 +9
====================================================
+ Hits 18460 18569 +109
- Misses 0 7 +7
- Partials 0 5 +5
|
|
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! |
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.
|
|
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. |
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