socketmodule.c: add sock_call() to fix how the timeout is recomputed

[vstinner]

BPO	23834
Nosy	@pitrou, @vstinner, @serhiy-storchaka
Files	sock_call.patch sendto_ssizet.patch sock_call-2.patch

^{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:

assignee = None
closed_at = <Date 2015-04-02.07:38:36.117>
created_at = <Date 2015-03-31.22:55:06.246>
labels = []
title = 'socketmodule.c: add sock_call() to fix how the timeout is recomputed'
updated_at = <Date 2015-07-27.21:39:32.939>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2015-07-27.21:39:32.939>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2015-04-02.07:38:36.117>
closer = 'vstinner'
components = []
creation = <Date 2015-03-31.22:55:06.246>
creator = 'vstinner'
dependencies = []
files = ['38770', '38771', '38774']
hgrepos = []
issue_num = 23834
keywords = ['patch']
message_count = 14.0
messages = ['239758', '239759', '239760', '239774', '239843', '239849', '239859', '239868', '239883', '239923', '239968', '240183', '240311', '247490']
nosy_count = 5.0
nosy_names = ['pitrou', 'vstinner', 'neologix', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue23834'
versions = ['Python 3.5']

[vstinner]

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.

[vstinner]

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?

[vstinner]

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.

[vstinner]

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().

[python-dev]

New changeset 358a2bcd0d0b by Victor Stinner in branch 'default':
Issue bpo-23834: Add sock_call() helper function
https://hg.python.org/cpython/rev/358a2bcd0d0b

[python-dev]

New changeset b3c7ae99b8e0 by Victor Stinner in branch 'default':
Issue bpo-23834: Modify socket.sendall() to reuse sock_call() with
https://hg.python.org/cpython/rev/b3c7ae99b8e0

[vstinner]

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

[python-dev]

New changeset 920b700d9509 by Victor Stinner in branch 'default':
Issue bpo-23834: Fix sock_call(), set deadline_initialized to recompute the timeout
https://hg.python.org/cpython/rev/920b700d9509

[vstinner]

The changeset 920b700d9509 fixed AMD64 Snow Leop 3.x, I close the issue.

[python-dev]

New changeset 29c32ca46652 by Victor Stinner in branch '2.7':
Issue bpo-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 bpo-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 bpo-23834: Fix socket.sendto(), use the C Py_ssize_t type to
https://hg.python.org/cpython/rev/a064abfd436c

[python-dev]

New changeset 7a9c49885cd3 by Victor Stinner in branch 'default':
Issue bpo-23834: Simplify timeout handling
https://hg.python.org/cpython/rev/7a9c49885cd3

[python-dev]

New changeset 7f54676348d3 by Victor Stinner in branch 'default':
Issue bpo-23834: Fix initial value of the socket timeout
https://hg.python.org/cpython/rev/7f54676348d3

[python-dev]

New changeset 462680f4e8af by Victor Stinner in branch 'default':
Issue bpo-23834: Fix the default socket timeout
https://hg.python.org/cpython/rev/462680f4e8af

[python-dev]

New changeset cd60eccaa331 by Victor Stinner in branch '3.5':
Issue bpo-24732, bpo-23834: Fix sock_accept_impl() on Windows
https://hg.python.org/cpython/rev/cd60eccaa331