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

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented May 11, 2019

@asvetlov
Copy link
Contributor Author

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 18:27
@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

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 Outdated Show resolved Hide resolved
Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

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

Lib/asyncio/streams.py Outdated Show resolved Hide resolved
@miss-islington miss-islington merged commit 23b4b69 into python:master May 27, 2019
@asvetlov asvetlov deleted the merge-streams branch September 12, 2019 12:42
aeros added a commit to aeros/cpython that referenced this pull request Sep 27, 2019
Reverts changes made in 3.8 to asyncio streams (23b4b69).
aeros added a commit to aeros/cpython that referenced this pull request Sep 28, 2019
1st1 pushed a commit to 1st1/cpython that referenced this pull request Sep 30, 2019
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants