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

Bad docs, or bad types for asyncio.WriteTransport? #5779

Closed
jnsnow opened this issue Jul 15, 2021 · 5 comments · Fixed by #7718
Closed

Bad docs, or bad types for asyncio.WriteTransport? #5779

jnsnow opened this issue Jul 15, 2021 · 5 comments · Fixed by #7718
Labels
topic: asyncio Asyncio-related issues

Comments

@jnsnow
Copy link
Contributor

jnsnow commented Jul 15, 2021

I have to admit I am not fully certain of this one, there's a few layers of abstraction to sift through.

Typeshed declares this:

class _FlowControlMixin(Transport):
    def __init__(self, extra: Optional[Mapping[Any, Any]] = ..., loop: Optional[AbstractEventLoop] = ...) -> None: ...
    def get_write_buffer_limits(self) -> Tuple[int, int]: ...

Which does seem true: CPython does indeed define get_write_buffer_limits() as part of that _FlowControlMixin class. Where I lose the plot, however, is the fact that https://docs.python.org/3/library/asyncio-protocol.html#write-only-transports seems to imply that all asyncio.WriteTransport types will offer this method. In practice, I suppose that isn't strictly true, since the CPython implementation declares the class like this:

class WriteTransport(BaseTransport):
    ...

And does not in fact guarantee the presence of this method. It turns out that the actual type of the transport beneath the StreamWriter that I have is ... asyncio.selector_events._SelectorSocketTransport, which inherits from _SelectorTransport, which in turn inherits from transports._FlowControlMixin and transports.Transport, the latter of which inherits from both ReadTransport and WriteTransport.

Phew, OK.

I suppose this means the docs are "wrong" in the sense that WriteTransport does not necessarily guarantee the presence of this method, but "in practice" all implementations of WriteTransport use the _FlowControlMixin that offers it.

Does the WriteTransport class just need a NotImplementedError stub for get_write_buffer_limits() ...? or is it intentional that this method isn't required and the docs are wrong?

@srittau
Copy link
Collaborator

srittau commented Aug 19, 2021

I am sorry for the late response. I am also unsure what to do here. Do you have any real world examples, where this causes problems? If that's the case, I suggest, we add get_write_buffer_limits() to WriteTransport in typeshed, with a comment that this differs from runtime. And maybe suggest to add the method to the implementation on bugs.python.org?

@jnsnow
Copy link
Contributor Author

jnsnow commented Aug 21, 2021

(Please, no apology is necessary. Thank you for your open source contributions!)

The real-world case it came up for is when I was trying to write a function to change the buffer limits -- itself something I was trying to do in order to coerce flush into guaranteeing that I had drained the entirety of the write buffer -- but, let's not worry about that.

async def flush(writer: asyncio.StreamWriter) -> None:
    transport = cast(asyncio.WriteTransport, writer.transport)
                                                                      
    low, high = transport.get_write_buffer_limits()  # <-- this line causes a problem                            
    transport.set_write_buffer_limits(0, 0)
    try:
        await writer.drain()
    finally:
        transport.set_write_buffer_limits(high, low)

I have to cast in the first place because writer.transport is known to mypy as asyncio.transports.BaseTransport. In practice, I believe in my heart that the actual type will always be isinstance(..., asyncio.WriteTransport)

Even with the cast, though, we still don't gain problem-free access to get_write_buffer_limits, and I can't downcast any more specifically because I don't actually necessarily know what the backing implementation is.

Maybe the right thing here really is a python bugfix to make the method abstract in the type that suggests it, then change typeshed to match.

@srittau
Copy link
Collaborator

srittau commented Aug 21, 2021

FWIW, I'd accept a PR to typeshed as outlined above. This method is part of the protocol, even if it's not "physically" present in the implementation.

@AlexWaygood AlexWaygood added the topic: asyncio Asyncio-related issues label Apr 9, 2022
@Akuli
Copy link
Collaborator

Akuli commented Apr 27, 2022

I have to cast in the first place because writer.transport is known to mypy as asyncio.transports.BaseTransport.

I fixed this in #7611, but the get_write_buffer_limits() problem is still not fixed. I currently use get_write_buffer_size() like this, and it has the same problem:

class Client:
    def __init__(self, ..., writer: asyncio.StreamWriter) -> None:
        ...
        self._writer = writer
        ...

    ...

    def _send_bytes(self, b: bytes) -> None:
        ...
        if self._writer.transport.get_write_buffer_size() > 4 * 1024:  # type: ignore
            ...

I agree we should just add get_write_buffer_limits() and get_write_buffer_size() to WriteTransport.

@Akuli
Copy link
Collaborator

Akuli commented Apr 27, 2022

Turns out I was confused. It's already fixed, but only if you target Python 3.9 or newer.

The following program produces the same output consistently on Python 3.6 and all newer versions, when you connect to localhost port 12345 while it's running:

import asyncio
import sys

async def handle_connection(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None:
    print(writer.transport.get_write_buffer_size())
    print(writer.transport.get_write_buffer_limits())
    writer.transport.set_write_buffer_limits(0, 0)


if sys.version_info >= (3, 7):
    async def main() -> None:
        async with await asyncio.start_server(handle_connection, port=12345) as server:
            await server.serve_forever()

    asyncio.run(main())
else:
    async def main() -> None:
        server = await asyncio.start_server(handle_connection, port=12345)
        await server.wait_closed()
    asyncio.get_event_loop().run_until_complete(main())

It only type-checks with --python-version 3.9 or newer though:

class WriteTransport(BaseTransport):
def set_write_buffer_limits(self, high: int | None = ..., low: int | None = ...) -> None: ...
def get_write_buffer_size(self) -> int: ...
if sys.version_info >= (3, 9):
def get_write_buffer_limits(self) -> tuple[int, int]: ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: asyncio Asyncio-related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants