bpo-38242: Revert new asyncio streaming API

[aeros]

Motivation: Comment in bpo issue from @1st1:

My final opinion on this issue: we must revert the new streaming API from asyncio and take our time to design it properly. I don't like what we have in the 3.8 branch right now. I don't feel comfortable rushing decisions and doing last minute API changes.

Reverts the following commits from 3.8:

Merge asyncio streams (primary reversion):
23b4b69

Documentation:
b9bfe14
7268fd0
ed9f356

Minor test changes (incompatible with asyncio.StreamReader and asyncio.StreamWriter):
4cdbc452ce3

Manual resolution of several conflicts was also required, the majority were for retaining the deprecation of explicitly passing the loop argument for tasks (345bfc9). There might be a few deprecation warnings missing, still working on adding some back.

This PR must be carefully reviewed, especially since it is removing multiple tests that no longer function properly without the changes made in 23b4b69. It is important to be 100% certain that no additional tests were removed. I believe @asvetlov would be the most qualified individual to verify the tests are in a correct state, since he wrote the majority of the tests related to the new streaming API.

It is most of the way there, but additional changes may be needed before it is complete. Until we are certain, I am adding the DO-NOT-MERGE label.

To @1st1 and @asvetlov:

I have enabled "Allow edits from maintainers" (as I usually do). Please feel free to directly commit changes into the PR. I would appreciate review feedback so that I can understand the reasons behind any changes made, but you certainly don't have to wait on me to make any changes to the PR. Especially since this is a release blocker for 3.8.0.

https://bugs.python.org/issue38242

[aeros]

There's still a few areas that I need to work on, but this should have the majority of the important changes.

Edit: To clarify, the changes I'm still working on involve adding back some of the deprecation warnings in the tests. Other than that, everything else is okay as far as I can tell.

[aeros]

Since StreamReader is now the defacto API with Stream being removed, should its "internal usage only" deprecation warning be removed?

DeprecationWarning: <class 'asyncio.streams.StreamReader'> should be instaniated by
asyncio internals only ...

[aeros]

Restored test__all__.py and test_sslproto.py since they did not require reversion. Also, removing tiran from reviewers (I believe he was added from ownership of test_sslproto).

Edit: Also restored test_base_events and test_server. The only difference between the restored tests was a removed DeprecationWarning for explicitly passing the loop arg (which should remain).

[1st1]

@asvetlov will you have time to glance over this today/tomorrow?

[1st1]

`StreamReaderProtocol` was technically a public API and a lot of people use it.  So I'd revert this change entirely.  We'll just deprecate all of the current streaming API in 3.9.

…

On Sat, Sep 28, 2019 at 11:49 AM, Kyle Stanley < ***@***.*** > wrote: ***@***.**** commented on this pull request. In Lib/ test/ test_asyncio/ test_windows_events. py ( #16455 (comment) ) : > @@ -118,16 +117,16 @@ def test_pipe(self): clients = [] for i in range(5): - stream = asyncio. Stream ( http://asyncio.stream/ ) ( mode=asyncio. StreamMode. READ ( http://mode=asyncio.streammode.read/ ) , - loop=self.loop, _asyncio_internal=True) - protocol = _StreamProtocol(stream, - loop=self.loop, - _asyncio_internal=True) + stream_reader = asyncio.StreamReader(loop=self.loop, + _asyncio_internal=True) @ 1st1 ( https://github.com/1st1 ) It should probably be removed from the __init__ of StreamReader (and all corresponding references) since it's no longer internal with Stream removed, but it can remain for StreamReaderProtocol since that class is still intended for internal usage. Is this okay? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #16455?email_source=notifications&email_token=AAB2LG7CVDJD4S34X2JV243QL6RK3A5CNFSM4I3MQJ4KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGH75FI#discussion_r329322187 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAB2LG2QP775ZTTC76FXZLTQL6RK3ANCNFSM4I3MQJ4A ).

[aeros]

StreamReaderProtocol was technically a public API and a lot of people use it.

Ah, I see. I had never used it myself, but based on the documentation summary of StreamReaderProtocol being an adapter class between Protocol and StreamReader, I had assumed it was primarily intended for internal usage. I'll remove it from both of them if it's considered public API.

[aeros]

@1st1 Should it be removed from Process.__init__() as well? Process is a part of the public API, but based on the docs, it looks like users are not intended to call the constructor directly (https://docs.python.org/3/library/asyncio-subprocess.html#asyncio.asyncio.subprocess.Process).

As a result, I would assume it should remain for Process.

[1st1]

You should use a combination of git blame, git log, git show to recover all bits and pieces that were checked in when we were working on new Streams. If that thing wasn't part of Process before it shouldn't be now.

[aeros]

@1st1

You should use a combination of git blame, git log, git show to recover all bits and pieces that were checked in when we were working on new Streams.

I have been primarily using git blame as much as possible and looking through the older commits, but I thought that one was a bit more subjective since the constructor args for Process are not documented. This led me to believe that asyncio_internal would make sense for Process, even without the new streaming API.

If that thing wasn't part of Process before it shouldn't be now.

It was added by @asvetlov in ad4ed87. The _asyncio_internal changes were technically done before the new streaming API was implemented in 23b4b69, which is why I wasn't certain. Are you considering ad4ed87 to be a part of that new streaming API or was it a separate change?

I'll work on removing it from Process though for now, it sounds like you probably don't want it since it was added around that same time.

[aeros]

I finishing removing all instances of asyncio_internal and removed a few tests that were lingering which were created for the purpose of testing the DeprecationWarnings for usage of the old streaming API ("... should be instantiated by asyncio internals only"). I've also manually added and removed DeprecationWarnings in other parts of the tests. The ones added for the new API were removed, and I added a few self.assertWarning(DeprecationWarning) to tests that were emitting warnings from explicit passing of the loop arg.

As far as I can tell, that should be everything. All of the tests are passing without any warnings and every component of the new streaming API has been reverted.

The only other thing I can think of would be adding the explicit loop arg deprecation to the old API classes. Currently, this does not emit a deprecation warning:

rd = StreamReader(loop=loop)

The same applies to StreamReaderProtocol, StreamWriter, SubprocessStreamProtocol, and Process. It was never added to those classes since the deprecation of passing an explicit loop arg was implemented against the new streaming API in 345bfc9 and 6d64a8f. However, that might not be necessary if we're adding another new streaming API in 3.9, so I'll leave that decision up to you two.

Let me know if there's anything I'm missing. If I don't respond within a short period of time, feel free to directly commit any suggested changes to the PR.

[aeros]

While looking over test_streams again I noticed a duplicated test name test_async_writer_api. At first, I thought the test itself was duplicated and was considering removing it, but upon closer inspection, I realized the tests were slightly different. Based on the diff, it looks like the name of the second one is supposed to be test_async_writer_api_exception_after_close.

I have no idea how that happened in the first place, but it's fixed now.

[aeros]

This was added in commit 23b4b69 with the new streaming API changes, but it looks like it's unrelated. It seems like it would still be quite useful for retaining errors when sockets are closed (see comment on lines 769 - 774 for more info).

[1st1]

Closing in favor of #16482