classification
Title: selectors: add keys() method
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, gvanrossum, neologix, pitrou, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2013-10-05 14:40 by neologix, last changed 2013-10-31 10:52 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
selectors_keys.diff neologix, 2013-10-05 14:40 review
selectors_map.patch pitrou, 2013-10-06 19:05 review
selectors_map-1.patch neologix, 2013-10-25 17:55 review
selectors_map-2.patch gvanrossum, 2013-10-30 18:42 review
Messages (21)
msg198988 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-05 14:40
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").
msg198990 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-05 16:27
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.
msg198994 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-05 17:50
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
msg198999 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-05 19:42
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?
msg199013 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-05 22:10
> 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.
msg199036 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-06 07:19
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.
msg199059 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-06 09:37
> 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().
msg199075 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-06 14:28
2013/10/6 Charles-François Natali <report@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.
msg199076 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-10-06 14:48
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?
msg199079 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-06 14:59
> 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().
msg199080 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-06 15:01
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
msg199081 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-06 15:02
> 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.
msg199110 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-06 19:05
Here is a proof-of-concept patch adding a get_map() method to Selector (and removing get_key()).
msg201268 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-25 17:55
> 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).
msg201701 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-10-30 00:07
I added some comments to the code review. Please take a look.
msg201758 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-10-30 18:42
(Adding CF's new patch so I can compare and review it.)
msg201761 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-10-30 18:48
LGTM. Commit!
msg201771 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-10-30 19:31
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
msg201776 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-30 19:45
Committed.

Antoine, thanks for the idea and patch!
msg201797 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2013-10-31 10:31
+   .. 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 :)
msg201799 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-10-31 10:52
> 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).
History
Date User Action Args
2013-10-31 10:52:37neologixsetmessages: + msg201799
2013-10-31 10:31:10berker.peksagsetkeywords: - needs review
nosy: + berker.peksag
messages: + msg201797

2013-10-30 19:45:59neologixsetstatus: open -> closed
resolution: fixed
messages: + msg201776

stage: patch review -> resolved
2013-10-30 19:31:53python-devsetnosy: + python-dev
messages: + msg201771
2013-10-30 18:48:10gvanrossumsetmessages: + msg201761
2013-10-30 18:42:26gvanrossumsetfiles: + selectors_map-2.patch

messages: + msg201758
2013-10-30 00:07:53gvanrossumsetmessages: + msg201701
2013-10-25 17:55:04neologixsetfiles: + selectors_map-1.patch

messages: + msg201268
2013-10-06 19:05:15pitrousetfiles: + selectors_map.patch

messages: + msg199110
2013-10-06 15:02:03pitrousetmessages: + msg199081
2013-10-06 15:01:11pitrousetmessages: + msg199080
2013-10-06 14:59:56vstinnersetmessages: + msg199079
2013-10-06 14:48:22gvanrossumsetmessages: + msg199076
2013-10-06 14:28:09vstinnersetmessages: + msg199075
2013-10-06 09:37:25neologixsetnosy: + gvanrossum
messages: + msg199059
2013-10-06 07:19:19vstinnersetmessages: + msg199036
2013-10-05 22:10:50neologixsetmessages: + msg199013
2013-10-05 19:42:05vstinnersetmessages: + msg198999
2013-10-05 17:50:41pitrousetmessages: + msg198994
2013-10-05 16:27:19vstinnersetmessages: + msg198990
2013-10-05 14:42:42neologixlinkissue18923 dependencies
2013-10-05 14:40:38neologixcreate