Skip to content
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

Closed
vstinner opened this issue Mar 31, 2015 · 14 comments
Closed

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

vstinner opened this issue Mar 31, 2015 · 14 comments

Comments

@vstinner
Copy link
Member

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
    Copy link
    Member Author

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

    vstinner commented Apr 1, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 1, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 1, 2015

    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
    Copy link
    Member Author

    vstinner commented Apr 1, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 2, 2015

    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
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

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

    @vstinner vstinner closed this as completed Apr 2, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 2, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 3, 2015

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 6, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 9, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jul 27, 2015

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant