Skip to content
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

Closed
neologix mannequin opened this issue Oct 5, 2013 · 21 comments
Closed

selectors: add keys() method #63371

neologix mannequin opened this issue Oct 5, 2013 · 21 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@neologix
Copy link
Mannequin

neologix mannequin commented Oct 5, 2013

BPO 19172
Nosy @gvanrossum, @pitrou, @vstinner, @berkerpeksag
Files
  • selectors_keys.diff
  • selectors_map.patch
  • selectors_map-1.patch
  • selectors_map-2.patch
  • 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:

    assignee = None
    closed_at = <Date 2013-10-30.19:45:59.326>
    created_at = <Date 2013-10-05.14:40:38.316>
    labels = ['type-feature', 'library']
    title = 'selectors: add keys() method'
    updated_at = <Date 2013-10-31.10:52:37.681>
    user = 'https://bugs.python.org/neologix'

    bugs.python.org fields:

    activity = <Date 2013-10-31.10:52:37.681>
    actor = 'neologix'
    assignee = 'none'
    closed = True
    closed_date = <Date 2013-10-30.19:45:59.326>
    closer = 'neologix'
    components = ['Library (Lib)']
    creation = <Date 2013-10-05.14:40:38.316>
    creator = 'neologix'
    dependencies = []
    files = ['31965', '31977', '32360', '32428']
    hgrepos = []
    issue_num = 19172
    keywords = ['patch']
    message_count = 21.0
    messages = ['198988', '198990', '198994', '198999', '199013', '199036', '199059', '199075', '199076', '199079', '199080', '199081', '199110', '201268', '201701', '201758', '201761', '201771', '201776', '201797', '201799']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'pitrou', 'vstinner', 'neologix', 'python-dev', 'berker.peksag']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue19172'
    versions = ['Python 3.4']

    @neologix
    Copy link
    Mannequin Author

    neologix mannequin commented Oct 5, 2013

    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 bpo-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").

    @neologix neologix mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Oct 5, 2013
    @vstinner
    Copy link
    Member

    vstinner commented Oct 5, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 5, 2013

    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

    @vstinner
    Copy link
    Member

    vstinner commented Oct 5, 2013

    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?

    @neologix
    Copy link
    Mannequin Author

    neologix mannequin commented Oct 5, 2013

    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.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 6, 2013

    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.

    @neologix
    Copy link
    Mannequin Author

    neologix mannequin commented Oct 6, 2013

    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().

    @vstinner
    Copy link
    Member

    vstinner commented Oct 6, 2013

    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.

    @gvanrossum
    Copy link
    Member

    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?

    @vstinner
    Copy link
    Member

    vstinner commented Oct 6, 2013

    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().

    @pitrou
    Copy link
    Member

    pitrou commented Oct 6, 2013

    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

    @pitrou
    Copy link
    Member

    pitrou commented Oct 6, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 6, 2013

    Here is a proof-of-concept patch adding a get_map() method to Selector (and removing get_key()).

    @neologix
    Copy link
    Mannequin Author

    neologix mannequin commented Oct 25, 2013

    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).

    @gvanrossum
    Copy link
    Member

    I added some comments to the code review. Please take a look.

    @gvanrossum
    Copy link
    Member

    (Adding CF's new patch so I can compare and review it.)

    @gvanrossum
    Copy link
    Member

    LGTM. Commit!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 30, 2013

    New changeset b0ae96700301 by Charles-François Natali in branch 'default':
    Issue bpo-19172: Add a get_map() method to selectors.
    http://hg.python.org/cpython/rev/b0ae96700301

    @neologix
    Copy link
    Mannequin Author

    neologix mannequin commented Oct 30, 2013

    Committed.

    Antoine, thanks for the idea and patch!

    @neologix neologix mannequin closed this as completed Oct 30, 2013
    @berkerpeksag
    Copy link
    Member

    + .. 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 :)

    @neologix
    Copy link
    Mannequin Author

    neologix mannequin commented Oct 31, 2013

    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).

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants