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

add SOCK_NONBLOCK and SOCK_CLOEXEC to socket module #51772

Closed
lekma mannequin opened this issue Dec 16, 2009 · 25 comments
Closed

add SOCK_NONBLOCK and SOCK_CLOEXEC to socket module #51772

lekma mannequin opened this issue Dec 16, 2009 · 25 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@lekma
Copy link
Mannequin

lekma mannequin commented Dec 16, 2009

BPO 7523
Nosy @pitrou, @giampaolo, @bitdancer, @lekma
Files
  • Issue7523.diff
  • Issue7523_2.diff
  • Issue7523_3.diff
  • issue7523_py3k.diff: Ported to py3k patch
  • issue7523_py3k_accept4.diff: Added accept4 via ifdef
  • issue7523_py3k_accept4_1.diff: new test and proper accept4 check
  • issue7523_py3k_accept4_2.diff
  • 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 2010-10-14.15:22:38.311>
    created_at = <Date 2009-12-16.11:00:36.889>
    labels = ['type-feature', 'library']
    title = 'add SOCK_NONBLOCK and SOCK_CLOEXEC to socket module'
    updated_at = <Date 2011-10-27.16:45:04.317>
    user = 'https://github.com/lekma'

    bugs.python.org fields:

    activity = <Date 2011-10-27.16:45:04.317>
    actor = 'nvetoshkin'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-10-14.15:22:38.311>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2009-12-16.11:00:36.889>
    creator = 'lekma'
    dependencies = []
    files = ['15573', '15578', '15621', '19200', '19206', '19223', '19226']
    hgrepos = []
    issue_num = 7523
    keywords = ['patch']
    message_count = 25.0
    messages = ['96482', '96484', '96504', '96513', '96551', '96559', '96598', '96609', '96666', '97636', '97641', '97699', '111182', '111183', '118457', '118463', '118471', '118473', '118569', '118581', '118583', '118584', '118668', '146502', '146503']
    nosy_count = 7.0
    nosy_names = ['exarkun', 'pitrou', 'giampaolo.rodola', 'r.david.murray', 'lekma', 'nvetoshkin', 'BreamoreBoy']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue7523'
    versions = ['Python 3.2']

    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Dec 16, 2009

    It would be nice to have those.
    See http://udrepper.livejournal.com/20407.html for background.

    @lekma lekma mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 16, 2009
    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Dec 16, 2009

    First attempt, against trunk.

    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Dec 17, 2009

    better patch (mainly test refactoring).
    still against trunk.

    should I provide one against py3k?

    @lekma lekma mannequin added the topic-IO label Dec 17, 2009
    @pitrou
    Copy link
    Member

    pitrou commented Dec 17, 2009

    A couple of things:

    • the test shouldn't be marked as Linux-specific since perhaps these
      constants will be supported by other systems some day
    • when importing fcntl, bear in mind that some systems don't have this
      module (e.g. Windows), so you should catch and silence the ImportError

    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Dec 18, 2009

    • when importing fcntl, bear in mind that some systems don't have this
      module (e.g. Windows), so you should catch and silence the ImportError

    would something like the following be ok?:

    try:
    import fcntl
    except ImportError:
    fcntl = False

    and then:

    if hasattr(socket, "SOCK_CLOEXEC") and fcntl:
        tests.append(CloexecLinuxConstantTest)

    @bitdancer
    Copy link
    Member

    It would be better to use test skipping: (eg: @unittest.SkipUnless
    before the test class).

    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Dec 19, 2009

    It would be better to use test skipping: (eg: @unittest.SkipUnless
    before the test class).

    I didn't know about this feature, thanks for the tip.

    Now I wonder if it would be better to do it this way:
    @unittest.SkipUnless(hasattr(socket, "SOCK_CLOEXEC") and fcntl,
    "SOCK_CLOEXEC not defined OR module fcntl not available")

    or this way:
    @unittest.SkipUnless(hasattr(socket, "SOCK_CLOEXEC"), "SOCK_CLOEXEC not
    defined")
    @unittest.SkipUnless(fcntl, "module fcntl not available")

    the second option seems better to me (obvious reason why the test was
    skipped), what do you guys think? (it doesn't really matter, I know, but
    while we're here...)

    @bitdancer
    Copy link
    Member

    I lean toward the second way, myself.

    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Dec 20, 2009

    this one addresses Antoines's comments (with the help of R. David Murray).

    @pitrou
    Copy link
    Member

    pitrou commented Jan 12, 2010

    The patch is basically fine. I'll add a "try .. finally" to the tests.
    lekma, do you have a real name that we should add to the ACKS file?

    @pitrou
    Copy link
    Member

    pitrou commented Jan 12, 2010

    Looking at it again, there's the question of accept() behaviour. In the original code, it will call internal_setblocking() on the new fd if the listening socket is non-blocking. In the new code, if SOCK_NONBLOCK is defined it will not call any OS function to set the new fd in non-blocking mode.

    Here is what the man page has to say:

       On Linux, the new socket returned by accept() does not inherit file status  flags  such  as
       O_NONBLOCK and O_ASYNC from the listening socket.  This behavior differs from the canonical
       BSD sockets implementation.  Portable programs should not rely on inheritance or non-inher‐
       itance  of  file  status  flags  and always explicitly set all required flags on the socket
       returned from accept().
    

    Linux has defined accept4() precisely for this purpose:

       If flags is 0, then accept4() is the same as accept().  The following values can be bitwise
       ORed in flags to obtain different behavior:
    
       SOCK_NONBLOCK   Set the O_NONBLOCK file status flag  on  the  new  open  file  description.
                       Using this flag saves extra calls to fcntl(2) to achieve the same result.
    
       SOCK_CLOEXEC    Set  the  close-on-exec  (FD_CLOEXEC) flag on the new file descriptor.  See
                       the description of the O_CLOEXEC flag in open(2) for reasons why  this  may
                       be useful.
    

    @lekma
    Copy link
    Mannequin Author

    lekma mannequin commented Jan 13, 2010

    lekma, do you have a real name that we should add to the ACKS file?
    no, lekma is fine (that's a nick I carry since childhood)

    Looking at it again, there's the question of accept() behaviour. [...]
    Sorry for not having seen that earlier and thanks for pointing it out to me.
    Unfortunately, I don't think there is a portable solution. We could restrict the patch to linux 2.6.28 and above (there is already an awful lot of ifdefs in this file).
    If you think that's the way to go, I can rework the patch, otherwise I think it's better to close the issue (maybe reopen when bsds catch up, if ever).
    Ideas?

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 22, 2010

    @antoine could you respond to msg97699, thanks.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 22, 2010

    @antoine could you respond to msg97699, thanks.

    Well my comments and the patch itself are outdated now that 2.7 is in bugfix mode. The socket implementation in 3.2 is slightly different, which means the patch should be updated (it isn't necessarily difficult to do so), and the accept() issue should be examined again.

    @pitrou pitrou removed the topic-IO label Jul 22, 2010
    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 12, 2010

    Made an attempt to port lekma's patch to py3k-trunk. No (logical) changes needed.

    Don't know about accept4() issue. As I saw in Qt sources, they ifdef'ed CLOEXEC by default on file descriptors. Don't think it's acceptable :) in this particular case. So, @antoine, what do you say?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 12, 2010

    The accept() issue is the following: the socket created by accept() (in Lib/socket.py) will formally inherit its parent's type attribute (including any SOCK_NONBLOCK and SOCK_CLOEXEC flags).

    However, the underlying Linux socket is created by the accept() system call, which doesn't inherit flags as mentioned in the aforementioned man page. Therefore, the Python socket gives the wrong information about the socket's real flags.

    This can be witnessed quickly:

    >>> s = socket.socket(socket.AF_INET, socket.SOCK_STREAM | socket.SOCK_CLOEXEC)
    >>> s.bind(("", 0))
    >>> s.getsockname()
    ('0.0.0.0', 34634)
    >>> s.listen(5)
    >>> c, a = s.accept()
       # Here, just start a "telnet" or "nc" session from another term
    >>> import fcntl
    >>> fcntl.fcntl(s, fcntl.F_GETFD)
    1
    >>> fcntl.fcntl(c, fcntl.F_GETFD)
    0
    >>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
    0
    >>> c.type & socket.SOCK_CLOEXEC
    524288

    The quick solution would be to mask out these flags when creating the Python socket in accept(). A better solution might be to inherit these flags by using the accept4() system call when possible (this is useful especially for SOCK_CLOEXEC, of course).

    Apart from that, the patch looks ok, but it would be nice to test that at least the underlying socket is really in non-blocking mode, like is done in NonBlockingTCPTests.testSetBlocking.

    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 12, 2010

    Thanks! I can see the problem now, but I think checking should be done like this:
    >>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
    0
    >>> fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
    1
    and with accept4() call I've got flag set:
    >>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
    1
    >>> fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
    1

    Don't know how to properly check if accept4 is available.

    Second attempt - dropping flags from sock_type should be done on Python level in socket.py and I don't quite like idea to check if SOCK_CLOEXEC is in locals every time.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 12, 2010

    You can check accept4() presence by adding it to the AC_CHECK_FUNCS macro in configure.in around line 2553, and add the corresponding HAVE_ACCEPT4 lines in pyconfig.h.in. Then run "autoconf" and you will have access to a HAVE_ACCEPT4 constant when accept4() exists.

    By the way, you shouldn't use C++-style comments ("// ..."), because some C compilers refuse them.

    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 13, 2010

    Another patch with:

    • testInitBlocking method
    • no c++ style comments
    • a newly generated configure script (almost 1.5k lines diff)
    • proper accept4 availability check
    With this patch I've got 
    Traceback (most recent call last):
      File "/home/nekto/workspace/py3k/Lib/test/test_socket.py", line 1564, in testInterruptedTimeout
        foo = self.serv.accept()
    socket.timeout: timed out

    Can someone test if there's a real regression?

    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 13, 2010

    Here's what strace on FAIL shows (print "in alarm_handler" added)

    alarm(2) = 0
    poll([{fd=3, events=POLLIN}], 1, 5000) = ? ERESTART_RESTARTBLOCK (To be restarted)
    --- SIGALRM (Alarm clock) @ 0 (0) ---
    rt_sigreturn(0xffffffff) = -1 EINTR (Interrupted system call)
    accept4(3, 0x7fffa94c4780, [16], 0) = -1 EAGAIN (Resource temporarily unavailable)
    poll([{fd=3, events=POLLIN}], 1, 2999) = 0 (Timeout)
    accept4(3, 0x7fffa94c4780, [16], 0) = -1 EAGAIN (Resource temporarily unavailable)
    write(1, "in alarm_handler\n", 17) = 17
    alarm(0) = 0

    Here's on OK:

    alarm(2) = 0
    poll([{fd=3, events=POLLIN}], 1, 5000) = ? ERESTART_RESTARTBLOCK (To be restarted)
    --- SIGALRM (Alarm clock) @ 0 (0) ---
    rt_sigreturn(0xffffffff) = -1 EINTR (Interrupted system call)
    write(1, "in alarm_handler\n", 17) = 17
    alarm(0)

    For some reason does another trip through BEGIN_SELECT_LOOP() macro

    @pitrou
    Copy link
    Member

    pitrou commented Oct 13, 2010

    For some reason does another trip through BEGIN_SELECT_LOOP() macro

    Indeed:

     if (!timeout)
    

    +#ifdef HAVE_ACCEPT4
    + /* inherit socket flags and use accept4 call */
    + flags = s->sock_type & (SOCK_CLOEXEC | SOCK_NONBLOCK);
    + newfd = accept4(s->sock_fd, SAS2SA(&addrbuf), &addrlen, flags);
    +#else

    There's a missing curly brace after "if (!timeout)", so accept4() is always called.

    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 13, 2010

    @antoine already found that myself, patched and tested :) thanks!

    @pitrou
    Copy link
    Member

    pitrou commented Oct 14, 2010

    Patch committed in r85480, thanks!

    @pitrou pitrou closed this as completed Oct 14, 2010
    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 27, 2011

    Started implementing accept4() socket method and stuck on socket object's timeout attribute. What value should we assign to sock->sock_timeout if SOCK_NONBLOCK was specified in accept4() call? And in socket.py should we check as in original accept:
    if getdefaulttimeout() is None and self.gettimeout():
    sock.setblocking(True)

    @nvetoshkin
    Copy link
    Mannequin

    nvetoshkin mannequin commented Oct 27, 2011

    Sorry, wrong ticket. Right one is 10115

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants