artfwo / pymonome

python library for interacting with monome devices
MIT License
55 stars 10 forks source link

Let the OS decide local_addr, and add host argument to connect method #10

Open lopter opened 2 years ago

lopter commented 2 years ago

Hello,

Thank you very much for your libraries, I was recently upgrading my monome Python stack (because Python 3.10 is deprecating some old asyncio stuff) and took the opportunity to package it into docker containers cross-compiled with the Nix package manager (https://nixos.org/).

I had to make the following changes to be able to run serialosc and pymonome under different containers.

artfwo commented 1 year ago

Hey @lopter , thanks for the PR. Sorry I must have missed the notification on this before somehow. Could you update this, so a default value for address is provided in the host argument?

I'd also suggest squashing the commits and updating the commit message, so it's clear what was changed, TY!

lopter commented 1 year ago

I did that, do we want to also move the host argument after the loop argument?

It makes the signature a bit less nice imo, but would be backward compatible.

artfwo commented 1 year ago

I did that, do we want to also move the host argument after the loop argument?

i don't think loop argument is frequently used, no need to do that.

how does it work with local_addr arguments being removed from create_datagram_endpoint? is the socket still being bound without it?

lopter commented 1 year ago

i don't think loop argument is frequently used, no need to do that.

👍

how does it work with local_addr arguments being removed from create_datagram_endpoint? is the socket still being bound without it?

Without local_addr bind(2) is not called and the socket cannot receive "connections", instead the OS will automatically choose a local address and port to use as the source for messages sent through that socket.

My understanding is that pymonome is a client for serialosc, why would pymonome need to receive "connections" (i.e. bind the socket used to connect to serialosc)?

artfwo commented 1 year ago

My understanding is that pymonome is a client for serialosc, why would pymonome need to receive "connections" (i.e. bind the socket used to connect to serialosc)?

Right, there's no persistent connection, as both serialosc and pymonome create UDP sockets, but the communication is still bidirectional - both pymonome device and serialosc endpoints receive OSC messages to the ports specified in local_addr (with 0 being an alias for a random free port, that we later send to serialosc with the /sys/port message).

With local_addr set to None, does it work the same way across different platforms without explicitly binding the sockets?

lopter commented 1 year ago

Yes, this does not prevent bidirectional communication, you can play with the UDP echo server & client examples on different platforms to see that: https://docs.python.org/3.11/library/asyncio-protocol.html#udp-echo-server

artfwo commented 1 year ago

Right, but the echo example binds the socket explicitly by specifying local_addr.

Moreover, the case seems to be that the sockets aren't bound at least on Windows until data is being sent:

https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-sendto#remarks

While this does not prevent bidirectional communication, explicit binding both grid and serialosc sockets improves readability and might simplify debugging. I suggest staying on the more explicit side following the zen of Python here, leaving local_addr arguments as they currently are. I can amend this PR as well if you agree.

lopter commented 1 year ago

The echo client (representing pymonome) does not bind the socket by specifying local_addr?

Moreover, the case seems to be that the sockets aren't bound at least on Windows until data is being sent:

https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-sendto#remarks

That makes sense, you don't expect a response until you've sent something.

Let's move back to TCP land for a second: a server needs to do bind+listen, a client does not need to use bind, the kernel does it implicitly on connect, from connect(3p):

If the socket has not already been bound to a local address, connect() shall bind it to an address which, unless the socket's address family is AF_UNIX, is an unused local address.

I don't remember someone saying the lack of an explicit call to bind on a TCP socket used in a client context being a problem.

Calling bind on the client side is annoying because you need to choose an address that the remote peer can reach, the kernel does that for you by looking at its routing table and selecting the address for the appropriate interface to use per its routing table.

Doing that manually is not practical nor zen imo.

artfwo commented 1 year ago

The echo client (representing pymonome) does not bind the socket by specifying local_addr?

yeah, because in UDP land they're both kind of servers and clients.

That makes sense, you don't expect a response until you've sent something.

Not necessarily, i.e. serialosc will send grid and arc events for keys and encoders to lastly configured ports without a prior request to do so, even after a fresh restart.

It may not be so useful with the current pymonome behavior - setting the port to random every time, but having a placeholder in the code to set the port explicitly does make sense for me.

Calling bind on the client side is annoying because you need to choose an address that the remote peer can reach, the kernel does that for you by looking at its routing table and selecting the address for the appropriate interface to use per its routing table.

Good point, I'm also fine with binding this to 0.0.0.0 or :: or just setting local_addr to None explicitly, so those parts are easier to read and modify later.

lopter commented 1 year ago

Thank you for clarifying how serialosc and pymonome communicate.

None seems like a bad idea because then node will passed as NULL to getaddrinfo and the socket will be bound to the loopback interface because AI_PASSIVE won't be set. (See create_datagram_endpoint, socket.getaddrinfo, getaddrinfo).

So 0.0.0.0/:: make more sense (we are trying to be able to have pymonome and serialosc be on different hosts/containers) and I thought we could just use :: thanks to dual-stack but I get an error when I use :: on the client side on my IPv4 network (still playing with the examples from the docs).

I'll get back to this later.