bpo-31245: Asyncio unix socket datagram by ovv · Pull Request #3164 · python/cpython

ovv · 2017-08-21T14:11:08Z

https://bugs.python.org/issue31245

Split the loop.create_unix_datagram_endpoint method into loop.create_unix_datagram_connection and loop.create_unix_datagram_server for cleaner API

vstinner · 2017-08-22T23:08:23Z

+                    # Let's improve the error message by adding
+                    # with what exact address it occurs.
+                    msg = 'Address {!r} is already in use'.format(path)
+                    raise OSError(errno.EADDRINUSE, msg) from None


Please chain the exception.

This was made to improve the error message by including the address that failed the bind. I'm not sure chaining the exception will provide more information on the error.

vstinner · 2017-08-22T23:09:52Z

+        tr = self.socket_datagram_transport(waiter=waiter)
+        self.loop.run_until_complete(waiter)
+
+        self.loop.assert_reader(7, tr._read_ready)


Please use self.sock_fd

vstinner · 2017-08-22T23:13:13Z

+            raise TypeError('data argument must be a bytes-like object, '
+                            'not %r' % type(data).__name__)
+        if not data:
+            return


Are you sure about the semantics? Can't you use send ("") as a cheap ping for example?

I suggest to remove this test.

It seems to be working without the check. I wonder if it's also not necessary for the SelectorDatagramTransport

That's correct – on a datagram socket, sending a packet with empty payload is totally legal, and is different from sending no packet at all.

vstinner · 2017-08-22T23:14:14Z

+        transport = self.socket_datagram_transport()
+        transport._closing = True
+        transport._buffer.append(data)
+        self.loop._add_writer(7, transport._send_ready)


Again, use self.sock_fd

vstinner · 2017-08-22T23:16:15Z

+            coro = self.loop.create_unix_datagram_server(mock.MagicMock,
+                                                         file.name)
+            with self.assertRaisesRegex(OSError,
+                                        'Address.*is already in use'):


I would prefer to check the error code rather than the message.

I will check both. So we make sure the error message is also correctly rewritten

vstinner · 2017-08-22T23:17:30Z

@@ -0,0 +1,2 @@
+Added `create_unix_datagram_connection` and `create_unix_datagram_server` to


Please mention asyncio. Maybe "asyncio: ...".

njsmith · 2017-08-24T01:16:44Z

This is a ton of new code (mostly copy/pasted?) for what's basically just... changing one constant. At the very least can't you share DatagramProtocol and most of the test code?

1st1 · 2017-08-24T01:26:29Z

This is a ton of new code (mostly copy/pasted?) for what's basically just... changing one constant. At the very least can't you share DatagramProtocol and most of the test code?

+1. This should be refactored, and I wouldn't expect to see a patch that's longer than +- 50 lines of code. Also, please don't merge without my final review.

ovv · 2017-08-24T07:46:52Z

Thanks for the feedback. I'll try to refactor to shorten the patch.

Concerning the protocol I'm not sure on the direction to take. I could use the DatagramProtocol, but in my opinion it doesn't make much sense to pass the addr to datagram_received for a unix socket.

ovv · 2017-08-24T14:30:11Z

Well it appears I had some misconception about how unix datagram socket works.

I think this patch is much more reasonable. Sorry for the mess it was previously.

1st1 · 2017-10-19T17:46:34Z

+            elif hasattr(socket, 'AF_UNIX') and family == socket.AF_UNIX:
+                for addr in (local_addr, remote_addr):
+                    if addr is not None:
+                        assert isinstance(addr, str)


Make it a proper exception -- TypeError in this case.

1st1

Please resolve one remaining nit. Otherwise LGTM

bedevere-bot · 2017-10-19T17:47:09Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ovv · 2017-10-30T09:14:54Z

I have made the requested changes; please review again

bedevere-bot · 2017-10-30T09:14:55Z

Thanks for making the requested changes!

@1st1: please review the changes made to this pull request.

1st1 · 2017-10-30T13:43:23Z

Thanks!

See https://github.com/python/cpython/blob/main/Lib/asyncio/base_events.py#L1319 Support for unix sockets was added to this function in python 3.7: python/cpython#3164

Since python 3.7, asyncio.loop.create_datagram_endpoint expects the arguments `local_addr` and `remote_addr` to be `str` or `None` when the argument family=`socket.AF_UNIX` and the argument `sock` is `None` or also a unix socket. Here is a link to the relevant line in the standard library: https://github.com/python/cpython/blob/main/Lib/asyncio/base_events.py#L1319 And here is a link to the pull request for the change which introduced this in python 3.7: python/cpython#3164

Quentin Dawans added 6 commits August 18, 2017 14:39

AF_UNIX SOCK_DGRAM support in asyncio

3a68749

Changes required by Haypo review

a2d0430

Split create_unix_datagram into connection & server

77dfbef

Split the loop.create_unix_datagram_endpoint method into loop.create_unix_datagram_connection and loop.create_unix_datagram_server for cleaner API

delete create_unix_datagram_endpoint

df3b696

loop.create_unix_datagram_* documentation

e5069ba

Tests

db73ada

ovv requested a review from 1st1 as a code owner August 21, 2017 14:11

bedevere-bot added the awaiting review label Aug 21, 2017

ovv changed the title ~~bpo- 31245: Asyncio unix socket datagram~~ bpo-31245: Asyncio unix socket datagram Aug 21, 2017

Quentin Dawans added 3 commits August 22, 2017 09:35

fix travis

6234c20

Remove not needed print

f82fd5f

Misc/News

43db3c1

vstinner reviewed Aug 22, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Aug 23, 2017

Haypore review 2. Use sock_fd, allow sending empty data

94ee0b6

Quentin Dawans added 2 commits August 24, 2017 16:16

bpo-31245: asyncio datagram support for AF_UNIX

c27a05e

documentation

567fdf9

verify socket.AF_UNIX is defined

ff75975

Mariatta added CLA not signed and removed CLA not signed labels Aug 29, 2017

the-knights-who-say-ni added the CLA signed label Aug 29, 2017

1st1 reviewed Oct 19, 2017

View reviewed changes

1st1 requested changes Oct 19, 2017

View reviewed changes

bedevere-bot removed the awaiting core review label Oct 19, 2017

bedevere-bot added the awaiting changes label Oct 19, 2017

Raise TypeError if addr is not string

16b7609

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 30, 2017

asvetlov approved these changes Oct 30, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 30, 2017

1st1 approved these changes Oct 30, 2017

View reviewed changes

1st1 merged commit fe4ea9c into python:master Oct 30, 2017

bedevere-bot removed the awaiting merge label Oct 30, 2017

embray pushed a commit to embray/cpython that referenced this pull request Nov 9, 2017

bpo-31245: Asyncio unix socket datagram (python#3164)

a7c5356

ovv added a commit to Eyepea/cpython that referenced this pull request Dec 29, 2017

bpo-31245: Asyncio unix socket datagram (python#3164)

d78392f

kitt-cat mentioned this pull request Jan 20, 2023

create_datagram_endpoint expects a str when family=AF_UNIX python/typeshed#9574

Merged

		@@ -0,0 +1,2 @@
		Added `create_unix_datagram_connection` and `create_unix_datagram_server` to

Uh oh!

Conversation

ovv commented Aug 21, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njsmith commented Aug 24, 2017

Uh oh!

1st1 commented Aug 24, 2017

Uh oh!

ovv commented Aug 24, 2017

Uh oh!

ovv commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 19, 2017

Uh oh!

ovv commented Oct 30, 2017

Uh oh!

bedevere-bot commented Oct 30, 2017

Uh oh!

1st1 commented Oct 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ovv commented Aug 21, 2017 •

edited by bedevere-bot

Loading