python / cpython

The Python programming language
https://www.python.org/
Other
60.01k stars 29.05k forks source link

On Mac / BSD sockets returned by accept inherit the parent's FD flags #52243

Closed 292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 closed 13 years ago

292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 commented 14 years ago
BPO 7995
Nosy @loewis, @ronaldoussoren, @pitrou, @kristjanvalur, @giampaolo, @ned-deily, @justincappos
Files
  • python-nonportable.py: an example program that demonstrates the issue. Test script that will fail on Mac / BSD but work on Windows / Linux.
  • 7995_v1.patch: Patch + test for issue
  • 7995_v2.patch: Updated patch
  • 7995_v3.patch: Updated patch to opt-out
  • nonblock.patch
  • nonblock2.patch
  • nonblock3.patch
  • bug.py: bug demonstration with cpython head revision
  • defined.patch: suggesteate to whatever
  • 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: ```python assignee = 'https://github.com/ronaldoussoren' closed_at = created_at = labels = ['OS-mac', 'type-bug', 'library'] title = "On Mac / BSD sockets returned by accept inherit the parent's FD flags" updated_at = user = 'https://github.com/justincappos' ``` bugs.python.org fields: ```python activity = actor = 'kristjan.jonsson' assignee = 'ronaldoussoren' closed = True closed_date = closer = 'pitrou' components = ['Library (Lib)', 'macOS'] creation = creator = 'Justin.Cappos' dependencies = [] files = ['16325', '20201', '20206', '20222', '20226', '20257', '20278', '21478', '21490'] hgrepos = ['14'] issue_num = 7995 keywords = ['patch'] message_count = 52.0 messages = ['99852', '100570', '121886', '121924', '121925', '121926', '121962', '121968', '121972', '121974', '121975', '121978', '121979', '121981', '121983', '121984', '121986', '121998', '122013', '122039', '122041', '122042', '122055', '122096', '122097', '122133', '124954', '124958', '124965', '124967', '125064', '125066', '125078', '125135', '125218', '125276', '125331', '125372', '125373', '125375', '125379', '125437', '125453', '125471', '132591', '132592', '132593', '132594', '132598', '132601', '132602', '132650'] nosy_count = 13.0 nosy_names = ['loewis', 'ronaldoussoren', 'exarkun', 'roysmith', 'pitrou', 'kristjan.jonsson', 'giampaolo.rodola', 'ned.deily', 'stutzbach', 'nicdumz', 'bbangert', 'Justin.Cappos', 'rosslagerwall'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue7995' versions = ['Python 3.2'] ```

    292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 commented 14 years ago

    Suppose there is a program that has a listening socket that calls accept to obtain new sockets for client connections. socketmodule.c assumes that these client sockets have timeouts / blocking in the default state for new sockets (which on most systems means the sockets will block). However, socketmodule.c does not verify the state of the socket object that is returned by the system call accept.

    From http://linux.die.net/man/2/accept : 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 behaviour differs from the canonical BSD sockets implementation. Portable programs should not rely on inheritance or non-inheritance of file status flags and always explicitly set all required flags on the socket returned from accept().

    socketmodule.c does not explicitly set or check these flags for sockets returned by accept. The attached program will print the following on Linux regardless of whether the settimeout line for s exists or not:

    a has timeout: None O_NONBLOCK is set: False received: hi

    On Mac / BSD, the program will produce the following output when the timeout is set on the listening socket:

    a has timeout: None
    O_NONBLOCK is set: True
    Traceback (most recent call last):
      File "python-nonportable.py", line 39, in <module>
        message = a.recv(1024)
    socket.error: (35, 'Resource temporarily unavailable')

    When the timeout is removed, the behavior is the same as linux:

    a has timeout: None O_NONBLOCK is set: False received: hi

    Note that the file descriptor problem crops up in odd ways on Mac systems. It's possible that bpo-5154 may be due to this bug. I am aware of other problems with the socketmodule on Mac and will report them in other tickets.

    I believe that this problem can be easily mitigated by always calling fcntl to unset the O_NONBLOCK flag after accept (O_ASYNC should be unset too, for correctness). I would recommend adding the below code snippet at line 1653 in socketmodule.c (r78335). The resulting code would look something like this (with '+' in front of the added lines):

    '''
    #ifdef MS_WINDOWS
            if (newfd == INVALID_SOCKET)
    #else
            if (newfd < 0)
    #endif
                    return s->errorhandler();

    +#if defined(APPLE) || defined(OpenBSD) || defined(FreeBSD) + int starting_flag; + // Unset O_NONBLOCK an O_ASYNC if they are inherited. + starting_flag = fcntl(newfd, F_GETFL, 0); + starting_flag &= \~(O_NONBLOCK | O_ASYNC); + fcntl(newfd, F_SETFL, starting_flag); +#endif

        /* Create the new object with unspecified family,
           to avoid calls to bind() etc. on it. \*/
        sock = (PyObject \*) new_sockobject(newfd,
                                           s-\>sock_family,
                                           s-\>sock_type,
                                           s-\>sock_proto);

    '''

    I've tested this patch on my Mac and Linux systems and it seems to work fine. I haven't had a chance to test on BSD. Also, I did not test for this problem in Python 3, but I assume it exists there as well and the same fix should be applied.

    ronaldoussoren commented 14 years ago

    I've removed 2.5 and added 3.2 because there won't be further bugfix releases of 2.5 and the issue also affects 3.2.

    IMHO changing this behavior is not a bugfix and is therefore out of scope for 2.6.x, in particular because this change might break code that runs fine on OSX and assumes the current behavior.

    The documntation is not entirely clear on the behavior of the accept method, the best I could find is the documentation of the setblocking method: that says that all sockets are blocking by default, that would mean that the result of accept should be a blocking socket.

    I'm therefore +1 for this change. I'm setting the 'needs review' flag because I'd like some input from other developers as well.

    ned-deily commented 13 years ago

    (See also bpo-7322)

    013be167-c92f-46a8-afa1-7818ebd81a32 commented 13 years ago

    The answer depends on what the socket module is trying to do. Is the goal simply to provide a pythonic thin wrapper over the underlying OS interfaces without altering their semantics, or to provide a completely homogeneous abstraction? Having attempted the latter before, I'm aware of just how difficult a job it can be.

    The docs have a big bold note right up top, "Note Some behavior may be platform dependent, since calls are made to the operating system socket APIs". This is followed up by, "The platform-specific reference material for the various socket-related system calls are also a valuable source of information on the details of socket semantics."

    What's not clear, however, is the intent. If the intent is to hide the platform differences, then the notes in the docs are simply warnings that we're not always successful at doing that. If so, then exposing the different behaviors of listen/accept is a bug which should be fixed.

    Anyway, my personal opinion is that we should consider the current behavior a bug and fix it. I like the idea of setting all accepted sockets to blocking mode (and documenting it clearly). I think it is what most people would expect. I understand that this would break code of people who were relying on the "accept inherits non-blocking mode" behavior on some OS's, but I suspect the number of people who are relying on that behavior is extremely close to zero, and they are relying on a non-portable feature of a specific OS.

    I leave it to others to figure out which versions it is reasonable to apply this to.

    pitrou commented 13 years ago

    It seems to me that it's a reasonable request. There is indeed a bug, since Python uses non-blocking sockets to implement its "timeout" feature, and flags inheritance means Python's view of whether the socket is non-blocking is not in sync with reality on the OS side.

    I think the patch should be opt-out rather than opt-in: that code path should probably be enabled for everything but Linux (and Windows?).

    Another possibility is to force inheritance of flags, such that an accept()ed socket inherits the timeout settings of its parent (even under Linux).

    pitrou commented 13 years ago

    Oh, and a test should be added of course.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    The answer depends on what the socket module is trying to do. Is the goal simply to provide a pythonic thin wrapper over the underlying OS interfaces without altering their semantics, or to provide a completely homogeneous abstraction?

    Most definitely the latter. The method called accept must absolutely always call the same-named underlying system call, and have no other side effects, unless a) the system doesn't provide such a system call, or b) something else is called, but this behaves *as if* the original system call was called, or c) deviating behavior was explicitly requested from the application.

    The docs have a big bold note right up top, "Note Some behavior may be platform dependent, since calls are made to the operating system socket APIs".

    Having such a note is fine with me; it shouldn't be big and bold, though. There is a strong opposition to big and bold notes; they should be only used for really serious problems (such as the likely risk of data loss).

    Anyway, my personal opinion is that we should consider the current behavior a bug and fix it. I like the idea of setting all accepted sockets to blocking mode (and documenting it clearly).

    -1.

    I think it is what most people would expect.

    Apparently, the designers of BSD thought differently. Remember that it is them who defined the socket API in the first place, so they have the right that their design decisions are considered.

    We should also take bpo-10115 into account, which proposes changes to accept on Linux. One solution might to add optional flags to accept(), asking for certain behavior variations.

    292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 commented 13 years ago

    Apparently, the designers of BSD thought differently. Remember that it is them who defined the socket API in the first place, so they have the right that their design decisions are considered.

    I think there is a bit of confusion here. The 'bug' isn't with different socket semantics on different OSes. The bug is that the programmer who wrote the wrapper for sockets on Python assumed the OS semantics weren't the BSD style.

    Here is the issue (put plainly):

    Python sockets support a notion of timeout (note this notion is not reflected in the OS socket API).

    The python socket implementation of timeouts uses the underlying OS / socket API to provide this by setting the socket to nonblocking and setting a timeout value in a Python object that holds socket info.

    This implementation assumes that the OS sets any socket it receives via accept to nonblocking. (this is a false assumption on BSD)

    The end result is that the OS has a nonblocking socket and the Python object thinks it is blocking. This is why the socket object in Python has timeout=None yet calling fcntl shows the socket is nonblocking.

    Calling code paths that handle timeouts and expect the socket to block causes bugs like I described in my code. This behavior is clearly wrong under any interpretation!

    You can debate whether the right patch is to use what I proposed or instead change new Python sockets to inherit the timeout / blocking setting on BSD. However, what is implemented now is clearly broken.

    pitrou commented 13 years ago

    > Anyway, my personal opinion is that we should consider the current > behavior a bug and fix it. I like the idea of setting all accepted > sockets to blocking mode (and documenting it clearly).

    -1.

    > I think it is what most people would expect.

    Apparently, the designers of BSD thought differently. Remember that it is them who defined the socket API in the first place, so they have the right that their design decisions are considered.

    We are talking about the timeout feature, which is a Python feature, not a BSD (or Linux) sockets feature. It should work properly, even if that means adding some boilerplate around system calls.

    We should also take bpo-10115 into account, which proposes changes to accept on Linux. One solution might to add optional flags to accept(), asking for certain behavior variations.

    Adding flags to control inheritance could be done. But here the problem is that the behaviour is buggy even without assuming the API makes any promise w.r.t. inheritance. (in other words, while bpo-10115 is a feature request, this issue is really a bug)

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    Here is the issue (put plainly):

    Unfortunately, I still don't understand it. I go through your plain elaboration below.

    Python sockets support a notion of timeout (note this notion is not reflected in the OS socket API).

    Correct.

    The python socket implementation of timeouts uses the underlying OS / socket API to provide this by setting the socket to nonblocking and setting a timeout value in a Python object that holds socket info.

    Correct.

    This implementation assumes that the OS sets any socket it receives via accept to nonblocking. (this is a false assumption on BSD)

    Not true. It doesn't assume that (it doesn't assume the reverse, either).

    The end result is that the OS has a nonblocking socket and the Python object thinks it is blocking. This is why the socket object in Python has timeout=None yet calling fcntl shows the socket is nonblocking.

    That conclusion is flawed. Python has not associated a timeout with the socket. It makes no claims as to whether the socket is blocking or not. So you have created a non-blocking socket without timeout.

    I cannot see anything wrong with such a thing. The system explicitly supports such sockets (and, as you point out, it doesn't even have the notion of "per-socket timeouts"). And so should Python support them as well.

    Calling code paths that handle timeouts and expect the socket to block causes bugs like I described in my code. This behavior is clearly wrong under any interpretation!

    What about my interpretation above? 1

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    We are talking about the timeout feature, which is a Python feature, not a BSD (or Linux) sockets feature. It should work properly, even if that means adding some boilerplate around system calls.

    I agree that the semantics of the socket timeout API is confusing (and I wish it had not been added in the first place). However, making it work "better" must not cause breakage to other legitimate applications.

    pitrou commented 13 years ago

    Le dimanche 21 novembre 2010 à 20:16 +0000, Martin v. Löwis a écrit :

    > We are talking about the timeout feature, which is a Python feature, not > a BSD (or Linux) sockets feature. It should work properly, even if that > means adding some boilerplate around system calls.

    I agree that the semantics of the socket timeout API is confusing (and I wish it had not been added in the first place). However, making it work "better" must not cause breakage to other legitimate applications.

    Well, I don't think setting a timeout on a listening socket and then expecting the socket received through accept() to be non-blocking (but only on BSD) is a legitimate application.

    292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 commented 13 years ago

    > This implementation assumes that the OS sets any socket it receives > via accept to nonblocking. (this is a false assumption on BSD)

    Not true. It doesn't assume that (it doesn't assume the reverse, either).

    The Python implementation sets timeout=None (which implies that the underlying socket is blocking).

    > The end result is that the OS has a nonblocking socket and the Python > object thinks it is blocking. This is why the socket object in > Python has timeout=None yet calling fcntl shows the socket is > nonblocking.

    That conclusion is flawed. Python has not associated a timeout with the socket. It makes no claims as to whether the socket is blocking or not. So you have created a non-blocking socket without timeout.

    The problem is that it has. It has created a new Python socket object with a specific value for timeout (None), but the underlying socket is nonblocking.

    The docs state that timeout = None makes the socket blocking.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    Well, I don't think setting a timeout on a listening socket and then expecting the socket received through accept() to be non-blocking (but only on BSD) is a legitimate application.

    Right. But setting the server socket to nonblocking, and then expecting the connection socket to also be nonblocking might be.

    292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 commented 13 years ago

    > Well, I don't think setting a timeout on a listening socket and then > expecting the socket received through accept() to be non-blocking (but > only on BSD) is a legitimate application.

    Right. But setting the server socket to nonblocking, and then expecting the connection socket to also be nonblocking might be.

    Okay sure. This is fine. That is why I suggested that if you don't like my patch, one might instead change new Python sockets to inherit the timeout / blocking setting on BSD.

    However, I hope we can all agree that having the Python socket object in a different blocking / non-blocking state than the OS socket descriptor is wrong. This is what needs to be fixed.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    The Python implementation sets timeout=None (which implies that the underlying socket is blocking).

    No, it doesn't. A socket may be non-blocking without having a timeout; that's the socket API (on all systems, not just BSD).

    The problem is that it has. It has created a new Python socket object with a specific value for timeout (None), but the underlying socket is nonblocking.

    The docs state that timeout = None makes the socket blocking.

    What specific wording are you looking at that makes you believe so? To me, the phrase

    # A socket object can be in one of three modes: blocking, non-blocking, # or timeout

    suggests otherwise.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    However, I hope we can all agree that having the Python socket object in a different blocking / non-blocking state than the OS socket descriptor is wrong. This is what needs to be fixed.

    Ok, now I see where your misunderstanding is: you simply can't infer from calling .gettimeout() whether the socket is blocking or not. Python does not "think" you can, nor should you. So I don't think anything needs to be fixed in this respect.

    013be167-c92f-46a8-afa1-7818ebd81a32 commented 13 years ago

    I got into this by starting with bpo-7322, which reports a scenario where data is lost using makefile(). The docs for makefile() say, "The socket must be in blocking mode (it can not have a timeout)". So, we've got published documentation which equates non-blocking mode with a timeout set.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    I got into this by starting with bpo-7322, which reports a scenario where data is lost using makefile(). The docs for makefile() say, "The socket must be in blocking mode (it can not have a timeout)". So, we've got published documentation which equates non-blocking mode with a timeout set.

    This wording goes back to r54491, from bpo-882297. Facundo writes

    # If the socket is in blocking mode, it can NOT have a timeout. # I fixed the docs, and made that explicit.

    so I don't think he really meant to equate the two. I'm not a native speaker (of English), so I'll refrain from proposing an alternative wording.

    ned-deily commented 13 years ago

    The notes in the documentation under socket.gettimeout() do go into more detail than elsewhere. But at least one thing there is at best misleading: "Sockets are always created in blocking mode" is, as we've seen, not correct for BSD-ish systems for sockets returned by accept(). So, at a minimum, this section and the documentation for socket.accept() should be expanded, along with any other changes.

    http://docs.python.org/py3k/library/socket.html#socket.socket.gettimeout http://docs.python.org/py3k/library/socket.html#socket.socket.accept

    013be167-c92f-46a8-afa1-7818ebd81a32 commented 13 years ago

    Responding to msg122013:

    I think he exactly meant to equate the two.

    The original problem described in bpo-882297 is that the makefile() documentation only stated that the socket could not be in non-blocking mode. The test case presented didn't appear to set non-blocking mode, it only set a timeout.

    The explanation by facundobatista was that setting a timeout inplies putting the socket into non-blocking mode, and the docs were updated to make this "non-blocking \<==> timeout" relationship clear.

    pitrou commented 13 years ago

    The notes in the documentation under socket.gettimeout() do go into more detail than elsewhere. But at least one thing there is at best misleading: "Sockets are always created in blocking mode" is, as we've seen, not correct for BSD-ish systems for sockets returned by accept(). So, at a minimum, this section and the documentation for socket.accept() should be expanded, along with any other changes.

    Well, unless someone explains convincingly how the current behaviour is desireable (rather than misleading and useless), I really think this is a bug that should be fix (then we can also discuss what the fix should exactly be).

    292e5c96-0ee3-4b75-bf2f-6d99e58ccbb0 commented 13 years ago

    > The Python implementation sets timeout=None (which implies that the > underlying socket is blocking).

    No, it doesn't. A socket may be non-blocking without having a timeout; that's the socket API (on all systems, not just BSD).

    Sure and this happens when the timeout is 0, but None has a different meaning than 0.

    > The problem is that it has. It has created a new Python socket > object with a specific value for timeout (None), but the underlying > socket is nonblocking. > > The docs state that timeout = None makes the socket blocking.

    What specific wording are you looking at that makes you believe so?

    Here is the last part of the description of settimeout:

    s.settimeout(None) is equivalent to s.setblocking(1)

    if you look at setblocking:

    Set blocking or non-blocking mode of the socket: if flag is 0, the socket is set to non-blocking, else to blocking mode.

    This seems to imply that timeout = None -> blocking.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    Well, unless someone explains convincingly how the current behaviour is desireable (rather than misleading and useless), I really think this is a bug that should be fix (then we can also discuss what the fix should exactly be).

    So please propose a fix.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    Set blocking or non-blocking mode of the socket: if flag is 0, the socket is set to non-blocking, else to blocking mode.

    This seems to imply that timeout = None -> blocking.

    I see. So gettimeout() should really make a system call, to determine whether to return 0.0 or None.

    pitrou commented 13 years ago

    > Well, unless someone explains convincingly how the current behaviour is > desireable (rather than misleading and useless), I really think this is > a bug that should be fix (then we can also discuss what the fix should > exactly be).

    So please propose a fix.

    Well, in case you didn't notice, a fix was already proposed :) (not in proper form, granted, and lacking a test)

    At least now you won't argue that this is normal behaviour, so we are progressing.

    a04be92c-af4e-4c3d-ab01-017f3a697ce8 commented 13 years ago

    Attached is a patch (the original one in patch form) against py3k with unit test.

    It seems to work well - tested on Linux & FreeBSD.

    a04be92c-af4e-4c3d-ab01-017f3a697ce8 commented 13 years ago

    bpo-1515839 seems to be a duplicate of this one.

    pitrou commented 13 years ago

    Thanks for the patch. Comments/questions:

    a04be92c-af4e-4c3d-ab01-017f3a697ce8 commented 13 years ago

    From what I coud see, the same applied to NetBSD so I enabled it for NetBSD as well - if there are any other OSes that need it enabled, they can be added.

    The updated patch checks for fcntl() failing and simply leaves the socket as is if it fails.

    The test silenced socket.error() just because the other tests did like testSetBlocking, testInitNonBlocking, testAccept, etc. I updated it to remove this.

    pitrou commented 13 years ago

    I've tried the patch under OpenSolaris and the test fails (EAGAIN), meaning that accept() semantics there are the same as under BSD:

    \====================================================================== ERROR: testInheritFlags (test.test_socket.NonBlockingTCPTests) ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/antoine/vbox/py3k/cc/Lib/test/test_socket.py", line 983,
    in testInheritFlags
        message = conn.recv(len(MSG))
    socket.error: [Errno 11] Resource temporarily unavailable

    I think the code path in the patch should be opt-out rather than opt-in: that is, it should be executed if HAVE_FCNTL, O_NONBLOCK are defined, and if not under Linux. (and I don't think O_ASYNC is useful here, is it?)

    a04be92c-af4e-4c3d-ab01-017f3a697ce8 commented 13 years ago

    OK try this one, it's now opt-out.

    pitrou commented 13 years ago

    After further testing, it turns out that Windows exhibits BSD-like behaviour too. So instead of complicating the flag-setting code again, I suggest an alternative of doing it in the Python wrapper. Patch attached.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    I like the logic of Antoine's patch much better (basing this on whether the listening socket had a timeout). I wonder whether it's correct though if there is a defaulttimeout: shouldn't it then leave the timeout on the socket instead?

    pitrou commented 13 years ago

    I like the logic of Antoine's patch much better (basing this on whether the listening socket had a timeout). I wonder whether it's correct though if there is a defaulttimeout: shouldn't it then leave the timeout on the socket instead?

    Indeed, so I'll try to make the patch better.

    pitrou commented 13 years ago

    bpo-976613 (duplicate) has a very simple patch, will see if it's sufficient.

    pitrou commented 13 years ago

    A new patch combining Ross' test with the simple approach from bpo-976613. Works under Linux, OpenSolaris and Windows.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    I think this patch (nonblock2.patch) is wrong. If I have a non-blocking server socket on *BSD, and do accept, with no default timeout: IIUC, under the patch, I will get a blocking connection socket. However, according to the operating system API, I'm entitled to get a non-blocking socket (i.e. O_NONBLOCK must be inherited across accept).

    pitrou commented 13 years ago

    I think this patch (nonblock2.patch) is wrong. If I have a non-blocking server socket on *BSD, and do accept, with no default timeout: IIUC, under the patch, I will get a blocking connection socket. However, according to the operating system API, I'm entitled to get a non-blocking socket (i.e. O_NONBLOCK must be inherited across accept).

    Well, either the defaulttimeout should have the priority over the parent socket's settings (your argument in msg125135), or it shouldn't. I'm fine with both, but I think any more complicated combination would end up puzzling for the user :)

    pitrou commented 13 years ago

    Antoine Pitrou \pitrou@free.fr\ added the comment:

    > I think this patch (nonblock2.patch) is wrong. If I have a > non-blocking server socket on *BSD, and do accept, with no default > timeout: IIUC, under the patch, I will get a blocking connection > socket. However, according to the operating system API, I'm entitled > to get a non-blocking socket (i.e. O_NONBLOCK must be inherited across > accept).

    Well, either the defaulttimeout should have the priority over the parent socket's settings (your argument in msg125135), or it shouldn't. I'm fine with both, but I think any more complicated combination would end up puzzling for the user :)

    I would add that, since flags inheritance through accept() is platform-dependent while the default timeout is a well-defined Python feature, I would lean slightly towards applying the default timeout.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    Well, either the defaulttimeout should have the priority over the parent socket's settings (your argument in msg125135), or it shouldn't. I'm fine with both, but I think any more complicated combination would end up puzzling for the user :)

    Applying the default timeout if set is fine, and I agree it should have precedence. In case no default timeout is configured, the platform semantics should prevail. This is tradition in Python: when we define some behavior (i.e. default timeout), we are entitled to consistently define all aspects of it. In the other places we use the platform semantics (leaning towards standards such as POSIX in case the platform offers alternative semantics).

    pitrou commented 13 years ago

    Ok, here is a patch which prefers the default timeout (if set) over fixing of inherited flags. Tested under Linux, Windows, OpenSolaris.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 13 years ago

    Ok, here is a patch which prefers the default timeout (if set) over fixing of inherited flags. Tested under Linux, Windows, OpenSolaris.

    This patch looks fine to me. Please also update the accept documentation to explain the situation (new socket gets default timeout if given, else is blocking if server socket has timeout, else inherits flags according to system defaults).

    I then think that gettimeout is also incorrect: it really fetch the O_NONBLOCK flag from the socket.

    pitrou commented 13 years ago

    Ok, I committed the patch in r87768 and overhauled the timeout docs in r87769. I'm not sure this should be backported (because of the very slight behaviour change), so I'm closing.

    84401114-8e59-4056-83cb-632106c0b648 commented 13 years ago

    Please see bpo-11721 where I was commenting on the same.

    I don't think the documentation makes it clear that socket.gettimeout() can be incorrect (i.e. return None when the socket is non-blocking).

    I also don't think there is a portable way to detect the NBIO attribute of a socket, so we still have a case of socket.gettimeout() not accurately reflecting the blocking state of the socket. If people don't think it is a bug, then this fact should be documented.

    I personally think that this logic should go into socketmodule.c where the rest of the timeout logic sits, rather than be exctracted like this out into the python world.

    Finally, IMHO this is a bug that should be backported as far back as possible.

    Cheers!

    pitrou commented 13 years ago

    I also don't think there is a portable way to detect the NBIO attribute of a socket, so we still have a case of socket.gettimeout() not accurately reflecting the blocking state of the socket

    Which case?

    I personally think that this logic should go into socketmodule.c where the rest of the timeout logic sits, rather than be exctracted like this out into the python world.

    I think practicality beats purity. If putting it in socket.py ends up producing bugs, we'll have to consider moving it to the C extension. Until then, the Python code has the advantage of being clear and concise.

    84401114-8e59-4056-83cb-632106c0b648 commented 13 years ago
    socket.defaulttimeout(None)
    s = socket.socket()
    s.settimeout(0) #nonblocking
    s.bind()
    s2, a = s.accept()
    print s2.gettimeout() #prints ´none´, meaning blocking
    s2.receive(10) #raises EWOULDBLOCK error, since internally it is non-blocking

    I don't agree with practicality vs. purity, particularly when trying to understand the timeout logic. Most of the timeout logic is implemented in c and never touched by python, in init_sockobject(). But then you tack on extra logic in socket.py, in what is even a socket object wrapper. This means that any module that uses the "pure" _socket.socket object, such as C extensions, will not get the "correct" behaviour.

    pitrou commented 13 years ago

    socket.defaulttimeout(None) s = socket.socket() s.settimeout(0) #nonblocking s.bind() s2, a = s.accept() print s2.gettimeout() #prints ´none´, meaning blocking s2.receive(10) #raises EWOULDBLOCK error, since internally it is non-blocking

    Could you post working Python 3 code which demonstrates the issue on 3.3?

    This means that any module that uses the "pure" _socket.socket object, such as C extensions, will not get the "correct" behaviour.

    Using undocumented implementation details (such as the _socket module) is, AFAIK, unsupported.

    Anyway, if you want the code to be changed, please propose a patch so that it can be judged on its own merits (together with tests that demonstrate the improvement, if any).

    cfc9f3e0-e33f-4ecd-9ddd-4123842d6c1e commented 13 years ago

    I'm confused by the patch (ed0259230611). The patch comment and the NEWS item state "the returned socket is now always non-blocking" but the code change adds "sock.setblocking(True)".

    84401114-8e59-4056-83cb-632106c0b648 commented 13 years ago

    Antoine, absolument. Please see attached file bug.py

    As for a different patch, we should agree what behaviour should be expected. I don't think it is possible to rely on some platform specific behaviour. This is because it is in general not possible to query the blokcking state of a socket. Instead we should simply define it for python, and in accordance to established tradition, namely that defaulttimeout prevails.

    Btw, defaulttimeout(None) doesn't mean that that there is no default, it means that the default is "blocking."