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

enhance socketpair code in tpool #167

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@vstinner
Contributor

vstinner commented Nov 19, 2014

tpool.setup() uses its own implementation of socket.socketpair().

The "sock" socket is not explicitly closed, which emits a
ResourceWarning with Python 3 when warnings are enabled (ex: python3
-Wd).

This change uses socket.socketpair() if available (which is now the case
an all platforms since Python 3.5). It also ensures that the "sock"
socket is closed, and that all sockets are closed in case of error.

My code is based on the new socket.socketpair() implementation for
Windows in Python 3.5.

enhance socketpair code in tpool
tpool.setup() uses its own implementation of socket.socketpair().

The "sock" socket is not explicitly closed, which emits a
ResourceWarning with Python 3 when warnings are enabled (ex: python3
-Wd).

This change uses socket.socketpair() if available (which is now the case
an all platforms since Python 3.5). It also ensures that the "sock"
socket is closed, and that all sockets are closed in case of error.

My code is based on the new socket.socketpair() implementation for
Windows in Python 3.5.
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Nov 20, 2014

Contributor

Oh, tpool.setup() doesn't work on Windows. sock.bind(('', 0)) is used, but then sock.getsockname() returns ('0.0.0.0', port) where port is a random port number: the address 0.0.0.0 is not valid.

csock.connect(sock.getsockname()) raises the exception socket.error(WSAEADDRNOTAVAIL, ...) (error 10049).

Contributor

vstinner commented Nov 20, 2014

Oh, tpool.setup() doesn't work on Windows. sock.bind(('', 0)) is used, but then sock.getsockname() returns ('0.0.0.0', port) where port is a random port number: the address 0.0.0.0 is not valid.

csock.connect(sock.getsockname()) raises the exception socket.error(WSAEADDRNOTAVAIL, ...) (error 10049).

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Nov 20, 2014

Contributor

Travis is failing, but failures don't look to be related to my changes.

If you don't want the whole change (use socket.socketpair if available, try/except and try/finally to ensure that sockets are closed in case of errors), the minimum changes are:

  • Replace sock.bind(('', 0)) with sock.bind(('127.0.0.1', 0)) to fix Windows and avoid strange issues
  • Close "sock" socket: add sock.close()
Contributor

vstinner commented Nov 20, 2014

Travis is failing, but failures don't look to be related to my changes.

If you don't want the whole change (use socket.socketpair if available, try/except and try/finally to ensure that sockets are closed in case of errors), the minimum changes are:

  • Replace sock.bind(('', 0)) with sock.bind(('127.0.0.1', 0)) to fix Windows and avoid strange issues
  • Close "sock" socket: add sock.close()
@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Nov 24, 2014

Member

Thank you, I've extracted minimum changes to tpool branch 7b9bf57

Since socketpair() is not available everywhere, it's better to stick to old code, so that eventlet behaves the same way across platforms.

Member

temoto commented Nov 24, 2014

Thank you, I've extracted minimum changes to tpool branch 7b9bf57

Since socketpair() is not available everywhere, it's better to stick to old code, so that eventlet behaves the same way across platforms.

temoto added a commit that referenced this pull request Dec 1, 2014

tpool: Windows compatibility, fix ResourceWarning. Thanks to Victor S…
…tinner

#167
Signal socket bind to 127.0.0.1, '' does not work on Windows
sock.close() to fix ResourceWarning with python3 -Wd
@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Dec 1, 2014

Member

Merged into master.

Member

temoto commented Dec 1, 2014

Merged into master.

@temoto temoto closed this Dec 1, 2014

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Dec 12, 2014

Contributor

Thanks for the fix!

Contributor

vstinner commented Dec 12, 2014

Thanks for the fix!

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