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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lopter
Copy link

@lopter lopter commented Mar 5, 2022

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
Copy link
Owner

artfwo commented Jul 4, 2023

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!

This allowed me to run serialosc and pymonome under different docker
containers.
@lopter
Copy link
Author

lopter commented Jul 7, 2023

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
Copy link
Owner

artfwo commented Jul 7, 2023

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
Copy link
Author

lopter commented Jul 8, 2023

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
Copy link
Owner

artfwo commented Jul 8, 2023

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
Copy link
Author

lopter commented Jul 8, 2023

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
Copy link
Owner

artfwo commented Jul 8, 2023

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
Copy link
Author

lopter commented Jul 8, 2023

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
Copy link
Owner

artfwo commented Jul 8, 2023

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
Copy link
Author

lopter commented Jul 9, 2023

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.

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

Successfully merging this pull request may close these issues.

2 participants