-
-
Notifications
You must be signed in to change notification settings - Fork 754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get the request.client under case uvicorn is run with a fd or a unix socket #729
Conversation
…cket (encode#636)" This reverts commit a796e1d
Modified MockSocket peername to pass tuples instead of list because socket.getpeername() and socket.getsockname() return tuples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that in some cases the socket_info.getpeername()
may return a string containing the path to the Unix socket, and we need to ignore that case, correct?
I'm curious - in what cases, when bound to a Unix socket, can it return a (host, port) tuple? Isn't binding to a Unix socket not done via a host/port by definition? Had we confirmed that #636 did fix #634?
uvicorn/protocols/utils.py
Outdated
if family in (socket.AF_INET, socket.AF_INET6): | ||
return (str(info[0]), int(info[1])) | ||
|
||
elif hasattr(socket, "AF_UNIX") and family is socket.AF_UNIX: | ||
if isinstance(info, tuple): | ||
# fd case | ||
# <uvloop.PseudoSocket fd=13, family=AddressFamily.AF_UNIX, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 8000), raddr=('127.0.0.1', 38634)> | ||
return (str(info[0]), int(info[1])) | ||
else: | ||
# unix socket case | ||
# <uvloop.PseudoSocket fd=21, family=AddressFamily.AF_UNIX, type=SocketKind.SOCK_STREAM, proto=0, laddr=/tmp/gunicorn.sock> | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd do something like this, to simplify:
HAS_AF_UNIX = hasattr(socket, "AF_UNIX")
def get_remote_addr(transport):
...
if family in (socket.AF_INET, socket.AF_INET6) or (
HAS_AF_UNIX and family == socket.AF_UNIX and isinstance(family, tuple)
):
return (str(info[0]), int(info[1]))
return None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just change the whole block to...
return (str(info[0]), int(info[1])) if isinstance(info, tuple) else None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it that way, way simpler,
Just note that I kept in get_remote_addr
the try except introduced it seems in #495
We might want to simply revert #636. It's not clear to me that it did fix anything, given how according to https://docs.python.org/3/library/socket.html there is no host/port information available when binding to a Unix socket. I think #634 was a non-issue ("yes there's no host/port because your server is not bound to a TCP socket"). |
correct
if you run uvicorn with
then you end up having |
I see, so in that case Supervisord's FastCGI support makes the TCP socket available at |
I merged this patch into integration test with nginx - it works https://github.com/bukforks/uvicorn/tree/django_nginx |
I guess we can merge this and get it along for 0.11.8. 👍 #731 |
should solve #727 and still give the correct info that #634 wanted