-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
bpo-32454: socket closefd #5048
Conversation
Modules/socketmodule.c
Outdated
Py_END_ALLOW_THREADS | ||
/* bpo-30319: The peer can already have closed the connection. | ||
Python ignores ECONNRESET on close(). */ | ||
if (res < 0 && errno != ECONNRESET) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be !CHECK_ERRNO(ECONNRESET)
, or use GET_SOCK_ERROR
instead of errno
.
Winsock is an OS library, so it can't use errno
of a particular C runtime library. It uses Windows API [WSA]GetLastError
and [WSA]SetLastError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Doc/library/socket.rst
Outdated
Duplicate a socket file descriptor. This is like :func:`os.dup`, but for | ||
sockets. On some platforms (most noticeable Windows) :func:`os.dup` | ||
does not work for socket file descriptors. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a versionadded
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The socket.dup() function exists since 2.6 at least. It just hasn't been documented yet.
@@ -1519,6 +1519,24 @@ def test_unusable_closed_socketio(self): | |||
self.assertRaises(ValueError, fp.writable) | |||
self.assertRaises(ValueError, fp.seekable) | |||
|
|||
def test_socket_close(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there already a test for dup()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping on this question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is no test. I don't have time to write a test case, Would you like me to drop the documentation for dup?
Lib/test/test_socket.py
Outdated
try: | ||
sock.close() | ||
except OSError: | ||
# sock.close() fails with EBADF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it always does, then how about a assertRaises
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertRaises in the finally block may mask an error in the try block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, Python 3 would display the exception chain.
Add close(fd) function to the socket module Signed-off-by: Christian Heimes <christian@python.org>
I removed the documentation for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Christian!
@tiran: Please replace |
thx @pitrou |
Add close(fd) function to the socket module
Signed-off-by: Christian Heimes christian@python.org
NOTE The PR also documents the previously undocumented
socket.close
function.https://bugs.python.org/issue32454