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

MNT: Bind to PORT 0 #543

Closed
wants to merge 6 commits into from
Closed

Conversation

ke-zhang-rd
Copy link
Contributor

Goal
This PR is trying to solve #540 and #514 related socket error only reported on windows.

Solution
It looks like Linux and Mac will assign ('0.0.0.0', 0) to socket, if we didn't explicit bind.

bind argument in this PR is based on

Test
This PR solved scenario in those two issues(#540, #514) but may need further tests.

@danielballan
Copy link
Collaborator

The concern mentioned in the article is that if you bind to the port, close the socket and then try binding to the same port again (perhaps in a separate “program”) the port may not still be available. Does that apply here?

@danielballan
Copy link
Collaborator

Actually I think there is a deeper problem here. We use this socket to broadcast to multiple interfaces

self.udp_sock.sendto(bytes_to_send, (host, specified_port))

so there is no single “our address”. I have an unfinished branch I worked on at ICALEPCS that might be helpful. Will push when I’m back to a laptop.

@ke-zhang-rd ke-zhang-rd closed this Nov 8, 2019
@ke-zhang-rd ke-zhang-rd reopened this Nov 8, 2019
@ke-zhang-rd
Copy link
Contributor Author

The concern mentioned in the article is that if you bind to the port, close the socket and then try binding to the same port again (perhaps in a separate “program”) the port may not still be available. Does that apply here?

I don't think so and want to double check with you.

@danielballan
Copy link
Collaborator

Gotcha. I don't think so either but wanted to double-check with you! :-D

@danielballan
Copy link
Collaborator

I think #544 and especially the change from our_address and their_address to our_addresses and their_addresses at the top of this commit ff8a91a will help.

@danielballan
Copy link
Collaborator

I don't think it fixes the issue, but it's the right framework in which to fix the issue. Can you test and see if the bug is still there, to be sure?

@ke-zhang-rd
Copy link
Contributor Author

I was trying to say bind socket with ''(empty string) might be right based figure I found here
https://realpython.com/python-sockets/#using-hostnames. Then we use getsockname to access address when we need it and we don't need to init our_address in __init__
image

I might also totally wrong.
I will check #544

@@ -670,7 +672,7 @@ def command_loop(self):
name, cid, *accepted_address, *new_address,
extra={'pv': name,
'their_address': accepted_address,
'our_address': self.udp_sock.getsockname()[:2]})
'our_address': self.broadcaster.client_address})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work until #544 is merged, right? I don't think client_address exists yet.

$ git grep client_address
caproto/_commands.py:    .. attribute:: client_address
caproto/_commands.py:    def __init__(self, client_address='0.0.0.0'):
caproto/_commands.py:            ipv4_to_int32(str(client_address)))
caproto/_commands.py:    def client_address(self):
caproto/tests/test_serialization.py:    'client_address': [ip],
doc/source/command-line-client.rst:    [D 20:00:47.564 _broadcaster:74] 1 of 1 RepeaterRegisterRequest(client_address='0.0.0.0')
doc/source/shark.rst:   1550679067.619182 192.168.86.21:55928->255.255.255.255:5065 RepeaterRegisterRequest(client_address='0.0.0.0')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean using our_address here. #544 isn't required.

danielballan added a commit to danielballan/caproto that referenced this pull request Jan 18, 2020
This fixes the critical Windows bug introduces in v0.4.0.

Closes caproto#540
Closes caproto#514

This takes the approach first proposed by @ke-zhang-rd
in caproto#543 commit a3fb3d5
and applies it uniformly to all clients. Also, we bind
immediately after creating the socket.
@danielballan
Copy link
Collaborator

Revisiting this, I think your first approach was the right one. Refining that a bit, if we bind right after we create the socket we can avoid getting tangled up in the our_addresses vs client_address code that follows. We can be sure that all calls to getsockname() from then on will work. See #551.

@klauer
Copy link
Member

klauer commented Apr 10, 2020

Is there still work to do here now that #551 is in?

@danielballan
Copy link
Collaborator

I think not. Please reopen if I am mistaken, @ke-zhang-rd.

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.

None yet

3 participants