cpython icon indicating copy to clipboard operation
cpython copied to clipboard

Is a UDP transport also a ReadTransport/WriteTransport?

Open 45bf83a6-23ac-4b3a-83de-214f929d7580 opened this issue 8 years ago • 8 comments

BPO 31176
Nosy @1st1, @twisteroidambassador

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 = None
created_at = <Date 2017-08-10.13:28:01.796>
labels = ['type-feature', '3.7', 'docs', 'expert-asyncio']
title = 'Is a UDP transport also a ReadTransport/WriteTransport?'
updated_at = <Date 2017-08-10.13:28:01.796>
user = 'https://github.com/twisteroidambassador'

bugs.python.org fields:

activity = <Date 2017-08-10.13:28:01.796>
actor = 'twisteroid ambassador'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation', 'asyncio']
creation = <Date 2017-08-10.13:28:01.796>
creator = 'twisteroid ambassador'
dependencies = []
files = []
hgrepos = []
issue_num = 31176
keywords = []
message_count = 1.0
messages = ['300083']
nosy_count = 3.0
nosy_names = ['docs@python', 'yselivanov', 'twisteroid ambassador']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue31176'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

In docs / Library Reference / asyncio / Transports and Protocols, it is mentioned that "asyncio currently implements transports for TCP, UDP, SSL, and subprocess pipes. The methods available on a transport depend on the transport’s kind." It also lists methods available on a BaseTransport, ReadTransport, WriteTransport, DatagramTransport and BaseSubprocessTransport.

However, the docs does not explain which transports have methods from which base classes, or in other words which base classes each concrete transport class inherits from. And this may not be obvious: for example, a UDP transport certainly is a DatagramTransport, but is it also a ReadTransport, or a WriteTransport?

(I feel like the answer is "no it isn't", but there are plenty of conflicting evidence. The docs show that WriteTransport has write_eof() and can_write_eof() -- methods clearly geared towards stream-like transports, and it duplicates abort() from DatagramTransport, so it would seem like WriteTransport and DatagramTransport are mutually exclusive. On the other hand, the default concrete implementation of _SelectorDatagramTransport actually inherits from Transport which inherits from both ReadTransport and WriteTransport, yet it does not inherit from DatagramTransport; As a result _SelectorDatagramTransport has all the methods from ReadTransport and WriteTransport, but many of them raise NotImplemented. This is why I'm asking this question in the first place: I found that the transport object I got from create_datagram_endpoint() has both pause_reading() and resume_reading() methods that raise NotImplemented, and thought that perhaps some event loop implementations would have these methods working, and I should try to use them. And before you say "UDP doesn't do flow control", asyncio actually does provide flow control for UDP on the writing end: see https://www.mail-archive.com/[email protected]/msg00532.html So it's not preposterous that there might be flow control on the reading end as well.)

I think it would be nice if the documentation can state the methods implemented for each type of transport, as the designers of Python intended, so there's a clear expectation of what methods will / should be available across different implementations of event loops and transports. Something along the lines of "The methods available on a transport depend on the transport’s kind: TCP transports support methods declared in BaseTransport, ReadTransport and WriteTransport below, etc."

@CAM-Gerlach Do you know if this inheritance pattern can be expressed better with a sphinx extension or something or does this needs to be done manually?

kumaraditya303 avatar Oct 15 '22 14:10 kumaraditya303

We should start by collecting the actual graph of inheritance between the various transports, here in this issue.

gvanrossum avatar Oct 15 '22 16:10 gvanrossum

In terms of support for presenting this information, there's the built in e.g. sphinx.ext.inheritance_diagram. But it seems to me, as it does @gvanrossum, that the main task at present is determining the intended and/or actual inheritance pattern between these classes. Once that is in hand, we can decide how to best present it, and implement that in the docs (which I could help with if desired).

CAM-Gerlach avatar Oct 15 '22 17:10 CAM-Gerlach

In terms of support for presenting this information, there's the built in e.g. sphinx.ext.inheritance_diagram.

Thanks, I didn't know that sphinx has graphviz support.

There is one bad news, the current inheritance of datagram transports is broken (I remembered that this was fixed long ago but apparently not). See https://github.com/python/cpython/issues/90352 by @asvetlov

kumaraditya303 avatar Oct 15 '22 17:10 kumaraditya303

On Windows, the proactor datagram transport is implemented correctly and has the following inheritance pattern:

classDiagram
BaseTransport <|-- DatagramTransport
BaseTransport <|-- ReadTransport
BaseTransport <|-- WriteTransport
BaseTransport <|-- _ProactorBasePipeTransport
DatagramTransport <|-- _ProactorDatagramTransport
ReadTransport <|-- Transport
Transport <|-- _FlowControlMixin
WriteTransport <|-- Transport
_FlowControlMixin <|-- _ProactorBasePipeTransport
_ProactorBasePipeTransport <|-- _ProactorDatagramTransport
object <|-- BaseTransport

kumaraditya303 avatar Oct 15 '22 18:10 kumaraditya303

Can you post a drawing for the selector datagram transport? Also maybe we should just merge the PR that supposedly fixes the thing? (Can you review it?)

gvanrossum avatar Oct 15 '22 21:10 gvanrossum

Selector Datagram Transport Inheritance:

classDiagram
BaseTransport <|-- ReadTransport
BaseTransport <|-- WriteTransport
ReadTransport <|-- Transport
Transport <|-- _FlowControlMixin
Transport <|-- _SelectorTransport
WriteTransport <|-- Transport
_FlowControlMixin <|-- _SelectorTransport
_SelectorTransport <|-- _SelectorDatagramTransport
object <|-- BaseTransport

kumaraditya303 avatar Oct 16 '22 03:10 kumaraditya303

(Can you review it?)

The PR has merge conflicts and CI was failing at the last run so I can't review it.

kumaraditya303 avatar Oct 17 '22 14:10 kumaraditya303

In that case, can you come up with your own PR (maybe inspired by that one) that does the intended thing?

gvanrossum avatar Oct 17 '22 15:10 gvanrossum

Before doing that I have a question what inheritance do we want for _SelectorDatagramTransport? Currently it inherits from Transport which IMO is wrong since it has methods like write which raise NotImplementedError, it should only inherit from things which it actually implements.

kumaraditya303 avatar Oct 28 '22 17:10 kumaraditya303

Also it seems this inheritance issue is from the beginning as asyncio was imported from the old repo https://github.com/python/cpython/blame/b27b57c6e44a276c8a9843fd37d4cf65b2827d5c/Lib/asyncio/selector_events.py#L1129. So you would probably know why things are implemented like this rather than how I would like it to be.

kumaraditya303 avatar Oct 28 '22 17:10 kumaraditya303

Before doing that I have a question what inheritance do we want for selector DatagramTransport? Currently it inherits from Transport which IMO is wrong since it has methods like write which raise NotImplementedError, it should only inherit from things which it actually implements.

Yeah, that would be nice but we're constrained by where some of the implementation bits are. We'd have to introduce even more intermediate classes in the class hierarchy.

Maybe all we need is to add DatagramTransport to the bases of _SelectorDatagramTransport (last in the list of base classes, so its abstract abort() doesn't override the concrete implementation in _SelectorTransport).

PS. The diagram for _ProactorDatagramTransport is confusing, it shows DatagramTransport first, but it should be last (for similar reasons).

gvanrossum avatar Oct 28 '22 17:10 gvanrossum

Maybe all we need is to add DatagramTransport to the bases of _SelectorDatagramTransport (last in the list of base classes, so its abstract abort() doesn't override the concrete implementation in _SelectorTransport).

https://github.com/python/cpython/pull/98844 adds DatagramTransport to the bases of _SelectorDatagramTransport.

kumaraditya303 avatar Oct 29 '22 09:10 kumaraditya303

It seems the initial issue here was one with the documentation rather than the implementation, specifically:

the docs does not explain which transports have methods from which base classes, or in other words which base classes each concrete transport class inherits from

An issue with the implementation of the inheritance hierarchy (#90352) that was brought up along the way was fixed, but has the documentation issue here actually been addressed, i.e. by documenting what that inheritance hierarchy currently is (e.g. via an inheritance diagram)?

CAM-Gerlach avatar Oct 30 '22 19:10 CAM-Gerlach