bpo-36889: Document Stream class and add docstrings #14488 8000

tirkarthi · 2019-06-30T17:36:21Z

This just copies the docs from StreamWriter and StreamReader.
Add docstring for asyncio functions.

https://bugs.python.org/issue36889

Automerge-Triggered-By: @asvetlov

Lib/asyncio/streams.py

Doc/library/asyncio-stream.rst

1st1 · 2019-09-09T10:13:06Z

Doc/library/asyncio-stream.rst

+   .. coroutinemethod:: start_tls(sslcontext, *, server_hostname=None, \
+                                  ssl_handshake_timeout=None)
+
+      Calls :meth:`Stream.drain()` and rest of the arguments are passed directly to


I'd actually elaborate a bit on this method - why we have it, ideally, with an example.

I have added a sentence over upgrading the TLS. I have also added similar sentence around sendfile above. I couldn't come up with a useful example than calling connect and then calling start_tls to upgrade SSL in next statement. If there is a more practical one please add in.

Lib/asyncio/streams.py

bedevere-bot · 2019-09-09T10:14:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Doc/library/asyncio-api-index.rst

Doc/library/asyncio-stream.rst

1st1 · 2019-09-10T13:26:19Z

Doc/library/asyncio-stream.rst

+      the high watermark, *drain()* blocks until the size of the
+      buffer is drained down to the low watermark and writing can
+      be resumed.  When there is nothing to wait for, the :meth:`drain`
+      returns immediately.


This is a bit more subtle. drain() is only needed when you do a few stream.write() calls. When you have only one stream.write() you can just say await stream.write() and not bother with drain(). @asvetlov am I right?

From my perspective, people always should use await stream.write() form and never drain() explicitly. There is no reason for late buffer overrun detection (and draining) if it can be done earlier.

The only reason why the method exists is backward compatibility for subprocess stdin/stdout/stderr pipes usage.

I think we should reflect it in docs but sorry, I cannot propose exact wording.

Doc/library/asyncio-stream.rst

asvetlov · 2019-09-10T19:08:07Z

Doc/library/asyncio-stream.rst

+   .. coroutinemethod:: start_tls(sslcontext, *, server_hostname=None, \
+                                  ssl_handshake_timeout=None)
+
+      Calls :meth:`Stream.drain()` to write to the connection and uses :meth:`loop.start_tls`


The same as for sendfile: the method upgrades a connection to TLS encrypted one.

Doc/library/asyncio-stream.rst

…hod and a coroutine.

tirkarthi · 2019-09-11T10:40:53Z

I have pushed the changes to make sure await stream.write(data) as recommended with stream.write(data); await stream.drain() also documented with a method. I think the only detail pending now is over how multiple stream.write can be buffered to use stream.drain later as below that should be noted in the stream.drain docs.

#14488 (comment)

This is a bit more subtle. drain() is only needed when you do a few stream.write() calls. When you have only one stream.write() you can just say await stream.write() and not bother with drain(). @asvetlov am I right?

asvetlov · 2019-09-11T10:52:36Z

I think the only detail pending now is over how multiple stream.write can be buffered to use stream.drain later as below that should be noted in the stream.drain docs.

The interesting part is that multiple sync writes and single drain after doesn't help with buffering. Instead, it may consume more memory then expected.

asyncio streams already have low and high watermarks for write buffer size control. If kernel write buffer is full the data is collected in asyncio internal write buffer. If the internal buffer size is greater than high watermark the writing is paused until the size of the buffer doesn't fall to less than the low watermark. After that write is resumed and keeps resumed until the high watermark is reached again.

So, data is effectively buffered even if await stream.write() notation is always used, drain() after multiple sync write() calls doesn't make any value.

I doubt if we need to document it here, in a method reference. Maybe a separate asyncio section about buffering makes sense?
The section is not related to streams itself, low-level protocol.pause_writing() / protocol.resume_writing() supports the same logic.

1st1 · 2019-09-11T13:27:43Z

Andrew, reading all of this makes me think about soft-deprecating non-await write() and drain() in the documentation (it will probably take us many releases before we can actually deprecate them, if ever). This will actually encourage people to just write await stream.write() and not touch drain() etc.

asvetlov · 2019-09-11T13:47:57Z

Andrew, reading all of this makes me think about soft-deprecating non-await write() and drain() in the documentation (it will probably take us many releases before we can actually deprecate them, if ever). This will actually encourage people to just write await stream.write() and not touch drain() etc.

@1st1 I told you at US PyCon that non-awaitable methods are useless and should be deprecated eventually. You had doubts, so we decided to support both.

I totally agree with the deprecation strategy.

Does soft-deprecation means ..deprecated: section or completely removal these API from docs?
The second is more aggressive, sure. But it simplifies documentation writing and (much more important) reading a lot.

@1st1 what do you prefer?

1st1 · 2019-09-11T13:58:57Z

Does soft-deprecation means ..deprecated: section or completely removal these API from docs?
The second is more aggressive, sure. But it simplifies documentation writing and (much more important) reading a lot.

I'd do it in a very soft way in 3.8/3.9; basically we just warn users that they are deprecated in the docs and that's it.

asvetlov · 2019-09-11T14:03:02Z

Ok, I agree with any decision.
@tirkarthi please add .. deprecated:: sections.

Sorry for the endless list of notes. Writing a new stream API took me weeks, documenting it takes a long as well.

tirkarthi · 2019-09-13T09:31:22Z

Added deprecated notes to Stream.drain and method Stream.write to recommending usage of await Stream.write as a coroutine to be awaited. There is also StreamWriter.write and StreamWriter.writelines but they are already recommended to use stream.write and contain examples on using it in 3.7 where await is not supported so I have not added deprecations there.

No problem, I got to know better about the API while documentation and also closed some old and outdated issues related to Streams on the tracker with the PR. Appreciate your help on word formation and care to perfect user facing documentation.

asvetlov

I like the current text, looks good.
The last nitpick: please provide an order to Stream methods.
I think alphabetical sorting is better than the current grouping.

tirkarthi · 2019-09-13T10:34:57Z

Good, done. The current order is alphabetic sort as below with attribute mode at first

mode

abort
at_eof
can_write_eof
close
drain
get_extra_info
is_closing
read
readexactly
readline
readuntil
sendfile
start_tls
wait_closed
write coroutine
write method
writelines
write_eof

``

asvetlov

Well done!
Thank you very much!

asvetlov · 2019-09-13T10:52:03Z

@1st1 please review again. The PR is ready for merging from my perspective

miss-islington · 2019-09-13T10:52:37Z

@tirkarthi: Status check is done, and it's a success ✅ .

miss-islington · 2019-09-13T10:52:41Z

Thanks @tirkarthi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2019-09-13T10:52:42Z

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

tirkarthi · 2019-09-13T10:56:29Z

Thank you very much Andrew and Yury. I can create a separate PR if there are additional improvements.

asvetlov · 2019-09-13T11:02:48Z

Oops. Sorry, @1st1
I've added automerge label with the hope that the merge will be blocked until your approval.
@miss-islington has own logic for this situation.

Anyway, please feel free to express your objection if any, @tirkarthi and I can fix them in the following PR quickly.

miss-islington · 2019-09-13T11:03:10Z

Thanks @tirkarthi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-09-13T11:03:19Z

GH-16087 is a backport of this pull request to the 3.8 branch.

@asvetlov

* This just copies the docs from `StreamWriter` and `StreamReader`. * Add docstring for asyncio functions. https://bugs.python.org/issue36889 Automerge-Triggered-By: @asvetlov (cherry picked from commit d31b315) Co-authored-by: Xtreak <tir.karthi@gmail.com>

…-14488)" This reverts commit b9bfe14.

tirkarthi added 2 commits June 30, 2019 23:03

Document Stream and add docstrings

7ed4629

Remove 3.7 example and group the functions

e2a64ad

tirkarthi requested review from 1st1 and asvetlov as code owners June 30, 2019 17:36

the-knights-who-say-ni added the CLA signed label Jun 30, 2019

bedevere-bot added the awaiting review label Jun 30, 2019

Fix tabs error

19798d5

asvetlov reviewed Sep 9, 2019

View reviewed changes

Lib/asyncio/streams.py Outdated Show resolved Hide resolved

1st1 requested changes Sep 9, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 9, 2019

asvetlov added the skip news label Sep 10, 2019

Add methods to API index and remove deprecated methods.

8ac5116

1st1 reviewed Sep 10, 2019

View reviewed changes

Doc/library/asyncio-api-index.rst Outdated Show resolved Hide resolved

1st1 reviewed Sep 10, 2019

View reviewed changes

Doc/library/asyncio-api-index.rst Outdated Show resolved Hide resolved

1st1 reviewed Sep 10, 2019

View reviewed changes

Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved

1st1 reviewed Sep 10, 2019

View reviewed changes

Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved

1st1 reviewed Sep 10, 2019

View reviewed changes

Add deprecations to index. Reword few sentences.

479a860

asvetlov reviewed Sep 10, 2019

View reviewed changes

Doc/library/asyncio-stream.rst Outdated Show resolved Hide resolved

asvetlov reviewed Sep 10, 2019

View reviewed changes

Doc/library/asyncio-stream.rst Show resolved Hide resolved

Remove reference to stream.drain. Add a note on stream.write as a met…

df72ef4

…hod and a coroutine.

tirkarthi added 2 commits September 13, 2019 10:23

Add deprecated notes to recommend await stream.write

99fefef

Fix write to writelines.

c191de6

asvetlov requested changes Sep 13, 2019

View reviewed changes

Sort the methods by alphabet.

623afb5

asvetlov approved these changes Sep 13, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 13, 2019

asvetlov added 🤖 automerge labels Sep 13, 2019

miss-islington merged commit d31b315 into python:master Sep 13, 2019

bedevere-bot removed the awaiting merge label Sep 13, 2019

asvetlov added needs backport to 3.8 and removed needs backport to 3.8 labels Sep 13, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 13, 2019

aeros added a commit to aeros/cpython that referenced this pull request Sep 28, 2019

Revert "bpo-36889: Document Stream class and add docstrings (pythonGH…

aaf003d

…-14488)" This reverts commit b9bfe14.

Uh oh!

bpo-36889: Document Stream class and add docstrings #14488 8000

bpo-36889: Document Stream class and add docstrings #14488

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!