Socket binding implemented properly for IPv6 and UNIX sockets. #1641
Conversation
- app.run("::1") for IPv6
- app.run("unix:/tmp/server.sock") for UNIX sockets
- app.run("localhost") retains old functionality (randomly either IPv4 or IPv6)
Do note that IPv6 and UNIX sockets are not fully supported by other Sanic facilities.
In particular, request.server_name and request.server_port are currently unreliable.
Codecov Report
@@ Coverage Diff @@
## master #1641 +/- ##
==========================================
+ Coverage 91.70% 92.26% +0.55%
==========================================
Files 27 27
Lines 3001 3075 +74
Branches 544 552 +8
==========================================
+ Hits 2752 2837 +85
+ Misses 171 165 -6
+ Partials 78 73 -5
Continue to review full report at Codecov.
|
|
Compatibility with create_server (used when workers=1) remains an issue. uvloop uses quite different approach to binding than Sanic's implementation, making this difficult to manage. UNIX sockets are useful (among other things) for nginx proxy_pass, where they perform much better than 127.0.0.1. |
sanic/server.py
Outdated
| name = host[5:] | ||
| sock = socket.socket(socket.AF_UNIX) | ||
| if os.path.exists(name) and os.stat(name) == stat.S_ISSOCK: | ||
| os.unlink(name) |
There was a problem hiding this comment.
is this because we can't clean up the socket on server close?
There was a problem hiding this comment.
Unlinking of existing socket? Standard practice that I didn't pay much attention to.
There was a problem hiding this comment.
I don't really have a preference, its inconsistent with what happens when you try to bind to a port that's in use though so I figured I'd mention it
There was a problem hiding this comment.
Sockets are left over e.g. after kill -9, so removing them is practical, and apparently asyncio's and uvloop's create_unix_server both will overwrite any existing socket this way. Also, neither of them removes the socket on close.
I've added the previously missing unlink on exit to serve() as well, for unix sockets created within this function. As a result, both modes of Sanic (workers=1 and serve_multiple) now (1) remove any existing unix socket before bind and (2) remove the socket after normal shutdown.
There was a problem hiding this comment.
@abuckenheimer Thanks for your notes/reviews! I'm off keyboard until Monday. Have a nice weekend :)
|
👍 for UNIX sockets. In addition, the implementation looked good. |
|
https://github.com/huge-success/sanic/blob/master/sanic/request.py#L401 |
This would conflict with #1638, which I expect to be merged first. Normally, however, even unix socket requests should have a Host header, which would then be used. E.g. try
With #1638 the existence of Host or other forwarding headers imply scheme's default port if not explicitly mentioned, and I think this would be the best fallback if no such headers are present: return 443 for SSL connections (https, wss) and 80 for anything else. |
…oin' fast @ unix-socket fixed.
|
New unix sockets are created with temporary names, set to listen and then moved over the existing socket to atomically replace it. This allows zero-downtime restarts, no connection attempts may fail. A new server may be started while the old one is still running. The old instance will not see any new requests but it may finish its pending requests while at the same time the new instance is already servicing new requests. |
sanic/server.py
Outdated
| sock = socket.socket(socket.AF_UNIX) | ||
| try: | ||
| # Atomic zero-downtime socket replace | ||
| tmp_path = f"{path}.{uuid4().hex[:8]}" |
There was a problem hiding this comment.
Would os.getpid() be a simpler solution? It is already ensured to be unique identifier (at least inside a pid namespace (e.g. single container)). If the file would exists, it's would be from an already dead process.
There was a problem hiding this comment.
Stale sockets might still be left from before reboot, causing services fail to start. There are apparently also some security concerns, in particular that if another user creates a symlink in place of the socket, bind may follow that symlink and create a socket at symlink's target. Eight random hex digits is far less likely to be hit by these issues than a pid, although it might be necessary to use even more.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=47492 (Linux has fixed the bug long ago but MacOS is still vulnerable)
A minor issue with this approach is that the initial random name sticks, and is returned as sockname on the server side and as peername on the clients.
|
Is anyone aware of why request checks peername and sockname from protocol instead of protocol filling these into requests? The only reasons I can think of are (1) it was the fastest way to hack it, or (2) not to consume runtime when these values are not used. Since the values need to be read from socket only once per connection, not per request, I plan to change this to happen right after connection. |
…w overrides everything.
…ket. - Would be a good idea to remove request.transport entirely but I didn't dare to touch it yet.
|
Quite many potentially breaking changes again. @abuckenheimer can you have a look and comment? |
|
Current implementation is balancing between backward compatibility and sensible implementation. And I didn't benchmark at all, in particular the ASGI server that creates new ConnInfo for every request. Probably no slowdown still, but needs to be tested. In any case, this will require further refining before merging. |
|
Also, it'd be great if someone could write tests for unix sockets (I have no idea how to fit that into the testing framework which afaik doesn't even test live servers). |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions. |
…mporary name in conn_info.
|
I'm done with this. Requesting review. |
|
This doesn't need to go in 20.3 but the ConnInfo structure is something I'll be using in the streaming branch as soon as this gets merged. |
|
@huge-success/sanic-core-devs Anyone care to review? |
|
@ahopkins Are you still including this one? |
|
Hmm, looks like some unix socket tests got broken now. I suppose that'll need more investigation. |
|
Tests were failing. If you can take a look and solve easily, let's push another release. |
#1853 made workers use spawn rather than fork on all OSes, and consequently doesn't allow using function-local app objects or even route decorators. However, I would expect that to break far more things than only this... Do you know what the current behaviour regards pickling should be? Perhaps something from master was not correctly merged to this? I'll try to investigate. |
|
This sets "spawn" mode, earlier "fork" was used on Linux and MacOS (Windows only supports "spawn"): As a result, with current master, this doesn't work: from sanic import Sanic
app = Sanic(__name__)
@app.get("/")
def hello(req):
pass
app.run(workers=2) |
|
The multiple worker test is now made compatible with either mode of multiprocessing, so that this can be merged. Let's move further discussion about the pickling to issue #1883 or related PR. |
|
This pull request has been mentioned on Sanic Community Discussion. There might be relevant details there: https://community.sanicframework.org/t/multiprocessing-is-currently-not-supported-on-nt/827/2 |
app.run("unix:/tmp/server.sock")app.run(unix="/tmp/server.sock") for UNIX socketsDo note that IPv6 and UNIX sockets are not fully supported by other Sanic facilities.
In particular, request.server_name and request.server_port are currently unreliable.