I am using the recently introduced FileObject to do cooperative IO on a
file descriptor. The FileObject.read() method by default will retry reads
until it can satisfy the amount of bytes requested. I think these semantics
are not great for non-blocking IO, at least not for my use case. In my case
i am reading the standard output of a command line process, and i do not
know how many bytes of output will be available.

One workaround would be to read 1 byte every time, which would suck for
performance.

The retry read behavior is actually implemented in Python's _fileobject not
in gevent's FileObject.

Hi,

Well, these are the semantics of a file object in Python.

OK, understand. Actually using FileObject was a workaround for me anyway
since i'm working directly with file descriptors and really don't need/want
a file object

I have attached a patch that adds cooperative versions of os.read() and
os.write() to gevent. The patch also updates monkey.patch_os() to install
these versions. I have tested it on Linux and Mac OSX, both the posix
variants as well as the thread pool variants. I don't have a usable
development system on Windows currently so i could not test that platform.

Any chance this could be added?

Thanks,
Geert

gevent-fd.patch 5K Download

Yeah, there is a chance :) I'll test it later. It will be called
gevent.os though.

Wait -- there is a serious problem with the attached posix_read and
posix_write functions when used with things like stdin/stdout file
descriptors. For example, if you attempt to use posix_read() on the
stdin.fileno(), fcntl.fcntl(fd, fcntl.F_SETFL, flags|os.O_NONBLOCK) also
makes stdout non-blocking. This causes writes to stdout to fail
sporadically with something like EWOULDBLOCK. I experienced this on MacOS
(and Linux?) very recently when trying to solve a similar problem. My app
needed to make gevent-friendly reads from stdin, while logging in my app
was configured to go to stdout. So, while my gevent-friendly fd-read
function blocked on the hub waiting for input, other greenlets would
execute and some would make logging calls, which would occasionally result
in an error exception with errno=EWOULDBLOCK because O_NONBLOCK on stdin
also made stdout O_NONBLOCK (stdin and stdout have different file
descriptors (0 and 1), but share fcntl flags, including O_NONBLOCK). My
solution that fixed the problem was to set O_NONBLOCK before os.read() and
restore the original flags in a finally clause immediately after os.read()
inside the loop; this way, the flags will be restored if the non-blocking os_read()
succeeds and posix_read() returns or if there is a greenlet context switch
as the result of hub.wait. Same applies to posix_write().

+def posix_read(fd, n):
+    """Read up to `n` bytes from file descriptor `fd`."""
+    flags = fcntl.fcntl(fd, fcntl.F_GETFL, 0)
+    if not flags & os.O_NONBLOCK:
+        fcntl.fcntl(fd, fcntl.F_SETFL, flags|os.O_NONBLOCK)
+    while True:
+        try:
+            return os_read(fd, n)
+        except OSError:
+            code = sys.exc_info()[1].args[0]
+            if code == EBADF:
+                return ''
+            elif code != EAGAIN:
+                raise
+            sys.exc_clear()
+        hub = get_hub()
+        event = hub.loop.io(fd, 1)
+        try:
+            hub.wait(event)
+        except IOError:
+            code = sys.exc_info()[1].args[0]
+            if code == EBADF:
+                return ''
+            raise

<<

Here is an example of the solution I described above for the os.write case
(this is inside the while loop):
    # Temporarily set fd in NONBLOCKING mode, so the following write won't
block
    originalFdFlags = _makeFDNonBlocking(fd)
    try:
      numBytesSent += os.write(fd, buffer(string, numBytesSent))
    except socket.error as e:
      if e.args[0] != errno.EWOULDBLOCK:
        raise
    finally:
      # Restore fd flags before we (possibly) block on gevent_select (or in
case
      # of exception), so that other threads/greenlets won't be impacted by
our
      # change to this fd (e.g., it might be stderr or stdout fd)
      _restoreFDFcntlFlags(fd, originalFdFlags)

<<

Restoring the fd flags before a context switch seem reasonable.
There's an overhead of one extra syscall per read, but that should
probably be fine. We already need at least 2 syscalls anyway (one to
see if the socket is blocking, one for the read), so adding a third
one maybe isn't such a big deal.

Regards,
Geert

On Jul 25, 9:07 am, Geert Jansen <gee...@gmail.com> wrote:

Right -- it's a necessary overhead since real apps will break without
it. stdin/stdout sharing fcntl flags is but one such example - there
may very well be other pairings like that.

Hi,

Just trying to keep the momentum on this issue... I am trying to create an
updated version of my patch that addresses vitaly's issue and also calls
the module gevent.os like you say above.

However since the "os" module is imported by many gevent submodules, they
would all import the gevent.os instead of the stdlib os. Do you prefer to
expose the entire stdlib's os in gevent.os, or use "from __future__ import
absolute_import" to prevent that?

Regards,
Geert

Even if you expose the entire stdlib's os in gevent.os you'd still
need to access the original os module somehow.

I used to just do __import__('module'). However, now I'd choose absolute_import.

In fact, now that support for python2.4 is dropped, it probably makes
sense to go back and replace all __import__('...') hacks with
absolute_import.

Hi,

Even if you expose the entire stdlib's os in gevent.os you'd still

Attached is an updated patch. Changes:

 * Implemented the proposed new semantics where the patched os.read() and
os.write() restore file descriptor flags.
 * The patched os.read() and os.write() now only raise an OSError.
Previously they could also raise an IOError which is inconsistent with the
stdlib versions.
 * The module is now called gevent.os instead of gevent.fd. This module
just provides the patched functions, it does not expose the entire stdlib.
 * fork() moved to gevent.os as well. This is a user visible change. Hope
that's still OK given that we're in beta, otherwise a stub can be added to
gevent.hub.
 * Added absolute_import where necessary to prevent other gevent code
importing gevent.os instead of the stdlib os.
 * Cleaned up most __import__ hacks and moved to absolute_import.
 * Updated tests for gevent.os.
 * Make sure gevent.fileobject uses the non-patched os.read/os.write.

I have tested this patch on Linux both with the posix and the threadpool
variants. On my system the test suite runs with 25 errors which all are
there before my patch too.

Regards,
Geert

gevent-os.patch 15K Download

Hi,

On Jul 31, 2:48 am, Geert Jansen <gee...@gmail.com> wrote:

The following annotated subset of the proposed gevent-os.patch details
serious bugs and os read/write API incompatibilities that I found in
the patch.  I will post the fixed posix_read and posix_write shortly.

Serious Bugs and OS read/write incompatibilities in the proposed patch
are tagged with BUG below; corresponding
fix proposals are tagged with FIX:

+def _wrap_errors(func, *args):
+    """Convert any error to an OSError."""
+    try:
+        return func(*args)
+    except Exception, e:
         BUG1: this breaks OSError, which is defined as (note errno
and strerror
          attributes): "It is raised when a
          function returns a system-related error (not for illegal
argument
          types or other incidental errors). The errno attribute is a
numeric
          error code from errno, and the strerror attribute is the
corresponding
          string, as would be printed by the C function perror(). See
the module
          errno, which contains names for the error codes defined by
the
          underlying operating system."
         BUG2: This also causes loss of the original traceback, making
it
          impossible for user code to determine which lower-level API
call
          actually failed.
         FIX: Don't use _wrap_errors, it is not needed; the actual
exception
          raised by the lower-level API is the acceptable exception.
+        raise OSError(*e.args)
+
+
+def posix_read(fd, n):
+    """Read up to `n` bytes from file descriptor `fd`."""
+    flags = _wrap_errors(fcntl.fcntl, fd, fcntl.F_GETFL, 0)
+    if not flags & os.O_NONBLOCK:
+        _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags|
os.O_NONBLOCK)
+    try:
+        while True:
+            try:
+                return os_read(fd, n)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: This breaks the definition of os.read, which
is defined
                      as: "Read at most n bytes from file descriptor
fd. Return
                      a string containing the bytes read. If the end
of the file
                      referred to by fd has been reached, an empty
string is
                      returned.".
                     FIX: Remove the "if e[0] == EBADF: return ''"
block. When
                      given a bad file descriptor, os.read raises
                      OSError with errno=EBADF in this case, which is
the
                      correct behavior (try os.read(999999, 10) and
see for yourself);
+                    return ''
+                elif e[0] != EAGAIN:
+                    raise
+                sys.exc_clear()
+            hub = get_hub()
+            event = hub.loop.io(fd, 1)
+            try:
+                _wrap_errors(hub.wait, event)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: same as above EBADF BUG
                     FIX: same as above EBADF FIX; the original
OSError
                      exception with errno=EBADF is the correct
behavior and must
                      NOT be overridden
+                    return ''
+                raise
+    finally:
         BUG: The flags are restored too late; there is a serious
problem with
          these posix_read and posix_write functions when used with
things like
          stdin/stdout file descriptors, or other file descriptors
whose fcntl
          flags are linked in a similar way. For example, if you
attempt to use
          posix_read() on the stdin.fileno(), fcntl.fcntl(fd,
fcntl.F_SETFL, flags|os.O_NONBLOCK)
          also makes stdout non-blocking. This causes writes to stdout
to fail
          sporadically with unexpected errors like EWOULDBLOCK when
attempted
          during gevent context switch while this function is waiting
in hub.wait.
          The converse also fails. I experienced this on MacOS (and
Linux?) very
          recently when trying to solve a similar problem. My app
needed to make
          gevent-friendly reads from stdin, while logging in my app
was configured
          to go to stdout. So, while my gevent-friendly fd-read
function blocked
          on the hub waiting for input, other greenlets would execute
and some
          would make logging calls, which would sporadically and
unexpectedly
          fail  exception with OSError with errno=EWOULDBLOCK because
O_NONBLOCK
          on stdin also made stdout O_NONBLOCK (stdin and stdout have
different
          file descriptors (0 and 1), but share fcntl flags, including
O_NONBLOCK).
         FIX: restore fcntl flags in a finally clause for return
os_write(fd, buf)
          (instead of the outer finally clause, which is not needed)
+        if not flags & os.O_NONBLOCK:
+            _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags)
+
+def posix_write(fd, buf):
+    """Write bytes from buffer `buf` to file descriptor `fd`."""
+    flags = _wrap_errors(fcntl.fcntl, fd, fcntl.F_GETFL, 0)
+    if not flags & os.O_NONBLOCK:
+        _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags|
os.O_NONBLOCK)
+    try:
+        while True:
+            try:
+                return os_write(fd, buf)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: This breaks the definition of os.write,
which is defined
                     as: "Write the string str to file descriptor fd.
Return the
                     number of bytes actually written."
                     FIX: Remove the "if e[0] == EBADF: return ''"
block. When
                      given a bad file descriptor, os.write raises
                      OSError with errno=EBADF in this case, which is
the
                      correct behavior (try os.write(999999, "abc")
and see for yourself);
+                    return ''
+                elif e[0] != EAGAIN:
+                    raise
+                sys.exc_clear()
+            hub = get_hub()
+            event = hub.loop.io(fd, 2)
+            try:
+                _wrap_errors(hub.wait, event)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: same as above EBADF BUG
                     FIX: same as above EBADF FIX; the original
OSError
                      exception with errno=EBADF is the correct
behavior and must
                      NOT be overridden
+                    return ''
+                raise
+    finally:
         BUG: The flags are restored too late; there is a serious
problem with
          these posix_read and posix_write functions when used with
things like
          stdin/stdout file descriptors, or other file descriptors
whose fcntl
          flags are linked in a similar way. For example, if you
attempt to use
          posix_read() on the stdin.fileno(), fcntl.fcntl(fd,
fcntl.F_SETFL, flags|os.O_NONBLOCK)
          also makes stdout non-blocking. This causes writes to stdout
to fail
          sporadically with unexpected errors like EWOULDBLOCK when
attempted
          during gevent context switch while this function is waiting
in hub.wait.
          The converse also fails. I experienced this on MacOS (and
Linux?) very
          recently when trying to solve a similar problem. My app
needed to make
          gevent-friendly reads from stdin, while logging in my app
was configured
          to go to stdout. So, while my gevent-friendly fd-read
function blocked
          on the hub waiting for input, other greenlets would execute
and some
          would make logging calls, which would sporadically and
unexpectedly
          fail  exception with OSError with errno=EWOULDBLOCK because
O_NONBLOCK
          on stdin also made stdout O_NONBLOCK (stdin and stdout have
different
          file descriptors (0 and 1), but share fcntl flags, including
O_NONBLOCK).
         FIX: restore fcntl flags in a finally clause for return
os_write(fd, buf)
          (instead of the outer finally clause, which is not needed)
+        if not flags & os.O_NONBLOCK:
+            _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags)

Thanks to Geert Jansen for the original posting of the patch and getting
everyone motivated around this.  I am attaching new versions of  posix_read
and posix_write with fixes for all the bugs and os read/write
incompatibilities that I identified in my previous post.  Please review.

Thank you,
Vitaly

Not sure what happened to the attachment, so trying to attach again...

posix_read_write.py 1K Download

Hi,

vitaly:

Could you please re-post this comment WITHOUT word wrapping?
Your comments are mostly unreadable because of it.

--
-- Matthias Urlichs

This is the requested re-post (hopefully without ugly word-wrapping this
time) of my earlier post that identifies a number of bugs and os read/write
incompatibilities in Geert Jansen's patch proposal for
posix_read/posix_write.  (I am wrapping it in <PRE> and </PRE> tags this
time.  If this doesn't help with the ugly wrapping, please suggest a more
appropriate technique).  My earlier post also includes an attachment with
fixes for these issues.

<PRE>
Serious Bugs and os read/write incompatibilities in the proposed patch are
tagged with BUG below; corresponding fix proposals are tagged with FIX:

+def _wrap_errors(func, *args):
+    """Convert any error to an OSError."""
+    try:
+        return func(*args)
+    except Exception, e:
         BUG1: this breaks OSError, which is defined as (note errno and
strerror
          attributes): "It is raised when a
          function returns a system-related error (not for illegal argument
          types or other incidental errors). The errno attribute is a
numeric
          error code from errno, and the strerror attribute is the
corresponding
          string, as would be printed by the C function perror(). See the
module
          errno, which contains names for the error codes defined by the
          underlying operating system."
         BUG2: This also causes loss of the original traceback, making it
          impossible for user code to determine which lower-level API call
          actually failed.
         FIX: Don't use _wrap_errors, it is not needed; the actual exception
          raised by the lower-level API is the acceptable exception.
+        raise OSError(*e.args)
+
+
+def posix_read(fd, n):
+    """Read up to `n` bytes from file descriptor `fd`."""
+    flags = _wrap_errors(fcntl.fcntl, fd, fcntl.F_GETFL, 0)
+    if not flags & os.O_NONBLOCK:
+        _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags|os.O_NONBLOCK)
+    try:
+        while True:
+            try:
+                return os_read(fd, n)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: This breaks the definition of os.read, which is
defined
                      as: "Read at most n bytes from file descriptor fd.
Return
                      a string containing the bytes read. If the end of the
file
                      referred to by fd has been reached, an empty string is
                      returned.".
                     FIX: Remove the "if e[0] == EBADF: return ''" block.
When
                      given a bad file descriptor, os.read raises
                      OSError with errno=EBADF in this case, which is the
                      correct behavior (try os.read(999999, 10) and see for
                      yourself);
+                    return ''
+                elif e[0] != EAGAIN:
+                    raise
+                sys.exc_clear()
+            hub = get_hub()
+            event = hub.loop.io(fd, 1)
+            try:
+                _wrap_errors(hub.wait, event)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: same as above EBADF BUG
                     FIX: same as above EBADF FIX; the original OSError
                      exception with errno=EBADF is the correct behavior
and must
                      NOT be overridden
+                    return ''
+                raise
+    finally:
         BUG: The flags are restored too late; there is a serious problem
with
          these posix_read and posix_write functions when used with things
like
          stdin/stdout file descriptors, or other file descriptors whose
fcntl
          flags are linked in a similar way. For example, if you attempt to
use
          posix_read() on the stdin.fileno(), fcntl.fcntl(fd,
fcntl.F_SETFL, flags|os.O_NONBLOCK)
          also makes stdout non-blocking. This causes writes to stdout to
fail
          sporadically with unexpected errors like EWOULDBLOCK when
attempted
          during gevent context switch while this function is waiting in
hub.wait.
          The converse also fails. I experienced this on MacOS (and Linux?)
very
          recently when trying to solve a similar problem. My app needed to
make
          gevent-friendly reads from stdin, while logging in my app was
configured
          to go to stdout. So, while my gevent-friendly fd-read function
blocked
          on the hub waiting for input, other greenlets would execute and
some
          would make logging calls, which would sporadically and
unexpectedly
          fail  exception with OSError with errno=EWOULDBLOCK because
O_NONBLOCK
          on stdin also made stdout O_NONBLOCK (stdin and stdout have
different
          file descriptors (0 and 1), but share fcntl flags, including
O_NONBLOCK).
         FIX: restore fcntl flags in a finally clause for return
os_write(fd, buf)
          (instead of the outer finally clause, which is not needed)
+        if not flags & os.O_NONBLOCK:
+            _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags)
+
+def posix_write(fd, buf):
+    """Write bytes from buffer `buf` to file descriptor `fd`."""
+    flags = _wrap_errors(fcntl.fcntl, fd, fcntl.F_GETFL, 0)
+    if not flags & os.O_NONBLOCK:
+        _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags|os.O_NONBLOCK)
+    try:
+        while True:
+            try:
+                return os_write(fd, buf)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: This breaks the definition of os.write, which is
defined
                     as: "Write the string str to file descriptor fd.
Return the
                     number of bytes actually written."
                     FIX: Remove the "if e[0] == EBADF: return ''" block.
When
                      given a bad file descriptor, os.write raises
                      OSError with errno=EBADF in this case, which is the
                      correct behavior (try os.write(999999, "abc") and see
for yourself);
+                    return ''
+                elif e[0] != EAGAIN:
+                    raise
+                sys.exc_clear()
+            hub = get_hub()
+            event = hub.loop.io(fd, 2)
+            try:
+                _wrap_errors(hub.wait, event)
+            except OSError, e:
+                if e[0] == EBADF:
                     BUG: same as above EBADF BUG
                     FIX: same as above EBADF FIX; the original OSError
                      exception with errno=EBADF is the correct behavior
and must
                      NOT be overridden
+                    return ''
+                raise
+    finally:
         BUG: The flags are restored too late; there is a serious problem
with
          these posix_read and posix_write functions when used with things
like
          stdin/stdout file descriptors, or other file descriptors whose
fcntl
          flags are linked in a similar way. For example, if you attempt to
use
          posix_read() on the stdin.fileno(), fcntl.fcntl(fd,
fcntl.F_SETFL, flags|os.O_NONBLOCK)
          also makes stdout non-blocking. This causes writes to stdout to
fail
          sporadically with unexpected errors like EWOULDBLOCK when
attempted
          during gevent context switch while this function is waiting in
hub.wait.
          The converse also fails. I experienced this on MacOS (and Linux?)
very
          recently when trying to solve a similar problem. My app needed to
make
          gevent-friendly reads from stdin, while logging in my app was
configured
          to go to stdout. So, while my gevent-friendly fd-read function
blocked
          on the hub waiting for input, other greenlets would execute and
some
          would make logging calls, which would sporadically and
unexpectedly
          fail  exception with OSError with errno=EWOULDBLOCK because
O_NONBLOCK
          on stdin also made stdout O_NONBLOCK (stdin and stdout have
different
          file descriptors (0 and 1), but share fcntl flags, including
O_NONBLOCK).
         FIX: restore fcntl flags in a finally clause for return
os_write(fd, buf)
          (instead of the outer finally clause, which is not needed)
+        if not flags & os.O_NONBLOCK:
+            _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags)
</PRE>

Hi Vitaly, thanks for the review.

See a few comments inline below.

The line wrapping in your response makes it hard to read by the way.

+def _wrap_errors(func, *args):

         BUG2: This also causes loss of the original traceback, making

The stdlib variant of os.read() only returns an OSError, never an IOError.
However fcntl will return an IOError. So we need to translate that IOError
into an OSError. Otherwise code that calls os.read() and only catches
OSError will break with our monkey patched version. This actually happened
to me in the test_ssl.py test suite that failed for me.

I think the proper fix is to catch everything /except/ OSError. OSError
should be re-raised with the original trackback, other errors should be
translated to OSError.

+        while True:

The EBADF test is actually copied from fileobject.py. I think it is not
correct around os_read() but it may be correct around hub.wait(). The first
time the fd was read from (in the os_read() call above), the file
descriptor was OK and it returned EAGAIN. Then you switch to the hub, which
calls select/poll on it, and for some reason the fd has gone bad. I think
the reason that this test was in fileobject.py is that you want to first
communicate an EOF to the user, and then next time you communicate the
EBADF (when os_read() is called again).

You are right about restoring the flags before the context switch to the
hub by the way. You mentioned that before, and it's a logic error on my
side.

I've added a v3 of the patch that i think addresses all points.

Regards,
Geert

gevent-os-v3.patch 15K Download

Argh.. Now attaching the right patch.

Regards,
Geert

gevent-os-v3.patch 15K Download

Apologies for spamming everybody's inboxes (and damn Google that doesn't
let me check an attachment after i upload it).

This is the right one.

Regards,
Geert

gevent-os-v3.patch 15K Download

Hi Geert, here I wanted to comment only about the following statement from
your earlier post, and I will post additional details in a separate post.

So, regarding

The EBADF test is actually copied from fileobject.py. I think it is not
correct around os_read() but it may be correct around hub.wait(). The first
time the fd was read from (in the os_read() call above), the file
descriptor was OK and it returned EAGAIN. Then you switch to the hub, which
calls select/poll on it, and for some reason the fd has gone bad. I think
the reason that this test was in fileobject.py is that you want to first
communicate an EOF to the user, and then next time you communicate the
EBADF (when os_read() is called again).
<<

EBADF usually means that either we were passed an invalid file descriptor
or that another thread/greenlet closed that file descriptor before we were
done using it. Both cases are either application errors or intentional
behavior by the apps and MUST be signaled by raising the intended exception
(OSError/EBADF in this case).

P.S., sorry about the line-wrapping; I'm in learning mode when it comes to
Google Groups Markup.

Hi Geert, I think we're almost there.  Here is my review of your latest
patch, with errors tagged with "BUG".  I am also attaching the version with
my fixes for those bugs. One thing became apparent to me is that when we
make certain non-obvious decisions in the code, we need to document why we
chose that path in order to help maintainers of the code with future
modifications.  So, here is the feedback:

<PRE>

+def _wrap_errors(func, *args):
+    """Convert any error to an OSError."""
+    try:
+        return func(*args)
+    except OSError:
+        raise
+    except Exception, e:
          # BUG: We may not arbitrarily map all Exception-based instances to
          #  OSError because OSError makes certain assumptions about its
args. In
          #  particular, there is the 2-tuple rule for both IOError and
OSError
          #  that stipulates that in case of a 2-tuple arg, the first
sub-arg in
          #  that case must be a valid errno and the second sub-arg must be
the
          #  corresponding strerror string.
          #
          # For example, if we allow something like this to be created:
          #  oserror = OSError("a", "aaa")
          # then oserror.errno will evaluate to "a", which is an invalid
errno!
+        raise OSError(*e.args)

+def posix_read(fd, n):
+    """Read up to `n` bytes from file descriptor `fd`."""
+    while True:
+        flags = _wrap_errors(fcntl.fcntl, fd, fcntl.F_GETFL, 0)
+        if not flags & os.O_NONBLOCK:
+            _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL,
flags|os.O_NONBLOCK)
+        try:
+            return os_read(fd, n)
+        except OSError, e:
+            if e[0] != EAGAIN:
+                raise
+            sys.exc_clear()
+        finally:
+            if not flags & os.O_NONBLOCK:
+                _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags)
+        hub = get_hub()
+        event = hub.loop.io(fd, 1)
+        try:
+            _wrap_errors(hub.wait, event)
+        except OSError, e:
+            if e[0] == EBADF:
                 # BUG: Built-in os.read is documented to "Return a string
                 #  containing the bytes read. If the end of the file
referred
                 #  to by fd has been reached, an empty string is returned."
                 #
                 # EBADF is very different from "end of file", so returning
an
                 #  empty string is a bug in this case. EBADF usually means
                 #  that either we were passed an invalid file descriptor
or that
                 #  another thread/greenlet closed that file descriptor
before we
                 #  were done using it. Both cases are either application
errors
                 #  or intentional behavior by the app and MUST be signaled
by
                 #  raising the intended exception (OSError/EBADF in this
case).
+                return ''
+            raise
+
+def posix_write(fd, buf):
+    """Write bytes from buffer `buf` to file descriptor `fd`."""
+    while True:
+        flags = _wrap_errors(fcntl.fcntl, fd, fcntl.F_GETFL, 0)
+        if not flags & os.O_NONBLOCK:
+            _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL,
flags|os.O_NONBLOCK)
+        try:
+            return os_write(fd, buf)
+        except OSError, e:
+            if e[0] != EAGAIN:
+                raise
+            sys.exc_clear()
+        finally:
+            if not flags & os.O_NONBLOCK:
+                _wrap_errors(fcntl.fcntl, fd, fcntl.F_SETFL, flags)
+        hub = get_hub()
+        event = hub.loop.io(fd, 2)
+        try:
+            _wrap_errors(hub.wait, event)
+        except OSError, e:
+            if e[0] == EBADF:
                 # 2 BUGs: Built-in os.write is documented to "Return the
number of
                 #  bytes actually written."  1. So, it's a bug to return
empty
                 #  string; 2. It's also a bug to suppress OSError/EBADF
for the
                 #  following reason:
                 #
                 # EBADF usually means that either we were passed an invalid
                 #  file descriptor or that another thread/greenlet closed
that
                 #  file descriptor before we were done using it. Both are
                 #  either application errors or intentional behavior and
MUST
                 #  be signaled by raising the intended exception
(OSError/EBADF
                 #  in this case).
+                return ''
+            raise

</PRE>

posix_read_write_2_with_fixes.py 4K Download

Thanks Vitaly,

Hi Geert, I think we're almost there.  Here is my review of your latest

Summarizing your last points for conciseness:

 1. Map IOError to IOError but do not map other errors.
 2. Remove the EBADF logic when switching to the hub.
 3. Document some of the reasoning.

Regarding #1 - I am OK with this, but with the remark that that solution
isn't 100% clean either. If at some point hub.wait() starts raising errors
other than IOError/OSError, this will break code that is calling
os.read/write and is only expecting OSError as the possible exception.

Regarding #2 - I am happy to remove it, which i did in the attached patch.
But i copied this from fileobject.py and there must be some kind of reason
for this code. Denis?

Regarding #3 - sure but let's be a little less verbose :)

Updated patch attached. Any more suggestions from your side? If not anybody
else or if not can my patch be added?

Regards,
Geert

gevent-os-v4.patch 16K Download

Thanks Geert!  I agree with the reasoning and conclusions in this post.  
Regarding comments, my lengthy ones were meant only for communication with
you, not for direct inclusion in final code -- nice editing!  I also don't
know why fileobject.py does that with EBADF, but I think that would be a
bug for posix_read/write.  I reviewed the source code for posix_read and
posix_write in your attachment, but haven't tried running it.  The code
looks nice/tidy.  I would only change these things:

1. Document the return values of posix_read/posix_write in their
docstrings; this will help with understanding the API and with maintenance;
in my proposals, I used text from python documentation:
posix_read: "Read at most n bytes from file descriptor fd. Return a string
containing the bytes read. If the end of the file referred to by fd has
been reached, an empty string is returned."
posix_write: "Write the string buf to file descriptor fd. Return the number
of bytes actually written."

2. e[0] in posix_read and posix_write is not safe; e.errno (instead of
e[0]) is safer for OSError and IOError; for example:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: tuple index out of range
'None'

Many thanks!

Vitaly

Thank you,
Vitaly

Hi Geert, any update on this?

Updated patch attached (gevent-os-v5).

Regards,
Geert

gevent-os-v5.patch 16K Download