New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
selectors: add keys() method #63371
Comments
This adds a keys() method to selectors, to return all the registered keys. 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: (since you'll get "dictonary changed size during iteration"). |
If you want to unregister while iterating on .keys(), just copy .keys(), as |
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:
|
Using .keys() to test if the selector is empty is surprising. To test if a Or sometimes I write if len(container): ... Selector has no length? |
Of course, but as noted by Antoine, it makes the return type dependent of an
No. For example, select.epoll objects don't have a length either. Returning all the keys is a generic method that can be useful besides checking
Sounds a bit overkill, no? |
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.
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?
"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. |
Yes, and there's nothing we can do about it :-(
I don't know, it makes me uncomfortable treating a selector like
It's more key as "indexing key", or token: it's the interface between the selector and the external world. I'd really like to have Guido's feeling regarding the above questions.
Yes, it's always defined: it's the object passed to register(). |
2013/10/6 Charles-François Natali <report@bugs.python.org>:
Oh, I just mentioned to corner case to say that it would nice to
I don't know if there is a real use case.
Oh, selector.register(0).fileobj gives me 0... I didn't know that 0 is |
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 bpo-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? |
If you don't want to rename the SelectorKey class, rename the method |
FWIW, I think the "ideal" solution would be for keys() (*) to return a (to implement Mapping, you can subclass Mapping and implement (*) or a better name |
Actually, you would use values() to iterate on SelectorKeys. |
Here is a proof-of-concept patch adding a get_map() method to Selector (and removing get_key()). |
Thanks, I think that's a great idea.
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 |
I added some comments to the code review. Please take a look. |
(Adding CF's new patch so I can compare and review it.) |
LGTM. Commit! |
New changeset b0ae96700301 by Charles-François Natali in branch 'default': |
Committed. Antoine, thanks for the idea and patch! |
+ .. method:: get_map() 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 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: