Silently ignore unregistering closed files. Fixes issue 19876. With docs and slight test refactor.

3 files changed, 129 insertions, 48 deletions

diff --git a/Doc/library/selectors.rst b/Doc/library/selectors.rst
index b0b002f..98377c8 100644
--- a/Doc/library/selectors.rst
+++ b/Doc/library/selectors.rst
@@ -102,7 +102,8 @@ below:
 
       Register a file object for selection, monitoring it for I/O events.
 
-      *fileobj* is the file object to monitor.
+      *fileobj* is the file object to monitor.  It may either be an integer
+      file descriptor or an object with a ``fileno()`` method.
       *events* is a bitwise mask of events to monitor.
       *data* is an opaque object.
 
@@ -118,7 +119,9 @@ below:
       *fileobj* must be a file object previously registered.
 
       This returns the associated :class:`SelectorKey` instance, or raises a
-      :exc:`KeyError` if the file object is not registered.
+      :exc:`KeyError` if *fileobj* is not registered.  It will raise
+      :exc:`ValueError` if *fileobj* is invalid (e.g. it has no ``fileno()``
+      method or its ``fileno()`` method has an invalid return value).
 
    .. method:: modify(fileobj, events, data=None)
 
diff --git a/Lib/selectors.py b/Lib/selectors.py
index c533f13..a44d5e9 100644
--- a/Lib/selectors.py
+++ b/Lib/selectors.py
@@ -25,6 +25,9 @@ def _fileobj_to_fd(fileobj):
 
     Returns:
     corresponding file descriptor
+
+    Raises:
+    ValueError if the object is invalid
     """
     if isinstance(fileobj, int):
         fd = fileobj
@@ -55,7 +58,8 @@ class _SelectorMapping(Mapping):
 
     def __getitem__(self, fileobj):
         try:
-            return self._selector._fd_to_key[_fileobj_to_fd(fileobj)]
+            fd = self._selector._fileobj_lookup(fileobj)
+            return self._selector._fd_to_key[fd]
         except KeyError:
             raise KeyError("{!r} is not registered".format(fileobj)) from None
 
@@ -89,6 +93,15 @@ class BaseSelector(metaclass=ABCMeta):
 
         Returns:
         SelectorKey instance
+
+        Raises:
+        ValueError if events is invalid
+        KeyError if fileobj is already registered
+        OSError if fileobj is closed or otherwise is unacceptable to
+                the underlying system call (if a system call is made)
+
+        Note:
+        OSError may or may not be raised
         """
         raise NotImplementedError
 
@@ -101,6 +114,13 @@ class BaseSelector(metaclass=ABCMeta):
 
         Returns:
         SelectorKey instance
+
+        Raises:
+        KeyError if fileobj is not registered
+
+        Note:
+        If fileobj is registered but has since been closed this does
+        *not* raise OSError (even if the wrapped syscall does)
         """
         raise NotImplementedError
 
@@ -114,6 +134,9 @@ class BaseSelector(metaclass=ABCMeta):
 
         Returns:
         SelectorKey instance
+
+        Raises:
+        Anything that unregister() or register() raises
         """
         self.unregister(fileobj)
         return self.register(fileobj, events, data)
@@ -177,22 +200,41 @@ class _BaseSelectorImpl(BaseSelector):
         # read-only mapping returned by get_map()
         self._map = _SelectorMapping(self)
 
+    def _fileobj_lookup(self, fileobj):
+        """Return a file descriptor from a file object.
+
+        This wraps _fileobj_to_fd() to do an exhaustive search in case
+        the object is invalid but we still have it in our map.  This
+        is used by unregister() so we can unregister an object that
+        was previously registered even if it is closed.  It is also
+        used by _SelectorMapping.
+        """
+        try:
+            return _fileobj_to_fd(fileobj)
+        except ValueError:
+            # Do an exhaustive search.
+            for key in self._fd_to_key.values():
+                if key.fileobj is fileobj:
+                    return key.fd
+            # Raise ValueError after all.
+            raise
+
     def register(self, fileobj, events, data=None):
         if (not events) or (events & ~(EVENT_READ | EVENT_WRITE)):
             raise ValueError("Invalid events: {!r}".format(events))
 
-        key = SelectorKey(fileobj, _fileobj_to_fd(fileobj), events, data)
+        key = SelectorKey(fileobj, self._fileobj_lookup(fileobj), events, data)
 
         if key.fd in self._fd_to_key:
-            raise KeyError("{!r} (FD {}) is already "
-                           "registered".format(fileobj, key.fd))
+            raise KeyError("{!r} (FD {}) is already registered"
+                           .format(fileobj, key.fd))
 
         self._fd_to_key[key.fd] = key
         return key
 
     def unregister(self, fileobj):
         try:
-            key = self._fd_to_key.pop(_fileobj_to_fd(fileobj))
+            key = self._fd_to_key.pop(self._fileobj_lookup(fileobj))
         except KeyError:
             raise KeyError("{!r} is not registered".format(fileobj)) from None
         return key
@@ -200,7 +242,7 @@ class _BaseSelectorImpl(BaseSelector):
     def modify(self, fileobj, events, data=None):
         # TODO: Subclasses can probably optimize this even further.
         try:
-            key = self._fd_to_key[_fileobj_to_fd(fileobj)]
+            key = self._fd_to_key[self._fileobj_lookup(fileobj)]
         except KeyError:
             raise KeyError("{!r} is not registered".format(fileobj)) from None
         if events != key.events:
@@ -352,7 +394,12 @@ if hasattr(select, 'epoll'):
 
         def unregister(self, fileobj):
             key = super().unregister(fileobj)
-            self._epoll.unregister(key.fd)
+            try:
+                self._epoll.unregister(key.fd)
+            except OSError:
+                # This can happen if the FD was closed since it
+                # was registered.
+                pass
             return key
 
         def select(self, timeout=None):
@@ -409,11 +456,20 @@ if hasattr(select, 'kqueue'):
             if key.events & EVENT_READ:
                 kev = select.kevent(key.fd, select.KQ_FILTER_READ,
                                     select.KQ_EV_DELETE)
-                self._kqueue.control([kev], 0, 0)
+                try:
+                    self._kqueue.control([kev], 0, 0)
+                except OSError:
+                    # This can happen if the FD was closed since it
+                    # was registered.
+                    pass
             if key.events & EVENT_WRITE:
                 kev = select.kevent(key.fd, select.KQ_FILTER_WRITE,
                                     select.KQ_EV_DELETE)
-                self._kqueue.control([kev], 0, 0)
+                try:
+                    self._kqueue.control([kev], 0, 0)
+                except OSError:
+                    # See comment above.
+                    pass
             return key
 
         def select(self, timeout=None):
diff --git a/Lib/test/test_selectors.py b/Lib/test/test_selectors.py
index f5e67b1..c8c16d5 100644
--- a/Lib/test/test_selectors.py
+++ b/Lib/test/test_selectors.py
@@ -1,4 +1,5 @@
 import errno
+import os
 import random
 import selectors
 import signal
@@ -49,13 +50,17 @@ def find_ready_matching(ready, flag):
 
 class BaseSelectorTestCase(unittest.TestCase):
 
+    def make_socketpair(self):
+        rd, wr = socketpair()
+        self.addCleanup(rd.close)
+        self.addCleanup(wr.close)
+        return rd, wr
+
     def test_register(self):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         key = s.register(rd, selectors.EVENT_READ, "data")
         self.assertIsInstance(key, selectors.SelectorKey)
@@ -81,9 +86,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         s.register(rd, selectors.EVENT_READ)
         s.unregister(rd)
@@ -94,13 +97,51 @@ class BaseSelectorTestCase(unittest.TestCase):
         # unregister twice
         self.assertRaises(KeyError, s.unregister, rd)
 
+    def test_unregister_after_fd_close(self):
+        s = self.SELECTOR()
+        self.addCleanup(s.close)
+        rd, wr = self.make_socketpair()
+        r, w = rd.fileno(), wr.fileno()
+        s.register(r, selectors.EVENT_READ)
+        s.register(w, selectors.EVENT_WRITE)
+        rd.close()
+        wr.close()
+        s.unregister(r)
+        s.unregister(w)
+
+    def test_unregister_after_fd_close_and_reuse(self):
+        s = self.SELECTOR()
+        self.addCleanup(s.close)
+        rd, wr = self.make_socketpair()
+        r, w = rd.fileno(), wr.fileno()
+        s.register(r, selectors.EVENT_READ)
+        s.register(w, selectors.EVENT_WRITE)
+        rd2, wr2 = self.make_socketpair()
+        rd.close()
+        wr.close()
+        os.dup2(rd2.fileno(), r)
+        os.dup2(wr2.fileno(), w)
+        self.addCleanup(os.close, r)
+        self.addCleanup(os.close, w)
+        s.unregister(r)
+        s.unregister(w)
+
+    def test_unregister_after_socket_close(self):
+        s = self.SELECTOR()
+        self.addCleanup(s.close)
+        rd, wr = self.make_socketpair()
+        s.register(rd, selectors.EVENT_READ)
+        s.register(wr, selectors.EVENT_WRITE)
+        rd.close()
+        wr.close()
+        s.unregister(rd)
+        s.unregister(wr)
+
     def test_modify(self):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         key = s.register(rd, selectors.EVENT_READ)
 
@@ -138,9 +179,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         s.register(rd, selectors.EVENT_READ)
         s.register(wr, selectors.EVENT_WRITE)
@@ -153,9 +192,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         key = s.register(rd, selectors.EVENT_READ, "data")
         self.assertEqual(key, s.get_key(rd))
@@ -167,9 +204,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         keys = s.get_map()
         self.assertFalse(keys)
@@ -194,9 +229,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         s.register(rd, selectors.EVENT_READ)
         wr_key = s.register(wr, selectors.EVENT_WRITE)
@@ -214,9 +247,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         with s as sel:
             sel.register(rd, selectors.EVENT_READ)
@@ -247,9 +278,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         w2r = {}
 
         for i in range(NUM_SOCKETS):
-            rd, wr = socketpair()
-            self.addCleanup(rd.close)
-            self.addCleanup(wr.close)
+            rd, wr = self.make_socketpair()
             s.register(rd, selectors.EVENT_READ)
             s.register(wr, selectors.EVENT_WRITE)
             readers.append(rd)
@@ -293,9 +322,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         s.register(wr, selectors.EVENT_WRITE)
         t = time()
@@ -322,9 +349,7 @@ class BaseSelectorTestCase(unittest.TestCase):
         s = self.SELECTOR()
         self.addCleanup(s.close)
 
-        rd, wr = socketpair()
-        self.addCleanup(rd.close)
-        self.addCleanup(wr.close)
+        rd, wr = self.make_socketpair()
 
         orig_alrm_handler = signal.signal(signal.SIGALRM, lambda *args: None)
         self.addCleanup(signal.signal, signal.SIGALRM, orig_alrm_handler)
@@ -364,16 +389,13 @@ class ScalableSelectorMixIn:
 
         for i in range(NUM_FDS // 2):
             try:
-                rd, wr = socketpair()
+                rd, wr = self.make_socketpair()
             except OSError:
                 # too many FDs, skip - note that we should only catch EMFILE
                 # here, but apparently *BSD and Solaris can fail upon connect()
                 # or bind() with EADDRNOTAVAIL, so let's be safe
                 self.skipTest("FD limit reached")
 
-            self.addCleanup(rd.close)
-            self.addCleanup(wr.close)
-
             try:
                 s.register(rd, selectors.EVENT_READ)
                 s.register(wr, selectors.EVENT_WRITE)