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

bpo-36889: Merge asyncio streams #13251

Merged
merged 95 commits into from May 27, 2019
Merged

Conversation

Copy link
Contributor

@asvetlov asvetlov commented May 11, 2019

@asvetlov
Copy link
Author

@asvetlov asvetlov commented May 13, 2019

The main part is basically done. The functionality is test covered for all significant code paths because the PR doesn't change too much but "just" merges StreamReader and StreamWriter.

There are two options for future steps:

  1. Add several tests for StreamMode related API, write NEWS, merge and make other improvements in following pull requests. I prefer this way for the sake of simplifying the review process. I have a strong feeling that the proposed change is viable. We did miss it starting from Python 3.5.
  2. Another approach is to keep working on this branch to collect all wish changes at once. The review becomes complicated though.

The wish list is:

  1. Add asyncio.connect() and asyncio.StreamServer() to avoid working with stream, stream where was reader, writer. The same for UNIX sockets.
    StreamServer should be mimic to asyncio.AbstractServer but without boilerplate. The usage is:
async with StreamServer(host, port) as server:
   await server.serve_forever()

StreamServer should create low-level asyncio server (loop.create_server()) with start_serving=False. We discussed the server design with @1st1 offline already.

  1. Support both Windows and UNIX pipes (a new API whose implementation is really trivial with unified streams). See loop.connect_read_pipe and loop.connect_write_pipe for the context.

  2. Rewrite SubprocessStreamProtocol to add subprotocols from stdin, stdout, and stderr. It can simplify things a lot.

@asvetlov asvetlov marked this pull request as ready for review May 13, 2019
@asvetlov asvetlov requested review from 1st1 and gpshead as code owners May 13, 2019
@asvetlov asvetlov changed the title bpo-36889: Merge streams bpo-36889: Merge asyncio streams May 13, 2019
Lib/asyncio/streams.py Outdated Show resolved Hide resolved
Lib/asyncio/streams.py Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
Merge asyncio.StreamReader and asyncio.StreamWriter into asyncio.Stream
Copy link
Member

@1st1 1st1 May 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please enumerate all new APIs in this NEWS entry.

Lib/asyncio/streams.py Show resolved Hide resolved
Lib/asyncio/streams.py Outdated Show resolved Hide resolved
1st1
1st1 approved these changes May 27, 2019
Copy link
Member

@1st1 1st1 left a comment

Please try to address my comments prior to merging. Great work, Andrew!

Lib/asyncio/streams.py Outdated Show resolved Hide resolved
@asvetlov asvetlov added the 🤖 automerge label May 27, 2019
@miss-islington miss-islington merged commit 23b4b69 into python:master May 27, 2019
5 checks passed
@asvetlov asvetlov deleted the merge-streams branch Sep 12, 2019
aeros added a commit to aeros/cpython that referenced this issue Sep 27, 2019
Reverts changes made in 3.8 to asyncio streams (23b4b69).
aeros added a commit to aeros/cpython that referenced this issue Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA signed 🤖 automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants