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

SocketIO should return None on EWOULDBLOCK #54063

Closed
pitrou opened this issue Sep 14, 2010 · 8 comments
Closed

SocketIO should return None on EWOULDBLOCK #54063

pitrou opened this issue Sep 14, 2010 · 8 comments
Labels
stdlib Python modules in the Lib dir topic-IO type-bug An unexpected behavior, bug, or error

Comments

@pitrou
Copy link
Member

pitrou commented Sep 14, 2010

BPO 9854
Nosy @amauryfa, @pitrou, @giampaolo, @benjaminp
Files
  • sockio_nonblock.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 2011-01-03.21:19:10.884>
    created_at = <Date 2010-09-14.16:19:55.879>
    labels = ['type-bug', 'library', 'expert-IO']
    title = 'SocketIO should return None on EWOULDBLOCK'
    updated_at = <Date 2011-01-03.21:19:10.883>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2011-01-03.21:19:10.883>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-01-03.21:19:10.884>
    closer = 'pitrou'
    components = ['Library (Lib)', 'IO']
    creation = <Date 2010-09-14.16:19:55.879>
    creator = 'pitrou'
    dependencies = []
    files = ['18884']
    hgrepos = []
    issue_num = 9854
    keywords = ['patch']
    message_count = 8.0
    messages = ['116408', '116417', '116421', '116426', '116428', '116471', '116477', '125244']
    nosy_count = 5.0
    nosy_names = ['amaury.forgeotdarc', 'pitrou', 'giampaolo.rodola', 'benjamin.peterson', 'stutzbach']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue9854'
    versions = ['Python 3.2']

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 14, 2010

    SocketIO claims to implement RawIOBase, but it lets blocking IO errors pass through on non-blocking sockets:

    >>> s = socket.create_connection(("python.org", 80)); s.settimeout(0.0)
    >>> f = s.makefile("rb", buffering=0)
    >>> f.readinto(bytearray(10))
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/antoine/py3k/nntp-9360/Lib/socket.py", line 228, in readinto
        return self._sock.recv_into(b)
    socket.error: [Errno 11] Resource temporarily unavailable

    Instead, readinto() should detect the blocking condition (EAGAIN / EWOULDBLOCK) and return None (same for write(), I imagine).

    There also seems to be a problem in the default read() implementation in RawIOBase, which doesn't accept a possible None result from readinto():

    >>> f.readinto = lambda x: None
    >>> f.read(1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: 'NoneType' object cannot be interpreted as an integer

    (the RawIOBase docs themselves don't mention that readinto() can return None, but it's the only logical possibility: catching EWOULDBLOCK in read() but not in readinto() wouldn't make sense)

    @pitrou pitrou added stdlib Python modules in the Lib dir topic-IO type-bug An unexpected behavior, bug, or error labels Sep 14, 2010
    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 14, 2010

    The problem with RawIOBase.read is fixed in r84814.

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 14, 2010

    Here is a patch.
    The tests are only run for unbuffered objects (buffering=0), since the behaviour of buffered objects is driven by BufferedReader and friends, not by the wrapped SocketIO.

    @giampaolo
    Copy link
    Contributor

    I've never used socket.socket.makefile so I'm not sure, but its documentation says:

    The socket must be in blocking mode (it can not have a timeout).

    If the statement is there because EAGAIN/EWOULDBLOCK were originally raised then it should be removed, otherwise I question whether makefile() is actually supposed to support non-blocking sockets in the first place.
    IMO, I think it's a matter of figuring out whether makefile() should provide a socket-like behavior or a file like-behavior first.
    In the first case I would expect all errors be raised as if I'm dealing with a common socket, otherwise they should be silenced/handled internally or makefile() just fail immediately as there's not such thing as "non-blocking files".

    Instead, readinto() should detect the blocking condition (EAGAIN / EWOULDBLOCK) and
    return None (same for write(), I imagine).

    io.RawIOBase.readinto doc says:

    Read up to len(b) bytes into bytearray b and return the number of bytes read.

    ...so returning 0 instead of None looks more natural to me.
    Same for write, also because:

    >>> open('xxx', 'w').write('')
    0

    I've also noticed that socket.SocketIO.readinto has a while loop which continues in case of EINTR and that's something which should be removed in case makefile() actually intends to support non-blocking sockets.

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 14, 2010

    Le mardi 14 septembre 2010 à 23:19 +0000, Giampaolo Rodola' a écrit :

    I've never used socket.socket.makefile so I'm not sure, but its
    documentation says:

    > The socket must be in blocking mode (it can not have a timeout).

    If the statement is there because EAGAIN/EWOULDBLOCK were originally
    raised then it should be removed, otherwise I question whether
    makefile() is actually supposed to support non-blocking sockets in
    the first place.

    If it wasn't supposed to, we can add that support.
    The statement comes from the 2.x doc; 2.x didn't have a notion of non-blocking file-like objects at all, so it makes sense.

    IMO, I think it's a matter of figuring out whether makefile() should
    provide a socket-like behavior or a file like-behavior first.

    Well, since it claims to implement RawIOBase, it should provide a RawIOBase-like behaviour :)
    (and, in any case, the only use of makefile() is to return something file-like. If you want socket-like behaviour, just use the socket)

    Returning None is what raw I/O objects are supposed to do when they fail reading or writing even a single byte. It is designed and documented as such.

    (the readinto() doc you mentioned lacked that precision, but I've just fixed it)

    The reason None is returned (rather than 0 or b'') is so that the caller can recognize the situation and take appropriate measures.
    For example, if read() returns None, it means some bytes *may* be following but we'll have to wait (select/poll/...) first; if read() returns 0, conversely, it means we've reached EOF (or, on a socket, that the peer has shutdown its side of the connection). These are two very different situations.

    ["file-like behaviour"]

    otherwise they should be
    silenced/handled internally or makefile() just fail immediately as
    there's not such thing as "non-blocking files".

    Non-blocking files exist:

    >>> import fcntl, os, io
    >>> r, w = os.pipe()
    >>> fcntl.fcntl(r, fcntl.F_SETFL, os.O_NONBLOCK)
    0
    >>> os.read(r, 2)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [Errno 11] Resource temporarily unavailable

    (It seems that on regular files O_NONBLOCK may not have an effect; not under Linux anyway)

    @giampaolo
    Copy link
    Contributor

    Non-blocking files exist

    Of course, I should have been more clear.
    What I meant is that there's no such thing as explicit and "native" as setblocking() for plain files.

    Returning None is what raw I/O objects are supposed to do when they
    fail reading or writing even a single byte. It is designed and
    documented as such.

    From http://docs.python.org/dev/library/io.html#module-io about io.BufferedIOBase.readinto()

    A BlockingIOError is raised if the underlying raw stream is in non
    blocking-mode, and has no data available at the moment.

    This is valid for BufferedReader, BufferWriter and BufferIOBase classes in various methods while io.RawIOBase.write() and io.RawIOBase.read() return None instead. Shouldn't they raise BlockingIOError as well? Why do they return None?

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 15, 2010

    Of course, I should have been more clear.
    What I meant is that there's no such thing as explicit and "native" as
    setblocking() for plain files.

    Indeed. It would probably be a nice addition, assuming it is portable
    enough.

    > A BlockingIOError is raised if the underlying raw stream is in non
    > blocking-mode, and has no data available at the moment.

    This is valid for BufferedReader, BufferWriter and BufferIOBase
    classes in various methods while io.RawIOBase.write() and
    io.RawIOBase.read() return None instead. Shouldn't they raise
    BlockingIOError as well? Why do they return None?

    Well, that's how it was decided when the new IO lib was designed.
    See http://www.python.org/dev/peps/pep-3116/#non-blocking-i-o
    (but this says that write() should return 0, while the FileIO
    implementation returns None; I'd say that for consistency with read()
    and readinto() returning None is the right thing).

    @pitrou
    Copy link
    Member Author

    pitrou commented Jan 3, 2011

    Was committed in r84887.

    @pitrou pitrou closed this as completed Jan 3, 2011
    @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 topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants