classification
Title: socketmodule.c: add sock_call() to fix how the timeout is recomputed
Type: Stage:
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: neologix, pitrou, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-03-31 22:55 by vstinner, last changed 2015-07-27 21:39 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
sock_call.patch vstinner, 2015-03-31 22:57 review
sendto_ssizet.patch vstinner, 2015-03-31 23:02 review
sock_call-2.patch vstinner, 2015-04-01 09:08 review
Messages (14)
msg239758 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-31 22:55
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
   the function: wait until the socket is writable if writing is nonzero, wait
   until the socket received data otherwise.

   If the function is interrupted by a signal (failed with EINTR): retry the
   function, except if the signal handler raised an exception (PEP 475).

   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.
msg239759 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-31 23:02
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?
msg239760 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-31 23:05
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.
msg239774 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-04-01 09:08
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().
msg239843 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-01 20:14
New changeset 358a2bcd0d0b by Victor Stinner in branch 'default':
Issue #23834: Add sock_call() helper function
https://hg.python.org/cpython/rev/358a2bcd0d0b
msg239849 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-01 21:23
New changeset b3c7ae99b8e0 by Victor Stinner in branch 'default':
Issue #23834: Modify socket.sendall() to reuse sock_call() with
https://hg.python.org/cpython/rev/b3c7ae99b8e0
msg239859 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-04-01 22:37
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

======================================================================
ERROR: testSendmsgTimeout (test.test_socket.SendmsgTCPTest)
----------------------------------------------------------------------
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

======================================================================
FAIL: testSendmsgTimeout (test.test_socket.SendmsgTCPTest)
----------------------------------------------------------------------
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
msg239868 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-02 01:23
New changeset 920b700d9509 by Victor Stinner in branch 'default':
Issue #23834: Fix sock_call(), set deadline_initialized to recompute the timeout
https://hg.python.org/cpython/rev/920b700d9509
msg239883 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-04-02 07:38
The changeset 920b700d9509 fixed AMD64 Snow Leop 3.x, I close the issue.
msg239923 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-02 15:22
New changeset 29c32ca46652 by Victor Stinner in branch '2.7':
Issue #23834: Fix socket.sendto(), use the C long type to store the result of
https://hg.python.org/cpython/rev/29c32ca46652

New changeset 436bb7ad6349 by Victor Stinner in branch '3.4':
Issue #23834: Fix socket.sendto(), use the C Py_ssize_t type to store the
https://hg.python.org/cpython/rev/436bb7ad6349

New changeset a064abfd436c by Victor Stinner in branch 'default':
(Merge 3.4) Issue #23834: Fix socket.sendto(), use the C Py_ssize_t type to
https://hg.python.org/cpython/rev/a064abfd436c
msg239968 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-03 11:36
New changeset 7a9c49885cd3 by Victor Stinner in branch 'default':
Issue #23834: Simplify timeout handling
https://hg.python.org/cpython/rev/7a9c49885cd3
msg240183 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-06 21:25
New changeset 7f54676348d3 by Victor Stinner in branch 'default':
Issue #23834: Fix initial value of the socket timeout
https://hg.python.org/cpython/rev/7f54676348d3
msg240311 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-09 08:32
New changeset 462680f4e8af by Victor Stinner in branch 'default':
Issue #23834: Fix the default socket timeout
https://hg.python.org/cpython/rev/462680f4e8af
msg247490 - (view) Author: Roundup Robot (python-dev) Date: 2015-07-27 21:39
New changeset cd60eccaa331 by Victor Stinner in branch '3.5':
Issue #24732, #23834: Fix sock_accept_impl() on Windows
https://hg.python.org/cpython/rev/cd60eccaa331
History
Date User Action Args
2015-07-27 21:39:32python-devsetmessages: + msg247490
2015-04-09 08:32:58python-devsetmessages: + msg240311
2015-04-06 21:25:13python-devsetmessages: + msg240183
2015-04-03 11:37:00python-devsetmessages: + msg239968
2015-04-02 15:22:07python-devsetmessages: + msg239923
2015-04-02 07:38:36vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg239883
2015-04-02 01:23:18python-devsetmessages: + msg239868
2015-04-01 22:37:07vstinnersetmessages: + msg239859
2015-04-01 21:23:24python-devsetmessages: + msg239849
2015-04-01 20:14:51python-devsetnosy: + python-dev
messages: + msg239843
2015-04-01 09:08:23vstinnersetfiles: + sock_call-2.patch

messages: + msg239774
2015-03-31 23:05:58vstinnersetmessages: + msg239760
2015-03-31 23:02:17vstinnersetfiles: + sendto_ssizet.patch

messages: + msg239759
2015-03-31 22:57:30vstinnersetfiles: + sock_call.patch
2015-03-31 22:57:21vstinnersetfiles: - sock_call.patch
2015-03-31 22:55:06vstinnercreate