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
socketmodule.c: add sock_call() to fix how the timeout is recomputed #68022
Comments
With the PEP-475, the BEGIN_SELECT_LOOP and END_SELECT_LOOP macros of Modules/socketmodule.c became complex. Moreover, they are misused to handle EINTR. Most functions currently reimplemented their own loop inside the existing "select loop", and so the timeout is not recomputed correctly. Attached patch replaces the two macros with a new sock_call() function which takes a function as a parameter. IMO, the new code (sock_xxx_impl functions) is simpler to read, understand and debug. I hate debugging code (especially in gdb) using complex macros. Copy of sock_call() documentation: /* Call a socket function. If the socket has a timeout, wait until the socket is ready before calling If the function is interrupted by a signal (failed with EINTR): retry the When the function is retried, recompute the timeout using a monotonic clock. Raise an exception and return -1 on error, return 0 on success. */ I was surprised by the number of lines of code. It probably means that we are solving a non trivial problem: calling correctly socket functions with a timeout and retrying when interrupted by a signal. |
By the way, socket.sendto() has a bug: it stores the result of sendto() into a C int. It must store the result into a Py_ssize_t. sock_call.patch already contains a fix for this. Attached sendto_ssizet.patch is the fix for Python 2.7 and 3.4. UDP packets cannot be larger than an ethernet frame which is smaller than 10,000 bytes. That's probabyl why nobody complained before. sendto() is only used for UDP, no? |
I chose to not modify yet socket.sendall(). I prefer to wait until this patch is merged to rework this method. Currently, this method never recomputes the timeout. |
sock_call-2.patch: add an inner looop in sock_call() to only retry func() when func() is interrupted by a signal. Limit also changes: try to only modify ctx around to call to sock_call(), move back some variables to the parent function. Rename the variable storing the result to "result". Add some more comments in sock_call(). |
New changeset 358a2bcd0d0b by Victor Stinner in branch 'default': |
New changeset b3c7ae99b8e0 by Victor Stinner in branch 'default': |
Snow Leopard doesn't like me (or the opposite?), the changeset 358a2bcd0d0b introduced a regression. I'm unable to reproduce, I ran test_socket on Linux (3.18), Mac OS X (Yosemite, Mac OS X 10.10) and FreeBSD (10). I don't see a significant difference in sock_sendmsg() http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/2896/steps/test/logs/stdio ====================================================================== Traceback (most recent call last):
File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_socket.py", line 266, in _tearDown
raise exc
File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_socket.py", line 278, in clientRun
test_func()
File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_socket.py", line 2224, in _testSendmsgTimeout
self.sendmsgToServer([b"a"*512])
File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_socket.py", line 1913, in sendmsgToServer
*(args + self.sendmsg_to_server_defaults[len(args):]))
BrokenPipeError: [Errno 32] Broken pipe ====================================================================== Traceback (most recent call last):
File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_socket.py", line 2217, in testSendmsgTimeout
self.assertTrue(self.misc_event.wait(timeout=self.fail_timeout))
AssertionError: False is not true |
New changeset 920b700d9509 by Victor Stinner in branch 'default': |
The changeset 920b700d9509 fixed AMD64 Snow Leop 3.x, I close the issue. |
New changeset 29c32ca46652 by Victor Stinner in branch '2.7': New changeset 436bb7ad6349 by Victor Stinner in branch '3.4': New changeset a064abfd436c by Victor Stinner in branch 'default': |
New changeset 7a9c49885cd3 by Victor Stinner in branch 'default': |
New changeset 7f54676348d3 by Victor Stinner in branch 'default': |
New changeset 462680f4e8af by Victor Stinner in branch 'default': |
New changeset cd60eccaa331 by Victor Stinner in branch '3.5': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: