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

Add local_address= kwarg to open_tcp_stream #1644

Merged
merged 2 commits into from
Jun 25, 2020

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Jun 24, 2020

Fixes gh-275

@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #1644 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1644   +/-   ##
=======================================
  Coverage   99.69%   99.69%           
=======================================
  Files         110      110           
  Lines       13858    13955   +97     
  Branches     1062     1074   +12     
=======================================
+ Hits        13816    13913   +97     
  Misses         27       27           
  Partials       15       15           
Impacted Files Coverage Δ
trio/_highlevel_open_tcp_stream.py 97.33% <100.00%> (+0.36%) ⬆️
trio/socket.py 100.00% <100.00%> (ø)
trio/tests/test_highlevel_open_tcp_stream.py 100.00% <100.00%> (ø)
trio/_core/_thread_cache.py 100.00% <0.00%> (ø)
trio/_core/tests/test_run.py 100.00% <0.00%> (ø)
trio/_core/tests/test_thread_cache.py 100.00% <0.00%> (ø)
trio/_core/_run.py 99.77% <0.00%> (+<0.01%) ⬆️

@oremanj
Copy link
Member

oremanj commented Jun 25, 2020

Thanks for the detailed comment on bind() -- I learned something!

@oremanj oremanj merged commit 270bf85 into python-trio:master Jun 25, 2020
@ntninja
Copy link

ntninja commented Jun 25, 2020

That was way too fast for me…! 😮

Do I see it correctly that with this API limiting a connection to socket.AF_INET/socket.AF_INET6 address families during name resolution is done by passing "0.0.0.0"/"::" respectively for the local_address=… arg?

@njsmith njsmith deleted the bind-address branch June 25, 2020 08:39
@njsmith
Copy link
Member Author

njsmith commented Jun 25, 2020

@ntninja Currently, open_tcp_stream always does full name resolution, but if you pass local_address="0.0.0.0" then it will only skip trying to connect to the AF_INET6 addresses and if you pass local_address="::" then it will skip trying to connect to the AF_INET addresses. Technically this is very slightly inefficient (getaddrinfo still has to look up both sets of addresses), but this is very minor, and in return, we get better error messages: e.g., if you accidentally try to connect to an IPv6-only host with local_address="0.0.0.0", then it will fail, but the error message will tell you that there were IPv6 addresses it considered connecting to and if you changed your local_address= you might succeed.

@ntninja
Copy link

ntninja commented Jun 25, 2020

Ok, thanks! I don't have any strong feelings on this, just wanted to know for sure!

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.

Allow setting source interface in open_tcp_stream
3 participants