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

Support AF_UNIX in server.HTTPServer.bind_addr #80

Merged
merged 3 commits into from Apr 7, 2018

Conversation

Projects
None yet
2 participants
@wrmsr
Contributor

wrmsr commented Apr 6, 2018

This hotfix breaks AF_UNIX binding by leaving only the first two characters of the socket path in bind_addr (as getsockname returns the socket file path in AF_UNIX). I caught this as my code then goes on to chmod bind_addr expecting it to be a correct path after binding.

This is imo a more urgent fix than the TODO of refactoring out the hack as the hack renders valid usecases broken in 6.1.0 without monkeypatching or equivalent.

@webknjaz webknjaz requested review from webknjaz and cherrypy/contributors Apr 6, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 6, 2018

Thanks for pointing this out! It might need a regression test to prevent it from breaking during further refactoring.
I'll try to accept this ASAP once I check that it won't affect anything else additionally, but if you're up to writing that test please let me know.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 6, 2018

@wrmsr do you directly use bind_address in your code? how does it break? any tracebacks? I'd like to document more details.

@codecov

This comment has been minimized.

codecov bot commented Apr 6, 2018

Codecov Report

Merging #80 into master will increase coverage by 0.76%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage    65.5%   66.26%   +0.76%     
==========================================
  Files          15       16       +1     
  Lines        2780     2855      +75     
==========================================
+ Hits         1821     1892      +71     
- Misses        959      963       +4

@webknjaz webknjaz changed the title from AF_UNIX bind_addr fix to Support AF_UNIX in server.HTTPServer.bind_addr Apr 7, 2018

wrmsr and others added some commits Apr 6, 2018

@webknjaz webknjaz force-pushed the wrmsr:wrmsr_unix_bind_addr_fix branch from d0763f6 to 16b5144 Apr 7, 2018

@webknjaz webknjaz merged commit 16b5144 into cherrypy:master Apr 7, 2018

4 of 7 checks passed

codeclimate 1 issue to fix
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
lgtm analysis: Python Running analyses for revisions
Details
WIP ready for review
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
ci/circleci: macos-build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

webknjaz added a commit that referenced this pull request Apr 7, 2018

Support AF_UNIX in server.HTTPServer.bind_addr
PR #80 by @wrmsr

Fixes regression introduced by 81abec0

Socket can be bound to:
* AF_INET  - (host, port) tuple
* AF_INET6 - (host, port, flowinfo, scopeid) tuple
* AF_UNIX  - socket_path_in_fs str or smth starting with null-byte
             for abstract sockets available under GNU/Linux

Ref: https://docs.python.org/3/library/socket.html#socket-families
@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 7, 2018

Thank you @wrmsr!

This code should reach PYPI as soon as this build is complete: https://travis-ci.org/cherrypy/cheroot/builds/363434136

It should become available at https://pypi.org/project/Cheroot/6.1.1/ in about 30 min from now.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 7, 2018

Looks like this PR broke Windows build: https://ci.appveyor.com/project/CherryPy/cheroot/build/291

/cc: @jaraco

@webknjaz

This comment has been minimized.

Member

webknjaz commented Apr 7, 2018

Ref: #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment