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

PEP 475: handle EINTR in the socket module (connect) #67806

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

PEP 475: handle EINTR in the socket module (connect) #67806

vstinner opened this issue Mar 9, 2015 · 31 comments
Labels
extension-modules C modules in the Modules dir

Comments

@vstinner
Copy link
Member

vstinner commented Mar 9, 2015

BPO 23618
Nosy @pitrou, @vstinner, @serhiy-storchaka
Files
  • connect_eintr.patch
  • connect_eintr.py
  • connect_eintr-2.patch
  • connect_test.c
  • connect_eintr-3.patch
  • connect_eintr-4.patch
  • connect_eintr-4.patch
  • test_connect_eintr.py
  • test_connect_eintr2.py
  • test_connect_eintr3.py
  • 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.14:17:55.254>
    created_at = <Date 2015-03-09.09:08:33.169>
    labels = ['extension-modules']
    title = 'PEP 475: handle EINTR in the socket module (connect)'
    updated_at = <Date 2015-04-09.08:32:58.616>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2015-04-09.08:32:58.616>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-04-02.14:17:55.254>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2015-03-09.09:08:33.169>
    creator = 'vstinner'
    dependencies = []
    files = ['38402', '38408', '38409', '38411', '38412', '38766', '38767', '38803', '38805', '38806']
    hgrepos = []
    issue_num = 23618
    keywords = ['patch']
    message_count = 31.0
    messages = ['237609', '237610', '237616', '237619', '237656', '237657', '237666', '237669', '238624', '238720', '239444', '239640', '239690', '239693', '239719', '239741', '239744', '239746', '239748', '239775', '239896', '239899', '239902', '239903', '239904', '239905', '239906', '239911', '239912', '239913', '240312']
    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/issue23618'
    versions = ['Python 3.5']

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 9, 2015

    The PEP-475 has been accepted and is partialy implemented. The socket module is not fully patched to handle EINTR. For example, socket.socket.connect() doesn't handle EINTR yet.

    Attached patch fixes socket.connect() to handle EINTR. It should fix issue bpo-11266 and bpo-20611 in Python 3.5 (Python 2.7 and 3.4 will need to be patched to handle explicitly InterruptedError/OSError(EINTR) in Python).

    By the way, some socket functions handle EINTR but don't recompute the timeout yet. _PyTime_monotonic() should be used.

    @vstinner vstinner added the extension-modules C modules in the Modules dir label Mar 9, 2015
    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 9, 2015

    If EINTR is received during connect, the socket is unusable, that's why i
    didn't implement it.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 9, 2015

    If EINTR is received during connect, the socket is unusable,
    that's why i didn't implement it.

    Can you elaborate?

    socket.connect() should be modified according to the PEP-475:
    https://www.python.org/dev/peps/pep-0475/#modified-functions

    What do you mean by "unusable"? Is it possible to retry connect()? Is it safe to call getsockopt(fd, SOL_SOCKET, SO_ERROR) until it returns EISCONN?

    For high-level functions for socket.create_connection(), how should we handle InterruptedError?

    See also this change in asyncio to handle InterruptedError in asyncio:
    https://hg.python.org/cpython/rev/ad67f66a5f3c
    (I wrote it and I didn't test my change, I didn't know how to test it.)
    Tulip issue: https://code.google.com/p/tulip/issues/detail?id=205

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2015

    About EINTR and connect(), I've found the following insightful page:
    http://www.madore.org/~david/computers/connect-intr.html

    Official POSIX wording is this:

    """If connect() is interrupted by a signal that is caught while blocked waiting to establish a connection, connect() shall fail and set errno to [EINTR], but the connection request shall not be aborted, and the connection shall be established asynchronously."""

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 9, 2015

    connect_eintr.py: script calling socket.connect() in a loop and sending SIGARLM signal every millisecond.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 9, 2015

    Oops, connect_eintr.py noticed me (thanks to my recent change of the issue bpo-23571 !) that connect_eintr.patch is wrong: socket.connect() returned None with an exception sent, send connect() was interrupted by SIGINT (CTRL+c).

    Fixed patch.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 9, 2015

    http://www.madore.org/~david/computers/connect-intr.html

    This article contains a program connect_test.c to test how connect() behaves on EINTR. Since it's in the public domain, I attached a copy.

    The program contains the comment: "All systems function as expected when TEST_TWO is set."

    If TEST_TWO is defined, poll() is used to wait until the socket is writable, and then getsockopt(SO_ERROR) is used to check if the error is now zero, when connect() fails with EINTR.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 9, 2015

    connect_eintr-3.patch: Different patch, don't retry connect() if it returns EINTR, but poll using poll/select. The patch changes also the asyncio module.

    @vstinner
    Copy link
    Member Author

    I tested polling+getsockopt(SO_ERROR) with connect_test.c on Linux, it works:

    haypo@smithers$ gcc -D TEST_TWO=1 connect_test.c -o connect_test
    haypo@smithers$ ./connect_test
    Will try to connect to 127.0.0.1 on port 80
    (connect has been interrupted and now completed successfully)

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 20, 2015

    Could you regenerate your latest patch?
    It doesn't show properly in the review tool.

    Also, what's with the assert?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 28, 2015

    New changeset f841d3bc30ee by Victor Stinner in branch 'default':
    Issue bpo-23618, bpo-22117: refactor socketmodule.c
    https://hg.python.org/cpython/rev/f841d3bc30ee

    @vstinner vstinner changed the title PEP 475: handle EINTR in the socket module PEP 475: handle EINTR in the socket module (connect) Mar 30, 2015
    @vstinner
    Copy link
    Member Author

    test_selectors.patch: Enhance test_selector to test the two kinds of signal handlers: one raises an exception, the other one doesn't. I wait until kqueue & devpoll retry on EINTR to push test_selectors.patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2015

    New changeset e8246baad0f6 by Victor Stinner in branch 'default':
    Issue bpo-23618: Refactor the _socket module
    https://hg.python.org/cpython/rev/e8246baad0f6

    New changeset fa5542660b17 by Victor Stinner in branch 'default':
    Issue bpo-23618: Refactor internal_select() to prepare socket.connect() for EINTR
    https://hg.python.org/cpython/rev/fa5542660b17

    New changeset c027d6468667 by Victor Stinner in branch 'default':
    Issue bpo-23618: internal_connect_select() now waits also for error events
    https://hg.python.org/cpython/rev/c027d6468667

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2015

    New changeset 47b2d1ff9743 by Victor Stinner in branch 'default':
    Issue bpo-23618: Fix internal_connect_select()
    https://hg.python.org/cpython/rev/47b2d1ff9743

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2015

    New changeset daf3d2a717e5 by Victor Stinner in branch 'default':
    Issue bpo-23618: Refactor internal_connect()
    https://hg.python.org/cpython/rev/daf3d2a717e5

    New changeset c59d81b802f8 by Victor Stinner in branch 'default':
    Issue bpo-23618: Refactor internal_connect()
    https://hg.python.org/cpython/rev/c59d81b802f8

    @vstinner
    Copy link
    Member Author

    Le mardi 31 mars 2015, Roundup Robot <report@bugs.python.org> a écrit :

    New changeset c59d81b802f8 by Victor Stinner in branch 'default':
    Issue bpo-23618: Refactor internal_connect()
    https://hg.python.org/cpython/rev/c59d81b802f8

    Oh I forgot to add an #ifdef for socklen_t. I didn't get a warning, so
    socklen_t is probably an int. Getsockopt() uses a int* for the value length.

    I also forgot to drop the comment before SetLastError(), it's no more
    revelant.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2015

    New changeset d9374864d4a9 by Victor Stinner in branch 'default':
    Issue bpo-23618: Cleanup internal_connect() in socketmodule.c
    https://hg.python.org/cpython/rev/d9374864d4a9

    New changeset 4fad2b9fc4e6 by Victor Stinner in branch 'default':
    Issue bpo-23618: Fix EINTR handling in socket.connect()
    https://hg.python.org/cpython/rev/4fad2b9fc4e6

    @vstinner
    Copy link
    Member Author

    connect_eintr-4.patch: Updated patch after all my changes to refactor socket.connect() (which is now almost a rewrite from scratch!).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2015

    New changeset 7ed567ad8b4c by Victor Stinner in branch 'default':
    Issue bpo-23618: Enhance EINTR handling in socket.connect()
    https://hg.python.org/cpython/rev/7ed567ad8b4c

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 1, 2015

    New changeset 8ec4acfdb851 by Victor Stinner in branch 'default':
    Issue bpo-23618: Fix EINTR handling on Windows
    https://hg.python.org/cpython/rev/8ec4acfdb851

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

    test_connnect_eintr.py: program to interrupt socket.connect() with signals. It looks like socket.connect() cannot be interrupted by signals: connect() only fails with WSAEINTR when WSACancelBlockingCall() is called, but WSACancelBlockingCall() "has been removed in compliance with the Windows Sockets 2 specification, revision 2.2.0":

    https://msdn.microsoft.com/en-us/library/windows/desktop/ms741547%28v=vs.85%29.aspx

    "Blocking hooks are generally used to keep a single-threaded GUI application responsive during calls to blocking functions. Instead of using blocking hooks, an applications should use a separate thread (separate from the main GUI thread) for network activity."

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 2, 2015

    New changeset aad52bfc816f by Victor Stinner in branch 'default':
    Issue bpo-23618: Don't declare recvmsg/sendmsg helper functions on Windows
    https://hg.python.org/cpython/rev/aad52bfc816f

    New changeset f22188acc77d by Victor Stinner in branch 'default':
    Issue bpo-23618: Document EINTR changes in socket documentation
    https://hg.python.org/cpython/rev/f22188acc77d

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

    Hum, connect() does not always block with test_connect_eintr.py, and this program sometimes fail with ConnectionResetError on connect() on FreeBSD.

    New program which works on Linux and FreeBSD. It now ensures that connect() will block.

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

    test_connect_eintr3.py: even better:

    • block signals in the server thread
    • count signals during connect()
    • display progress: "*" for signal received during connect(), "_" for signal received before/after connect(), "[" and "]" for the beginning and end of a connection, "#" for client connection reset

    Example of output on FreeBSD:

    Register SIGINT
    Register SIGALRM
    Register SIGWINCH
    Register SIGTERM
    Register SIGCHLD
    Send SIGALRM every 200.0 ms
    Run func() during 5.0 seconds
    Type CTRL+c, resize the window, etc.

    ___[][][#][#][#][#]____[#][**]____[#][#][#][#][*#][#][#][*#][#][#][#][#][#][#][#][#][#]_____[#][#][#][#][#][#][#][#][*#][#]___[#]

    Test completed in 5.1 sec
    func() has been called 36 times
    Got 204 signals
    Got 7 signals during connect()

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

    Example of test_connect_eintr3.py output on Linux (3.18):

    Register SIGINT
    Register SIGALRM
    Register SIGWINCH
    Register SIGTERM
    Register SIGCHLD
    Send SIGALRM every 200.0 ms
    Run func() during 5.0 seconds
    Type CTRL+c, resize the window, etc.

    [][][][][][][][**********][][]___[][]

    Test completed in 5.4 sec
    func() has been called 12 times
    Got 85 signals
    Got 50 signals during connect()

    Example of test_connect_eintr3.py output on Mac OS X Yosemite (10.10):

    Register SIGINT
    Register SIGALRM
    Register SIGWINCH
    Register SIGTERM
    Register SIGCHLD
    Send SIGALRM every 200.0 ms
    Run func() during 5.0 seconds
    Type CTRL+c, resize the window, etc.

    []_____[][**][**********][][***]

    Test completed in 5.3 sec
    func() has been called 6 times
    Got 55 signals
    Got 49 signals during connect()

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

    Example of test_connect_eintr3.py output on OpenIndiana:

    Register SIGINT
    Register SIGALRM
    Register SIGWINCH
    Register SIGTERM
    Register SIGCHLD
    Send SIGALRM every 200.0 ms
    Run func() during 5.0 seconds
    Type CTRL+c, resize the window, etc.

    [][][******][][*****][*****]____[]

    Test completed in 5.2 sec
    func() has been called 7 times
    Got 56 signals
    Got 16 signals during connect()

    Oh, and obviously, test_connect_eintr3.py fails with InterruptedError without the patch. Example on Linux:

    Register SIGINT
    Register SIGALRM
    Register SIGWINCH
    Register SIGTERM
    Register SIGCHLD
    Send SIGALRM every 200.0 ms
    Run func() during 5.0 seconds
    Type CTRL+c, resize the window, etc.

    []____[]_____[]_____[]____[******Traceback (most recent call last):
      File "test_connect_eintr.py", line 97, in <module>
        func()
      File "test_connect_eintr.py", line 51, in func
        client.connect(server_addr)
    InterruptedError: [Errno 4] Interrupted system call

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 2, 2015

    New changeset 75fcc7a3738a by Victor Stinner in branch 'default':
    Issue bpo-23618: socket.socket.connect() now waits until the connection completes
    https://hg.python.org/cpython/rev/75fcc7a3738a

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 2, 2015

    http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/2904/steps/test/logs/stdio

    ======================================================================
    FAIL: test_connect_ex_error (test.test_ssl.NetworkedTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ssl.py", line 1441, in test_connect_ex_error
        self.assertIn(rc, (errno.ECONNREFUSED, errno.EWOULDBLOCK))
    AssertionError: 36 not found in (61, 35)

    where 36 is errno.EINPROGRESS

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 2, 2015

    New changeset 44adbb5eeb4b by Victor Stinner in branch 'default':
    Issue bpo-23618: Fix sock_connect_impl(), set the socket error code
    https://hg.python.org/cpython/rev/44adbb5eeb4b

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 2, 2015

    New changeset 09a4d5cc6afd by Victor Stinner in branch 'default':
    Issue bpo-23618: Ooops, remove abort() added for debug purpose
    https://hg.python.org/cpython/rev/09a4d5cc6afd

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

    python-dev mannequin commented Apr 9, 2015

    New changeset bff88c866886 by Victor Stinner in branch 'default':
    Issue bpo-23618: Fix internal_select() for negative timeout (blocking socket) when
    https://hg.python.org/cpython/rev/bff88c866886

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants