Skip to content

_HasFileno in platform/asyncio.py must be hashable because instances are used as dictionary key#3622

Open
jonathandung wants to merge 3 commits into
tornadoweb:masterfrom
jonathandung:patch-1
Open

_HasFileno in platform/asyncio.py must be hashable because instances are used as dictionary key#3622
jonathandung wants to merge 3 commits into
tornadoweb:masterfrom
jonathandung:patch-1

Conversation

@jonathandung
Copy link
Copy Markdown

@bdarnell
Copy link
Copy Markdown
Member

Thanks! Looks good to me aside from the fact that the linter requires a blank line between the method definitions.

@bdarnell
Copy link
Copy Markdown
Member

Hmm, so this change can only be made cleanly (without ignoring the override violation) if it's made in lockstep with an update to typeshed? That's unfortunate. Is there some way to sequence the changes so they can be applied in order? (For example, change the typing of asyncio.AbstractEventLoop to require __hash__, then update event loop subclasses like this one, and only then update dict to require __hash__). Or does that just stretch things out over a very long time frame without any concrete benefit?

Is there some way for me to reference the type used by AbstractEventLoop.add_reader instead of redefining the protocol here?

I don't mind the type:ignore here too much, but I wouldn't want to put something like that in for a typeshed change that may not merge for a long time (if at all - I'm sympathetic to the idea that hashability is too weird to be worth modeling in the static type layer since object is hashable). Can we wait on this until the fate of the typeshed PR is more certain?

@jonathandung
Copy link
Copy Markdown
Author

The add_reader and add_writer methods on asyncio.AbstractEventLoop do not require the argument to be hashable. I just verified that my typeshed PR does not change this behaviour, so the type: ignores are necessary given the implementation.

Reading through the CPython source, it seems that the add_reader method of asyncio.BaseSelectorEventLoop relies on the abstraction provided by the get_map method of the selector, which famously advertises itself as returning a mapping in the stubs, but calls a conversion function internally so that the 'keys' of the mapping returned need not be hashable. This is a standard library example that would not satisfy the interface if asyncio.AbstractEventLoop typed add_reader as taking hashable file descriptor-like objects only, so that would not go through.

@jonathandung
Copy link
Copy Markdown
Author

If the # type: ignore's were to be completely eliminated, it would require a slightly bigger refactor in which ThreadSelector called something like this function before passing fd through:

def _fileobj_to_fd(fileobj):
    """Return a file descriptor from a file object.

    Parameters:
    fileobj -- file object or file descriptor

    Returns:
    corresponding file descriptor

    Raises:
    ValueError if the object is invalid
    """
    if isinstance(fileobj, int):
        fd = fileobj
    else:
        try:
            fd = int(fileobj.fileno())
        except (AttributeError, TypeError, ValueError):
            raise ValueError("Invalid file object: "
                             "{!r}".format(fileobj)) from None
    if fd < 0:
        raise ValueError("Invalid file descriptor: {}".format(fd))
    return fd

Taken from Lib/selectors.py on the main branch. I'm not completely sure if this is correct though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants