[Python-checkins] cpython: Silently ignore unregistering closed files. Fixes issue 19876. With docs and

guido.van.rossum python-checkins at python.org
Sun Dec 8 00:57:15 CET 2013

• Previous message: [Python-checkins] cpython: Issue #19922: define _INCLUDE__STDC_A1_SOURCE in HP-UX to include mbstate_t
• Next message: [Python-checkins] cpython: News item for issue 19876.
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

---

http://hg.python.org/cpython/rev/39e7995f9ad1
changeset: 87816:39e7995f9ad1
user: Guido van Rossum <guido at python.org>
date: Sat Dec 07 15:57:01 2013 -0800
summary:
 Silently ignore unregistering closed files. Fixes issue 19876. With docs and slight test refactor.
files:
 Doc/library/selectors.rst | 7 +-
 Lib/selectors.py | 74 +++++++++++++++++--
 Lib/test/test_selectors.py | 96 +++++++++++++++----------
 3 files changed, 129 insertions(+), 48 deletions(-)
diff --git a/Doc/library/selectors.rst b/Doc/library/selectors.rst
--- a/Doc/library/selectors.rst
+++ b/Doc/library/selectors.rst
@@ -102,7 +102,8 @@
 
 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 @@
 *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
--- a/Lib/selectors.py
+++ b/Lib/selectors.py
@@ -25,6 +25,9 @@
 
 Returns:
 corresponding file descriptor
+
+ Raises:
+ ValueError if the object is invalid
 """
 if isinstance(fileobj, int):
 fd = fileobj
@@ -55,7 +58,8 @@
 
 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 @@
 
 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 @@
 
 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 @@
 
 Returns:
 SelectorKey instance
+
+ Raises:
+ Anything that unregister() or register() raises
 """
 self.unregister(fileobj)
 return self.register(fileobj, events, data)
@@ -177,22 +200,41 @@
 # 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 @@
 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 @@
 
 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 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
--- 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 @@
 
 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 @@
 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 @@
 # 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 @@
 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 @@
 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 @@
 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 @@
 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 @@
 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 @@
 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 @@
 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 @@
 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 @@
 
 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)
-- 
Repository URL: http://hg.python.org/cpython

---

• Previous message: [Python-checkins] cpython: Issue #19922: define _INCLUDE__STDC_A1_SOURCE in HP-UX to include mbstate_t
• Next message: [Python-checkins] cpython: News item for issue 19876.
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

---

More information about the Python-checkins mailing list