Skip to content
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

uvloop does not support original unix udp socketpair #304

Closed
ZhuYuJin opened this issue Dec 11, 2019 · 9 comments · Fixed by #341
Closed

uvloop does not support original unix udp socketpair #304

ZhuYuJin opened this issue Dec 11, 2019 · 9 comments · Fixed by #341
Labels

Comments

@ZhuYuJin
Copy link

ZhuYuJin commented Dec 11, 2019

import asyncio
import os, socket
import uvloop
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())

try:
    os.unlink("./tmp")
except:
    pass
try:
    os.unlink("./tmp1")
except:
    pass

socket_pair = socket.socketpair(socket.AF_UNIX, socket.SOCK_DGRAM, 0)

class WorkerConnection(asyncio.Protocol):
    def __init__(self):
        print("init connection in worker")
        self.aio_connection = None

    def connection_made(self, transport):
        self.aio_connection = transport

    def datagram_received(self, data: bytes, addr):
    	print(data)

class DispatcherConnection(asyncio.Protocol):
    def __init__(self):
        print("init connection in dispatcher")
        self.aio_connection = None

    def connection_made(self, transport):
        self.aio_connection = transport

    def datagram_received(self, data: bytes, addr):
    	print(data)

loop = uvloop.new_event_loop()
asyncio.set_event_loop(loop)

coros = loop.create_datagram_endpoint(WorkerConnection, sock=socket_pair[0])
(transport1, protocol1) = loop.run_until_complete(coros)

coros = loop.create_datagram_endpoint(DispatcherConnection, sock=socket_pair[1])
(transport2, protocol2) = loop.run_until_complete(coros)

transport1.sendto(b'transport1')
transport2.sendto(b'transport2')

loop.run_forever()

I initialize a unix udp socketpair, and try to use uvloop to manage both sockets.
The code above has the following err:
image

However, when I bind address to both sockets, the err disappears.

socket_pair[0].bind("./tmp")
socket_pair[1].bind("./tmp1")

It shows uvloop does not support the original unix udp socketpair.

@1st1 1st1 added the bug label Jan 16, 2020
@fantix
Copy link
Member

fantix commented Apr 4, 2020

This seems to be a low-level behavior:

#include <unistd.h>
#include <stdint.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/errno.h>
#include <sys/ioctl.h>


void do_send(int fd) {
    int32_t w_buf = 32;
    write(fd, &w_buf, sizeof(w_buf));
}

void do_recv(int fd) {
    // copied from uv__udp_recvmsg
    struct sockaddr_storage peer;
    struct msghdr h;
    ssize_t nread;
    struct iovec buf;
    int r;
    int nonblocking = 1;
    buf.iov_base = malloc((32) * sizeof(char));
    buf.iov_len = 32;

    memset(&h, 0, sizeof(h));
    memset(&peer, 0, sizeof(peer));
    h.msg_name = &peer;
    h.msg_namelen = sizeof(peer);
    h.msg_iov = (void*) &buf;
    h.msg_iovlen = 1;

    // copied from uv__nonblock_ioctl
    do
        r = ioctl(fd, FIONBIO, &nonblocking);
    while (r == -1 && errno == EINTR);
    if (r) {
        printf("ioctl errno: %d %s\n", r, strerror(errno));
    }

    nread = recvmsg(fd, &h, 0);
    if (nread == -1 || errno != 0) {
        printf("%ld %s\n", nread, strerror(errno));
    } else {
        printf("nread=%ld sa_family=%d msg_namelen=%d\n", nread, ((struct sockaddr *)&peer)->sa_family, h.msg_namelen);
    }
}

int main() {
    int socket_vector[2];

    // use either SOCK_DGRAM or SOCK_STREAM
    if(0 != socketpair(AF_UNIX, SOCK_DGRAM, 0, socket_vector)) {
        perror("cannot create socket pair");
        return 2;
    }

    do_send(socket_vector[0]);
    do_recv(socket_vector[1]);
}
OS socketpair type nonblocking peer.sa_family msg_namelen
Linux SOCK_DGRAM 0 AF_UNSPEC 0
Linux SOCK_DGRAM 1 AF_UNSPEC 0
Linux SOCK_STREAM 0 AF_UNSPEC 0
Linux SOCK_STREAM 1 AF_UNSPEC 0
macOS SOCK_DGRAM 0 AF_UNIX 16
macOS SOCK_DGRAM 1 AF_UNIX 16
macOS SOCK_STREAM 0 AF_UNSPEC 0
macOS SOCK_STREAM 1 AF_UNSPEC 0

And ((struct sockaddr_un *) &peer)->sun_path is always an empty string.

Related code:

@ZhuYuJin 's code fails because __convert_sockaddr_to_pyaddr doesn't support AF_UNSPEC. @1st1 shall we patch __convert_sockaddr_to_pyaddr so that it returns an empty string if the family is AF_UNSPEC like this (Solution A)?

diff --git a/uvloop/dns.pyx b/uvloop/dns.pyx
index a252a24..843b5d5 100644
--- a/uvloop/dns.pyx
+++ b/uvloop/dns.pyx
@@ -65,6 +67,9 @@ cdef __convert_sockaddr_to_pyaddr(const system.sockaddr* addr):
         addr_un = <system.sockaddr_un*>addr
         return system.MakeUnixSockPyAddr(addr_un)

+    elif addr.sa_family == uv.AF_UNSPEC:
+        return ""
+
     raise RuntimeError("cannot convert sockaddr into Python object")

Alternatively, we could patch udp.pyx to handle it like this (Solution B):

diff --git a/uvloop/handles/udp.pyx b/uvloop/handles/udp.pyx
index 675407d..5a0f225 100644
--- a/uvloop/handles/udp.pyx
+++ b/uvloop/handles/udp.pyx
@@ -330,6 +330,8 @@ cdef void __uv_udp_on_receive(uv.uv_udp_t* handle,

     if addr is NULL:
         pyaddr = None
+    elif addr.sa_family == uv.AF_UNSPEC:
+        pyaddr = ""
     else:
         try:
             pyaddr = __convert_sockaddr_to_pyaddr(addr)

Or less efficiently but support potential future changes to __convert_sockaddr_to_pyaddr (Solution C):

diff --git a/uvloop/handles/udp.pyx b/uvloop/handles/udp.pyx
index 675407d..16aced6 100644
--- a/uvloop/handles/udp.pyx
+++ b/uvloop/handles/udp.pyx
@@ -334,8 +334,11 @@ cdef void __uv_udp_on_receive(uv.uv_udp_t* handle,
         try:
             pyaddr = __convert_sockaddr_to_pyaddr(addr)
         except BaseException as exc:
-            udp._error(exc, False)
-            return
+            if addr.sa_family == uv.AF_UNSPEC:
+                pyaddr = ""
+            else:
+                udp._error(exc, False)
+                return

     if nread < 0:
         exc = convert_error(nread)

@ZhuYuJin
Copy link
Author

@fantix It should solve the problem.
However, I specify a unix socket pair. Why does it become a AF_UNSPEC?
This code performs well in Mac OS.

@fantix
Copy link
Member

fantix commented Apr 17, 2020

Yeah, I think it is from Linux kernel - I tried to track it down but it's a lot of code to read, so we might want to work it around in uvloop first.

@1st1
Copy link
Member

1st1 commented Apr 17, 2020

C or B would work. How does CPython deal with this btw?

@fantix
Copy link
Member

fantix commented Apr 17, 2020

That's a really good point - let me check.

@fantix
Copy link
Member

fantix commented Apr 17, 2020

Looks like it'll just return None:

https://github.com/python/cpython/blob/87502ddd710eb1f030b8ff5a60b05becea3f474f/Modules/socketmodule.c#L1335-L1341

static PyObject *
makesockaddr(SOCKET_T sockfd, struct sockaddr *addr, size_t addrlen, int proto)
{
    if (addrlen == 0) {
        /* No address -- may be recvfrom() from known socket */
        Py_RETURN_NONE;
    }

Guess solution D would be to update __convert_sockaddr_to_pyaddr to include this logic.

@1st1
Copy link
Member

1st1 commented Apr 22, 2020

Guess solution D would be to update __convert_sockaddr_to_pyaddr to include this logic.

Yep!

@fantix
Copy link
Member

fantix commented Apr 25, 2020

Bad news: libuv didn't provide the addrlen (the msg_namelen below as a value-result argument) for callbacks (I'll file an issue in libuv):

https://github.com/libuv/libuv/blob/16910d23a34b8b8c313d24824fd85383aa8a8d40/src/unix/udp.c#L180-L200

static void uv__udp_recvmsg(uv_udp_t* handle) {
  struct sockaddr_storage peer;
  struct msghdr h;
  ...
    memset(&h, 0, sizeof(h));
    memset(&peer, 0, sizeof(peer));
    h.msg_name = &peer;
    h.msg_namelen = sizeof(peer);
    ...
      handle->recv_cb(handle, nread, &buf, (const struct sockaddr*) &peer, flags);

And I don't think it is a reliable way to check if the first byte is \0 as an equivalent to checking addrlen == 0 in __convert_sockaddr_to_pyaddr, because we don't know for sure what addr->sa_family is.

In this case, I would go for solution B if no objections.


By the way, @ZhuYuJin 's code doesn't work with vanilla asyncio - it uses sendto if peername is not set:

https://github.com/python/cpython/blob/515fce4fc4bb0d2db97b17df275cf90640017f56/Lib/asyncio/selector_events.py#L1043-L1046

    def sendto(self, data, addr=None):
        ...
                if self._extra['peername']:
                    self._sock.send(data)
                else:
                    self._sock.sendto(data, addr)

But the peername is always empty for a socketpair:

import socket

s1, s2 = socket.socketpair(socket.AF_UNIX, socket.SOCK_DGRAM, 0)
print("peername:", repr(s1.getpeername()))
s1.sendto(b'xxx', None)
s1.sendto(b'xxx', b'')
peername: ''
Traceback (most recent call last):
  File "t304.py", line 5, in <module>
    s1.sendto(b'xxx', None)
TypeError: a bytes-like object is required, not 'NoneType'

Traceback (most recent call last):
  File "t304.py", line 5, in <module>
    s1.sendto(b'xxx', b'')
OSError: [Errno 56] Socket is already connected

Should we consider this a bug in CPython asyncio and fix it there too?

@1st1
Copy link
Member

1st1 commented Apr 26, 2020

Yeah, let's to B.

Bad news: libuv didn't provide the addrlen (the msg_namelen below as a value-result argument) for callbacks (I'll file an issue in libuv):

I think I also encountered this limitation myself one time, so resolving this at the libuv level would be great. IIRC I also had problems with dealing with abstract unix sockets in libuv, and maybe it was somehow related to this, I don't remember now.

fantix added a commit to fantix/uvloop that referenced this issue Apr 27, 2020
fantix added a commit to fantix/uvloop that referenced this issue Apr 27, 2020
fantix added a commit to fantix/uvloop that referenced this issue Apr 27, 2020
fantix added a commit that referenced this issue Apr 28, 2020
This was referenced Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants