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

Do not monkey-patch socket, ensure socketpair returns non-blocking sockets #918

Merged
merged 3 commits into from Jan 11, 2018

Conversation

Projects
None yet
2 participants
@lukebakken
Contributor

lukebakken commented Jan 11, 2018

Fixes #916
Fixes #917

@lukebakken lukebakken added this to the 0.12.0 milestone Jan 11, 2018

@codecov

This comment has been minimized.

codecov bot commented Jan 11, 2018

Codecov Report

Merging #918 into master will increase coverage by 0.59%.
The diff coverage is 51.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
+ Coverage   81.52%   82.12%   +0.59%     
==========================================
  Files          21       21              
  Lines        3789     3776      -13     
  Branches      563      560       -3     
==========================================
+ Hits         3089     3101      +12     
+ Misses        545      517      -28     
- Partials      155      158       +3
Impacted Files Coverage Δ
pika/adapters/select_connection.py 79.12% <50%> (-0.12%) ⬇️
pika/compat/__init__.py 84.31% <52%> (+20.59%) ⬆️

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 4433edf...1e2eccb. Read the comment docs.

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Jan 11, 2018

@vitaly-krugl thanks for reviewing our recently merged PRs 😄

@vitaly-krugl

@lukebakken, thanks for picking this up. I added a couple of requests pertaining to the core changes. The auxiliary pika.compat.* attribute accessor changes look good.

finally:
lsock.close()
csock.setblocking(False)
ssock.setblocking(False)

This comment has been minimized.

@vitaly-krugl

vitaly-krugl Jan 11, 2018

Member

My 2 cents: this function's name is exactly like the real deal in socket module which returns blocking sockets; if some other part of pika or pika's user use it, they might expect that it behaves just like the one in socket. For this reason, I would add the logic that makes the sockets non-blocking in the specialized method _get_interrupt_pair augmented with a comment, such as "make sockets non-blocking to avoid potential deadlock".

# We create a connected TCP socket. Note the trick with
# setblocking(False) that prevents us from having to create a thread.
lsock = socket.socket(family, type, proto)
def socketpair(family=socket.AF_INET, type=socket.SOCK_STREAM, proto=0):

This comment has been minimized.

@vitaly-krugl

vitaly-krugl Jan 11, 2018

Member

Add a function comment that explains the reasons for having this method - Windows doesn't provide socket.socketpair.

@lukebakken lukebakken requested a review from vitaly-krugl Jan 11, 2018

@vitaly-krugl

LGTM

# We create a connected TCP socket. Note the trick with
# setblocking(False) that prevents us from having to create a thread.
lsock = socket.socket(family, type, proto)
def _nonblocking_socketpair(family=socket.AF_INET, type=socket.SOCK_STREAM, proto=0):

This comment has been minimized.

@vitaly-krugl
@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Jan 11, 2018

@lukebakken - what's the story with codecov/patch failure, though?

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Jan 11, 2018

@vitaly-krugl - I'm not sure. I think it has to do with test coverage specific to the changes in this PR. I'm not too concerned since everything else passes. What do you think?

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Jan 11, 2018

@lukebakken - Your call. I am the wrong person to ask, as I am a bit pedantic about such things :).

@lukebakken lukebakken merged commit 9cc3738 into master Jan 11, 2018

5 of 6 checks passed

codecov/patch 51.72% of diff hit (target 81.52%)
Details
codecov/project 82.12% (+0.59%) compared to 4433edf
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lukebakken lukebakken deleted the pika-916 branch Jan 11, 2018

gbartl pushed a commit to gbartl/pika that referenced this pull request Jan 27, 2018

Merge pull request pika#918 from pika/pika-916
Do not monkey-patch socket, ensure socketpair returns non-blocking sockets

lukebakken added a commit that referenced this pull request Apr 12, 2018

Merge pull request #918 from pika/pika-916
Do not monkey-patch socket, ensure socketpair returns non-blocking sockets

(cherry picked from commit 9cc3738)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment