Skip to content
This repository has been archived by the owner on Nov 23, 2017. It is now read-only.

Add UDP-related methods on the event loop. #321

Closed
wants to merge 1 commit into from
Closed

Add UDP-related methods on the event loop. #321

wants to merge 1 commit into from

Conversation

sbstp
Copy link

@sbstp sbstp commented Feb 21, 2016

Adds sock_recvfrom and sock_sendto to the event loop. Tests were added, based on the other tests. There's no Proactor support yet, because it requires changing overlapped.c.

I did some quick research and it seems that sendto might block, although it usually does not, hence why I also added it.

@asvetlov
Copy link

IIRC methods like sock_recv appeared exactly for sake of Proactor support, selector-based event loop don't need it. add_reader and family should be enough.

That's why I'm considering that Proactor support is crucial for proposed functions (if we want to have them at all).

@sbstp
Copy link
Author

sbstp commented Feb 21, 2016

I'm not against writing the code for Windows. According to the docs, create_datagram_endpoint cannot be used on Windows (using the Proactor loop). If the code for Windows is written, it would improve support for async udp on Windows.

@gvanrossum
Copy link
Member

gvanrossum commented Feb 21, 2016 via email

@sbstp
Copy link
Author

sbstp commented Feb 21, 2016

I agree that the streams.py API is much better than the sock_* methods. I didn't want to put the UDP code there because it's not a streaming protocol.

What if we added a create_datagram() function that creates a UDP socket wrapper with a sendto and a recvfrom coroutine? The returned object could have a bind(address) method, or we could add another method create_datagram_server(address) that creates the socket and binds it instantly.

@gjcarneiro
Copy link

On 21 February 2016 at 17:18, Guido van Rossum notifications@github.com
wrote:

Honestly I still don't want this at all. The sock_recv() methods etc.
are all kind of the wrong level and were added as compromises. Look at
how asyncio/streams.py provides a much more reasonable adaptation
compared to sock_recv() and sock_send().

IMHO, comparing the TCP and UDP cases is not reasonable.

For TCP, the high-level interface you want is asyncio's stream API, so it's
a good thing Stream was added to the library. Otherwise, you have to
manually handle cases where you ask to send N bytes but M<N bytes are
actually sent instead, and similar problem for receiving.

For the case of UDP, the high level interface you could possibly want is
"send this message in its entirety" and "wait until the next entire message
is received", which is exactly the same interface that so called "low
level" datagram sockets provide.

Sure, we could create a high level datagram sending API, but I am not
seeing what value the high level API could add to the plain socket API.

Gustavo J. A. M. Carneiro
Gambit Research
"The universe is always one step beyond logic." -- Frank Herbert

@gvanrossum
Copy link
Member

IMHO, comparing the TCP and UDP cases is not reasonable.

I am the first to agree. This is why I am resisting the addition of
sock_sendto() and sock_recvfrom().

[...]

Sure, we could create a high level datagram sending API, but I am not
seeing what value the high level API could add to the plain socket API.

My preference is to stick with the status quo. The OP seems to prefer
writing loop.sock_recvfrom() over writing a simple DatagramProtocol
subclass (with or without an adapter so he can write
.receive()). Maybe you can help them with the latter so we
can just close this PR?

@gvanrossum
Copy link
Member

What if we added a create_datagram() function that creates a UDP socket
wrapper with a sendto and a recvfrom coroutine? The returned object could
have a bind(address) method, or we could add another method
create_datagram_server(address) that creates the socket and binds it
instantly.

All that functionality already exists in create_datagram_endpoint(),
except for the simple logic that enables you to write "next_packet =
yield from ". Please give that a try rather than sending a
PR.

@sbstp
Copy link
Author

sbstp commented Feb 22, 2016

I will, thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants