Improve compliance of select.select and select.poll

- Compliance: If :func:`gevent.select.select` is given a negative *timeout*
  argument, raise an exception like the standard library does.
- Compliance: If :func:`gevent.select.select` is given closed or invalid
  file descriptors in any of its lists, raise the appropriate
  ``EBADF`` exception like the standard library does. Previously,
  libev would tend to return the descriptor as ready. In the worst
  case, this adds an extra system call, but may also reduce latency if
  descriptors are ready at the time of entry.
- Compliance: :meth:`gevent.select.poll.unregister` raises an exception if *fd* is not
  registered, like the standard library.
- Compliance: :meth:`gevent.select.poll.poll` returns an event with
  ``POLLNVAL`` for registered fds that are invalid. Previously it
  would tend to report both read and write events.

changelog.rst

@@ -33,6 +33,20 @@
 - The modules :mod:`gevent.os`, :mod:`gevent.signal` and
   :mod:`gevent.select` export all the attributes from their
   corresponding standard library counterpart.
+- Compliance: If :func:`gevent.select.select` is given a negative *timeout*
+  argument, raise an exception like the standard library does.
+- Compliance: If :func:`gevent.select.select` is given closed or invalid
+  file descriptors in any of its lists, raise the appropriate
+  ``EBADF`` exception like the standard library does. Previously,
+  libev would tend to return the descriptor as ready. In the worst
+  case, this adds an extra system call, but may also reduce latency if
+  descriptors are ready at the time of entry.
+- Compliance: :meth:`gevent.select.poll.unregister` raises an exception if *fd* is not
+  registered, like the standard library.
+- Compliance: :meth:`gevent.select.poll.poll` returns an event with
+  ``POLLNVAL`` for registered fds that are invalid. Previously it
+  would tend to report both read and write events.
+
 
 1.1.0 (Mar 5, 2016)
 ===================

gevent/_compat.py

@@ -40,8 +40,10 @@ else:
 ## Functions
 if PY3:
     iteritems = dict.items
+    itervalues = dict.values
     xrange = range
 
 else:
     iteritems = dict.iteritems # python 3: pylint:disable=no-member
+    itervalues = dict.itervalues # python 3: pylint:disable=no-member
     xrange = __builtin__.xrange # python 2: pylint:disable=redefined-variable-type

gevent/select.py

@@ -3,15 +3,20 @@
 Waiting for I/O completion.
 """
 from __future__ import absolute_import
+
 from gevent.event import Event
 from gevent.hub import get_hub
 from gevent._compat import integer_types
+from gevent._compat import iteritems
+from gevent._compat import itervalues
 from gevent._util import copy_globals
+from gevent._util import _NONE
 
+from select import select as _original_select
 
 try:
     from select import poll as original_poll
-    from select import POLLIN, POLLOUT
+    from select import POLLIN, POLLOUT, POLLNVAL
     __implements__ = ['select', 'poll']
 except ImportError:
     original_poll = None
@@ -27,6 +32,9 @@ __imports__ = copy_globals(__select__, globals(),
                            names_to_ignore=__all__,
                            dunder_names_to_keep=())
 
+_EV_READ = 1
+_EV_WRITE = 2
+
 def get_fileno(obj):
     try:
         fileno_f = obj.fileno
@@ -39,7 +47,7 @@ def get_fileno(obj):
 
 
 class SelectResult(object):
-    __slots__ = ['read', 'write', 'event']
+    __slots__ = ('read', 'write', 'event')
 
     def __init__(self):
         self.read = []
@@ -50,53 +58,99 @@ class SelectResult(object):
         self.read.append(socket)
         self.event.set()
 
+    add_read.event = _EV_READ
+
     def add_write(self, socket):
         self.write.append(socket)
         self.event.set()
 
+    add_write.event = _EV_WRITE
+
+    def __add_watchers(self, watchers, fdlist, callback, io, pri):
+        for fd in fdlist:
+            watcher = io(get_fileno(fd), callback.event)
+            watcher.priority = pri
+            watchers.append(watcher)
+            watcher.start(callback, fd)
+
+    def _make_watchers(self, watchers, rlist, wlist):
+        loop = get_hub().loop
+        io = loop.io
+        MAXPRI = loop.MAXPRI
+
+        try:
+            self.__add_watchers(watchers, rlist, self.add_read, io, MAXPRI)
+            self.__add_watchers(watchers, wlist, self.add_write, io, MAXPRI)
+        except IOError as ex:
+            raise error(*ex.args)
+
+    def _closeall(self, watchers):
+        for watcher in watchers:
+            watcher.stop()
+        del watchers[:]
+
+    def select(self, rlist, wlist, timeout):
+        watchers = []
+        try:
+            self._make_watchers(watchers, rlist, wlist)
+            self.event.wait(timeout=timeout)
+            return self.read, self.write, []
+        finally:
+            self._closeall(watchers)
+
 
 def select(rlist, wlist, xlist, timeout=None): # pylint:disable=unused-argument
     """An implementation of :meth:`select.select` that blocks only the current greenlet.
 
-    Note: *xlist* is ignored.
+    .. caution:: *xlist* is ignored.
+
+    .. versionchanged:: 1.2a1
+       Raise a :exc:`ValueError` if timeout is negative. This matches Python 3's
+       behaviour (Python 2 would raise a ``select.error``). Previously gevent had
+       undefined behaviour.
+    .. versionchanged:: 1.2a1
+       Raise an exception if any of the file descriptors are invalid.
     """
-    watchers = []
-    loop = get_hub().loop
-    io = loop.io
-    MAXPRI = loop.MAXPRI
+    if timeout is not None and timeout < 0:
+        # Raise an error like the real implementation; which error
+        # depends on the version. Python 3, where select.error is OSError,
+        # raises a ValueError (which makes sense). Older pythons raise
+        # the error from the select syscall...but we don't actually get there.
+        # We choose to just raise the ValueError as it makes more sense and is
+        # forward compatible (plus we don't have to import errno)
+        raise ValueError("timeout must be non-negative")
+
+    # First, do a poll with the original select system call. This
+    # is the most efficient way to check to see if any of the file descriptors
+    # have previously been closed and raise the correct corresponding exception.
+    sel_results = _original_select(rlist, wlist, [], 0)
+    if sel_results[0] or sel_results[1]:
+        # If we actually had stuff ready, go ahead and return it. No need
+        # to go through the trouble of doing our own stuff.
+        return sel_results
+
     result = SelectResult()
-    try:
-        try:
-            for readfd in rlist:
-                watcher = io(get_fileno(readfd), 1)
-                watcher.priority = MAXPRI
-                watcher.start(result.add_read, readfd)
-                watchers.append(watcher)
-            for writefd in wlist:
-                watcher = io(get_fileno(writefd), 2)
-                watcher.priority = MAXPRI
-                watcher.start(result.add_write, writefd)
-                watchers.append(watcher)
-        except IOError as ex:
-            raise error(*ex.args)
-        result.event.wait(timeout=timeout)
-        return result.read, result.write, []
-    finally:
-        for awatcher in watchers:
-            awatcher.stop()
+    return result.select(rlist, wlist, timeout)
+
 
 if original_poll is not None:
     class PollResult(object):
-        __slots__ = ['events', 'event']
+        __slots__ = ('events', 'event')
 
         def __init__(self):
             self.events = set()
             self.event = Event()
 
         def add_event(self, events, fd):
-            result_flags = 0
-            result_flags |= POLLIN if events & 1 else 0
-            result_flags |= POLLOUT if events & 2 else 0
+            if events < 0:
+                result_flags = POLLNVAL
+            else:
+                result_flags = 0
+                if events & _EV_READ:
+                    result_flags = POLLIN
+                if events & _EV_WRITE:
+                    result_flags |= POLLOUT
+
             self.events.add((fd, result_flags))
             self.event.set()
 
@@ -104,35 +158,62 @@ if original_poll is not None:
         """
         An implementation of :class:`select.poll` that blocks only the current greenlet.
 
+        .. caution:: ``POLLPRI`` data is not supported.
+
         .. versionadded:: 1.1b1
         """
         def __init__(self):
-            self.fds = {}
+            self.fds = {} # {int -> watcher}
             self.loop = get_hub().loop
 
-        def register(self, fd, eventmask=POLLIN | POLLOUT):
-            flags = 0
-            flags |= 1 if eventmask & POLLIN else 0
-            flags |= 2 if eventmask & POLLOUT else 0
-            watcher = self.loop.io(get_fileno(fd), flags)
+        def register(self, fd, eventmask=_NONE):
+            if eventmask is _NONE:
+                flags = _EV_READ | _EV_WRITE
+            else:
+                flags = 0
+                if eventmask & POLLIN:
+                    flags = _EV_READ
+                if eventmask & POLLOUT:
+                    flags |= _EV_WRITE
+                # If they ask for POLLPRI, we can't support
+                # that. Should we raise an error?
+
+            fileno = get_fileno(fd)
+            watcher = self.loop.io(fileno, flags)
             watcher.priority = self.loop.MAXPRI
-            self.fds[fd] = watcher
+            self.fds[fileno] = watcher
 
         def modify(self, fd, eventmask):
             self.register(fd, eventmask)
 
         def poll(self, timeout=None):
+            """
+            poll the registered fds.
+
+            .. versionchanged:: 1.2a1
+               File descriptors that are closed are reported with POLLNVAL.
+            """
             result = PollResult()
             try:
-                for fd in self.fds:
-                    self.fds[fd].start(result.add_event, get_fileno(fd), pass_events=True)
+                for fd, watcher in iteritems(self.fds):
+                    watcher.start(result.add_event, fd, pass_events=True)
                 if timeout is not None and timeout > -1:
                     timeout /= 1000.0
                 result.event.wait(timeout=timeout)
                 return list(result.events)
             finally:
-                for afd in self.fds:
-                    self.fds[afd].stop()
+                for awatcher in itervalues(self.fds):
+                    awatcher.stop()
 
         def unregister(self, fd):
-            self.fds.pop(fd, None)
+            """
+            Unregister the *fd*.
+
+            .. versionchanged:: 1.2a1
+               Raise a `KeyError` if *fd* was not registered, like the standard
+               library. Previously gevent did nothing.
+            """
+            fileno = get_fileno(fd)
+            del self.fds[fileno]
+
+del original_poll

greentest/3.4/test_select.py

+import errno
+import os
+import select
+import sys
+import unittest
+from test import support
+
+@unittest.skipIf((sys.platform[:3]=='win'),
+                 "can't easily test on this system")
+class SelectTestCase(unittest.TestCase):
+
+    class Nope:
+        pass
+
+    class Almost:
+        def fileno(self):
+            return 'fileno'
+
+    def test_error_conditions(self):
+        self.assertRaises(TypeError, select.select, 1, 2, 3)
+        self.assertRaises(TypeError, select.select, [self.Nope()], [], [])
+        self.assertRaises(TypeError, select.select, [self.Almost()], [], [])
+        self.assertRaises(TypeError, select.select, [], [], [], "not a number")
+        self.assertRaises(ValueError, select.select, [], [], [], -1)
+
+    # Issue #12367: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/155606
+    @unittest.skipIf(sys.platform.startswith('freebsd'),
+                     'skip because of a FreeBSD bug: kern/155606')
+    def test_errno(self):
+        with open(__file__, 'rb') as fp:
+            fd = fp.fileno()
+            fp.close()
+            #from IPython.core.debugger import Tracer; Tracer()() ## DEBUG ##
+
+            try:
+                select.select([fd], [], [], 0)
+            except OSError as err:
+                self.assertEqual(err.errno, errno.EBADF)
+            else:
+                self.fail("exception not raised")
+
+    def test_returned_list_identity(self):
+        # See issue #8329
+        r, w, x = select.select([], [], [], 1)
+        self.assertIsNot(r, w)
+        self.assertIsNot(r, x)
+        self.assertIsNot(w, x)
+
+    def test_select(self):
+        cmd = 'for i in 0 1 2 3 4 5 6 7 8 9; do echo testing...; sleep 1; done'
+        p = os.popen(cmd, 'r')
+        for tout in (0, 1, 2, 4, 8, 16) + (None,)*10:
+            if support.verbose:
+                print('timeout =', tout)
+            rfd, wfd, xfd = select.select([p], [], [], tout)
+            if (rfd, wfd, xfd) == ([], [], []):
+                continue
+            if (rfd, wfd, xfd) == ([p], [], []):
+                line = p.readline()
+                if support.verbose:
+                    print(repr(line))
+                if not line:
+                    if support.verbose:
+                        print('EOF')
+                    break
+                continue
+            self.fail('Unexpected return values from select():', rfd, wfd, xfd)
+        p.close()
+
+    # Issue 16230: Crash on select resized list
+    def test_select_mutated(self):
+        a = []
+        class F:
+            def fileno(self):
+                del a[-1]
+                return sys.__stdout__.fileno()
+        a[:] = [F()] * 10
+        self.assertEqual(select.select([], a, []), ([], a[:5], []))
+
+def test_main():
+    support.run_unittest(SelectTestCase)
+    support.reap_children()
+
+if __name__ == "__main__":
+    test_main()

greentest/3.5/test_select.py

+import errno
+import os
+import select
+import sys
+import unittest
+from test import support
+
+@unittest.skipIf((sys.platform[:3]=='win'),
+                 "can't easily test on this system")
+class SelectTestCase(unittest.TestCase):
+
+    class Nope:
+        pass
+
+    class Almost:
+        def fileno(self):
+            return 'fileno'
+
+    def test_error_conditions(self):
+        self.assertRaises(TypeError, select.select, 1, 2, 3)
+        self.assertRaises(TypeError, select.select, [self.Nope()], [], [])
+        self.assertRaises(TypeError, select.select, [self.Almost()], [], [])
+        self.assertRaises(TypeError, select.select, [], [], [], "not a number")
+        self.assertRaises(ValueError, select.select, [], [], [], -1)
+
+    # Issue #12367: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/155606
+    @unittest.skipIf(sys.platform.startswith('freebsd'),
+                     'skip because of a FreeBSD bug: kern/155606')
+    def test_errno(self):
+        with open(__file__, 'rb') as fp:
+            fd = fp.fileno()
+            fp.close()
+            try:
+                select.select([fd], [], [], 0)
+            except OSError as err:
+                self.assertEqual(err.errno, errno.EBADF)
+            else:
+                self.fail("exception not raised")
+
+    def test_returned_list_identity(self):
+        # See issue #8329
+        r, w, x = select.select([], [], [], 1)
+        self.assertIsNot(r, w)
+        self.assertIsNot(r, x)
+        self.assertIsNot(w, x)
+
+    def test_select(self):
+        cmd = 'for i in 0 1 2 3 4 5 6 7 8 9; do echo testing...; sleep 1; done'
+        p = os.popen(cmd, 'r')
+        for tout in (0, 1, 2, 4, 8, 16) + (None,)*10:
+            if support.verbose:
+                print('timeout =', tout)
+            rfd, wfd, xfd = select.select([p], [], [], tout)
+            if (rfd, wfd, xfd) == ([], [], []):
+                continue
+            if (rfd, wfd, xfd) == ([p], [], []):
+                line = p.readline()
+                if support.verbose:
+                    print(repr(line))
+                if not line:
+                    if support.verbose:
+                        print('EOF')
+                    break
+                continue
+            self.fail('Unexpected return values from select():', rfd, wfd, xfd)
+        p.close()
+
+    # Issue 16230: Crash on select resized list
+    def test_select_mutated(self):
+        a = []
+        class F:
+            def fileno(self):
+                del a[-1]
+                return sys.__stdout__.fileno()
+        a[:] = [F()] * 10
+        self.assertEqual(select.select([], a, []), ([], a[:5], []))
+
+def tearDownModule():
+    support.reap_children()
+
+if __name__ == "__main__":
+    unittest.main()

greentest/test__select.py

 import six
 import sys
 import os
+import errno
 from gevent import select, socket
 import greentest
+import unittest
 
 
 class TestSelect(greentest.GenericWaitTestCase):
@@ -23,20 +25,59 @@ if sys.platform != 'win32':
                 os.close(r)
                 os.close(w)
 
-    if hasattr(select, 'poll') and sys.platform != 'darwin':
+        # Issue #12367: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/155606
+        @unittest.skipIf(sys.platform.startswith('freebsd'),
+                         'skip because of a FreeBSD bug: kern/155606')
+        def test_errno(self):
+            # Backported from test_select.py in 3.4
+            with open(__file__, 'rb') as fp:
+                fd = fp.fileno()
+                fp.close()
+                try:
+                    select.select([fd], [], [], 0)
+                except OSError as err:
+                    # Python 3
+                    self.assertEqual(err.errno, errno.EBADF)
+                except select.error as err: # pylint:disable=duplicate-except
+                    # Python 2 (select.error is OSError on py3)
+                    self.assertEqual(err.args[0], errno.EBADF)
+                else:
+                    self.fail("exception not raised")
+
+
+    if hasattr(select, 'poll'):
 
         class TestPollRead(greentest.GenericWaitTestCase):
             def wait(self, timeout):
+                # On darwin, the read pipe is reported as writable
+                # immediately, for some reason. So we carefully register
+                # it only for read events (the default is read and write)
                 r, w = os.pipe()
                 try:
                     poll = select.poll()
-                    poll.register(r)
+                    poll.register(r, select.POLLIN)
                     poll.poll(timeout * 1000)
-                    poll.unregister(r)
                 finally:
+                    poll.unregister(r)
                     os.close(r)
                     os.close(w)
 
+            def test_unregister_never_registered(self):
+                # "Attempting to remove a file descriptor that was
+                # never registered causes a KeyError exception to be
+                # raised."
+                poll = select.poll()
+                self.assertRaises(KeyError, poll.unregister, 5)
+
+            def test_poll_invalid(self):
+                with open(__file__, 'rb') as fp:
+                    fd = fp.fileno()
+                    fp.close()
+
+                    poll = select.poll()
+                    poll.register(fd, select.POLLIN)
+                    result = poll.poll(0)
+                    self.assertEqual(result, [(fd, select.POLLNVAL)]) # pylint:disable=no-member
 
 class TestSelectTypes(greentest.TestCase):