trio
trio copied to clipboard
Implement highlevel unix socket listeners
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
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.
: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.
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!
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.
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.
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)
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.