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

Fix error messages for host not found when `host` is passed in as bytes #633

merged 4 commits into from Aug 28, 2018


None yet
2 participants

ncollins commented Aug 26, 2018

open_tcp_stream can take host as either bytes or str, but the formatting for the error message when a host is not found depends on host being a string. When trying to connect to a non-existent host, if passed as a string I would get the exception:

OSError: all attempts to connect to localhost:8081 failed

but when passed as bytes I would get the exception:

if ":" in host:

TypeError: a bytes-like object is required, not 'str'

This PR adds a single line to convert bytes to a string before running the rest of the function (and some tests).

ncollins added some commits Aug 26, 2018

fixes error messages when `host` is bytes not str
`open_tcp_stream` can take `host` as either str or bytes, but
`format_host_port` (used to format error messages) only worked when
`host` was a str.

This comment has been minimized.


njsmith commented Aug 26, 2018

Nice catch!

The test failure is #624, nothing to worry about here.

The one thing that jumped out at me is that the interaction of hostnames and text encoding is stupidly complicated. So is a plain .decode() the right thing to do? On Python 2 that meant "use some random system-defined text encoding", which is definitely not right, but it looks like on Python 3 they made it so plain .decode() always uses utf-8, which is less wrong... but still probably wrong, because when a hostname is passed as bytes, we assume that it's already in the standard hostname "A-label" form, which is required to be all-ascii. Also, for hostnames we guarantee that bytes and str representations behave identically if they're all ascii, but for non-ascii characters idna gets involved and who knows what that even does.

So I think the right thing to do here is .decode("ascii"). Can you make that change?

And finally, can you add a newsfragment? Just create a file called newsfragments/633.bugfix.rst, and put a brief description of what was fixed.

ncollins added some commits Aug 27, 2018

adds explict "ascii" parameter when decoding bytes
In trio, str and bytes representations of hostnames are only guaranteed to
behave the same way when they are ASCII encoded.

This comment has been minimized.

codecov bot commented Aug 27, 2018

Codecov Report

Merging #633 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #633      +/-   ##
+ Coverage   99.28%   99.28%   +<.01%     
  Files          91       91              
  Lines       10763    10767       +4     
  Branches      768      768              
+ Hits        10686    10690       +4     
  Misses         58       58              
  Partials       19       19
Impacted Files Coverage Δ
trio/ 96.92% <100%> (+0.04%) ⬆️
trio/tests/ 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b926f5d...befc427. Read the comment docs.


This comment has been minimized.


ncollins commented Aug 27, 2018

Thanks for the help. I've made those changes.


This comment has been minimized.


njsmith commented Aug 28, 2018

Awesome, thanks! Merging now 🎉 🎂 And, no pressure, but if you'd like to keep contributing then we'd love to have you, so I'm also sending you a github invite. You can read more about this in our contributing documentation.

@njsmith njsmith merged commit 1095e37 into python-trio:master Aug 28, 2018

5 checks passed

codecov/patch 100% of diff hit (target 99.28%)
codecov/project 99.28% (+<.01%) compared to b926f5d
continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/jenkins/pr-merge This commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment