[issue19172] selectors: add keys() method
Berker Peksag added the comment: + .. method:: get_map() + + Return a mapping of file objects to selector keys. I kind of feel like a walking linter though I think this needs a versionadded tag :) -- keywords: -needs review nosy: +berker.peksag ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Charles-François Natali added the comment: Berker Peksag added the comment: + .. method:: get_map() + + Return a mapping of file objects to selector keys. I kind of feel like a walking linter though I think this needs a versionadded tag :) Well, selectors have been added to 3.4 - which hasn't been released yet - so no versionadded is needed (there's one at the module-level). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Guido van Rossum added the comment: LGTM. Commit! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Guido van Rossum added the comment: (Adding CF's new patch so I can compare and review it.) -- Added file: http://bugs.python.org/file32428/selectors_map-2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Roundup Robot added the comment: New changeset b0ae96700301 by Charles-François Natali in branch 'default': Issue #19172: Add a get_map() method to selectors. http://hg.python.org/cpython/rev/b0ae96700301 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Charles-François Natali added the comment: Committed. Antoine, thanks for the idea and patch! -- resolution: - fixed stage: patch review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Guido van Rossum added the comment: I added some comments to the code review. Please take a look. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Charles-François Natali added the comment:
Antoine Pitrou added the comment:
FWIW, I think the ideal solution would be for keys() (*) to return a
read-only Mapping implementation, allowing for file object lookup (using
__getitem__) as well as iteration on SelectorKeys (using __iter__) and
fast emptiness checking (using __len__).
Thanks, I think that's a great idea.
I'm attaching a patch updating yours with the following:
- asyncio/test_asyncio update
- selectors' documentation update
IMO, it really offers both a compact, easy to use and performant API.
(Note: the mapping doesn't really have to be recreated upon each
get_map() call and could be kept as a private member, but IMO that's
not a performance issue).
--
Added file: http://bugs.python.org/file32360/selectors_map-1.patch
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19172
___diff -r eb1edc9e3722 Doc/library/selectors.rst
--- a/Doc/library/selectors.rst Fri Oct 25 17:56:00 2013 +0200
+++ b/Doc/library/selectors.rst Fri Oct 25 19:47:35 2013 +0200
@@ -157,12 +157,13 @@
This must be called to make sure that any underlying resource is freed.
The selector shall not be used once it has been closed.
- .. method:: get_key(fileobj)
+ .. method:: get_map()
- Return the key associated to a registered file object.
+ Return a mapping of file objects to selector keys.
- This returns the :class:`SelectorKey` instance associated to this file
- object, or raises :exc:`KeyError` if the file object is not registered.
+ This returns a :class:`~collections.abc.Mapping` instance mapping
+ registered file objects to their associated :class:`SelectorKey`
+ instance.
.. class:: DefaultSelector()
diff -r eb1edc9e3722 Lib/asyncio/selector_events.py
--- a/Lib/asyncio/selector_events.pyFri Oct 25 17:56:00 2013 +0200
+++ b/Lib/asyncio/selector_events.pyFri Oct 25 19:47:35 2013 +0200
@@ -122,7 +122,7 @@
Add a reader callback.
handle = events.make_handle(callback, args)
try:
-key = self._selector.get_key(fd)
+key = self._selector.get_map()[fd]
except KeyError:
self._selector.register(fd, selectors.EVENT_READ,
(handle, None))
@@ -136,7 +136,7 @@
def remove_reader(self, fd):
Remove a reader callback.
try:
-key = self._selector.get_key(fd)
+key = self._selector.get_map()[fd]
except KeyError:
return False
else:
@@ -157,7 +157,7 @@
Add a writer callback..
handle = events.make_handle(callback, args)
try:
-key = self._selector.get_key(fd)
+key = self._selector.get_map()[fd]
except KeyError:
self._selector.register(fd, selectors.EVENT_WRITE,
(None, handle))
@@ -171,7 +171,7 @@
def remove_writer(self, fd):
Remove a writer callback.
try:
-key = self._selector.get_key(fd)
+key = self._selector.get_map()[fd]
except KeyError:
return False
else:
diff -r eb1edc9e3722 Lib/selectors.py
--- a/Lib/selectors.py Fri Oct 25 17:56:00 2013 +0200
+++ b/Lib/selectors.py Fri Oct 25 19:47:35 2013 +0200
@@ -6,7 +6,7 @@
from abc import ABCMeta, abstractmethod
-from collections import namedtuple
+from collections import namedtuple, Mapping
import functools
import select
import sys
@@ -44,6 +44,25 @@
selected event mask and attached data.
+class _SelectorMapping(Mapping):
+Mapping of file objects to selector keys.
+
+def __init__(self, selector):
+self._fd_to_key = selector._fd_to_key
+
+def __len__(self):
+return len(self._fd_to_key)
+
+def __getitem__(self, fileobj):
+try:
+return self._fd_to_key[_fileobj_to_fd(fileobj)]
+except KeyError:
+raise KeyError({!r} is not registered.format(fileobj)) from None
+
+def __iter__(self):
+return iter(self._fd_to_key)
+
+
class BaseSelector(metaclass=ABCMeta):
Base selector class.
@@ -151,16 +170,9 @@
self._fd_to_key.clear()
-def get_key(self, fileobj):
-Return the key associated to a registered file object.
-
-Returns:
-SelectorKey for this file object
-
-try:
-return self._fd_to_key[_fileobj_to_fd(fileobj)]
-except KeyError:
-raise KeyError({!r} is not registered.format(fileobj)) from None
+def get_map(self):
+Return a mapping of file objects to selector keys.
+return _SelectorMapping(self)
def __enter__(self):
return self
diff -r eb1edc9e3722 Lib/test/test_asyncio/test_selector_events.py
--- a/Lib/test/test_asyncio/test_selector_events.py Fri Oct 25 17:56:00
2013 +0200
+++
[issue19172] selectors: add keys() method
STINNER Victor added the comment: BaseSelector.register(fd) raises a KeyError if fd is already registered, which means that any selector must know the list of all registered FDs. For EpollSelector, the list may be inconsistent *if* the epoll is object is modified externally, but it's really a corner case. while selector: ... is a nice (common?) pattern, it is used by subprocess.communicate() at least. I'm not sure whether selectors have a natural length len(self._fd_to_key) is the natural length. bool(selector) uses selector.__length__() is defined, right? What do you think of having some mapping methods? - iter(selector), selector.keys(): : iter(self._fd_to_key), iterator on file descriptor numbers (int) - selector.values(): self._fd_to_key.values(), iterate on SelectorKey objects - selector.items(): self._fd_to_key.items(), iterator on (fd, SelectorKey) tuples SelectorKey name is confusing, it's not really a key as a dictionary dict. SelectorKey.fd *is* the key, Selector.event is not. SelectorFile or SelectorItem would be more explicit, no? By the way, is SelectorKey.fileobj always defined? If not, the documentation is wrong: the attribut should be documented as Optional, as .data. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Charles-François Natali added the comment: BaseSelector.register(fd) raises a KeyError if fd is already registered, which means that any selector must know the list of all registered FDs. For EpollSelector, the list may be inconsistent *if* the epoll is object is modified externally, but it's really a corner case. Yes, and there's nothing we can do about it :-( What do you think of having some mapping methods? - iter(selector), selector.keys(): : iter(self._fd_to_key), iterator on file descriptor numbers (int) - selector.values(): self._fd_to_key.values(), iterate on SelectorKey objects - selector.items(): self._fd_to_key.items(), iterator on (fd, SelectorKey) tuples I don't know, it makes me uncomfortable treating a selector like a plain container. SelectorKey name is confusing, it's not really a key as a dictionary dict. SelectorKey.fd *is* the key, Selector.event is not. SelectorFile or SelectorItem would be more explicit, no? It's more key as indexing key, or token: it's the interface between the selector and the external world. The FD is indeed the key internally, but that's an implementation detail. I'd really like to have Guido's feeling regarding the above questions. By the way, is SelectorKey.fileobj always defined? If not, the documentation is wrong: the attribut should be documented as Optional, as .data. Yes, it's always defined: it's the object passed to register(). -- nosy: +gvanrossum ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
STINNER Victor added the comment: 2013/10/6 Charles-François Natali rep...@bugs.python.org: BaseSelector.register(fd) raises a KeyError if fd is already registered, which means that any selector must know the list of all registered FDs. For EpollSelector, the list may be inconsistent *if* the epoll is object is modified externally, but it's really a corner case. Yes, and there's nothing we can do about it :-( Oh, I just mentioned to corner case to say that it would nice to expose the length of a selector. What do you think of having some mapping methods? - iter(selector), selector.keys(): : iter(self._fd_to_key), iterator on file descriptor numbers (int) - selector.values(): self._fd_to_key.values(), iterate on SelectorKey objects - selector.items(): self._fd_to_key.items(), iterator on (fd, SelectorKey) tuples I don't know, it makes me uncomfortable treating a selector like a plain container. I don't know if there is a real use case. By the way, is SelectorKey.fileobj always defined? If not, the documentation is wrong: the attribut should be documented as Optional, as .data. Yes, it's always defined: it's the object passed to register(). Oh, selector.register(0).fileobj gives me 0... I didn't know that 0 is a file object :-) I would expect .fileobj=None and .fd=0. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Guido van Rossum added the comment: No time to follow this in detail, but one thing: please do not make the selector appear false under *any* circumstances. I've seen too many code write if foo where they meant if foo is not None and get in trouble because foo wasn't None but happened to have no content. (See e.g. recent issue 19097, although the situation there is even more complicated.) (And for things formally deriving from Container it's a different thing. But that shouldn't be done lightly.) I think it's useful to be able to get the keys; it would be nice if that was an O(1) operation so you can also check for emptiness without needing a second method. Perhaps the method shouldn't be called keys() to avoid any confusion with subclasses of the Container ABC? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
STINNER Victor added the comment: Perhaps the method shouldn't be called keys() to avoid any confusion with subclasses of the Container ABC? If you don't want to rename the SelectorKey class, rename the method to get_keys(). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Antoine Pitrou added the comment: FWIW, I think the ideal solution would be for keys() (*) to return a read-only Mapping implementation, allowing for file object lookup (using __getitem__) as well as iteration on SelectorKeys (using __iter__) and fast emptiness checking (using __len__). Actually, you would use values() to iterate on SelectorKeys. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Antoine Pitrou added the comment: FWIW, I think the ideal solution would be for keys() (*) to return a read-only Mapping implementation, allowing for file object lookup (using __getitem__) as well as iteration on SelectorKeys (using __iter__) and fast emptiness checking (using __len__). (to implement Mapping, you can subclass Mapping and implement __getitem__, __len__ and __iter__) (*) or a better name -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Antoine Pitrou added the comment: Here is a proof-of-concept patch adding a get_map() method to Selector (and removing get_key()). -- Added file: http://bugs.python.org/file31977/selectors_map.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
New submission from Charles-François Natali: This adds a keys() method to selectors, to return all the registered keys. It's useful, because one often needs to loop until all registered file objects have been unregistered e.g. (inspired from subprocess, see #18923): while selector.keys(): for key, events in selector.select(): # process events also, it can be useful if you want e.g. the list of all currently logged users, etc. It avoids having to maintain a separate data-structure tracking registered file objects. The patch attached returns a new set upon each call: another way to handle this would be to just return the dictionary's .values() view: it would be faster and use less memory, but it's immutable and also this would prevent the user from doing: for key in selectior.keys(): if somecondition: selector.unregister(key.fileobj) (since you'll get dictonary changed size during iteration). -- components: Library (Lib) files: selectors_keys.diff keywords: needs review, patch messages: 198988 nosy: haypo, neologix, pitrou priority: normal severity: normal stage: patch review status: open title: selectors: add keys() method type: enhancement versions: Python 3.4 Added file: http://bugs.python.org/file31965/selectors_keys.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
STINNER Victor added the comment: If you want to unregister while iterating on .keys(), just copy .keys(), as I do when removing items from a list or a dict while iterating on it. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Antoine Pitrou added the comment: Indeed it's easy enough to call list() or set() on the result if you need it. On the other hand, the problem with returning a dict view is that it makes the return type dependent on an implementation detail. Returning a simple iterator would be great, except that you can't as easily test it for emptiness. So perhaps two methods are needed: - a keys() method returning an iterator - a has_keys() method returning a boolean -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
STINNER Victor added the comment: Using .keys() to test if the selector is empty is surprising. To test if a str, list, tuple, dict, set, ..., is empty: if container: not empty is enough. Or sometimes I write if len(container): ... Selector has no length? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19172] selectors: add keys() method
Charles-François Natali added the comment: If you want to unregister while iterating on .keys(), just copy .keys(), as I do when removing items from a list or a dict while iterating on it. Of course, but as noted by Antoine, it makes the return type dependent of an implementation detail (I don't think there's any other place in the stdlib where a dict view is returned). Using .keys() to test if the selector is empty is surprising. To test if a str, list, tuple, dict, set, ..., is empty: if container: not empty is enough. Or sometimes I write if len(container): ... Selector has no length? No. For example, select.epoll objects don't have a length either. I'm not sure whether selectors have a natural length: if we added a __len__, we should also add __iter__ and IMO a selector isn't a container. Returning all the keys is a generic method that can be useful besides checking that the selector has registered keys. So perhaps two methods are needed: - a keys() method returning an iterator - a has_keys() method returning a boolean Sounds a bit overkill, no? keys() is definitely usful, and I'm not sure that it'll actually be a performance bottleneck in practice. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19172 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
