Revert the new asyncio Streams API

@rhettinger

BPO	38242
Nosy	@rhettinger, @njsmith, @asvetlov, @cjrh, @ambv, @1st1, @icgood, @tirkarthi, @bmerry, @aeros
PRs	[3.8] bpo-38242: Revert new asyncio streaming API #16445 bpo-38242: Revert new asyncio streaming API #16455 bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)" #16482 [3.8] bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)" (#16482) #16485 bpo-34975: Add start_tls() method to streams API #13143

^{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 = <Date 2019-12-08.16:46:58.395>
created_at = <Date 2019-09-20.22:58:45.061>
labels = ['type-bug', '3.8', 'expert-asyncio']
title = 'Revert the new asyncio Streams API'
updated_at = <Date 2022-03-22.16:23:05.647>
user = 'https://github.com/1st1'

bugs.python.org fields:

activity = <Date 2022-03-22.16:23:05.647>
actor = 'icgood'
assignee = 'none'
closed = True
closed_date = <Date 2019-12-08.16:46:58.395>
closer = 'asvetlov'
components = ['asyncio']
creation = <Date 2019-09-20.22:58:45.061>
creator = 'yselivanov'
dependencies = []
files = []
hgrepos = []
issue_num = 38242
keywords = ['patch']
message_count = 25.0
messages = ['352908', '352911', '352931', '352934', '352935', '352936', '352942', '352943', '352947', '352948', '353044', '353045', '353046', '353052', '353057', '353058', '353103', '353401', '353405', '353417', '353420', '353536', '353539', '353540', '353541']
nosy_count = 10.0
nosy_names = ['rhettinger', 'njs', 'asvetlov', 'cjrh', 'lukasz.langa', 'yselivanov', 'icgood', 'xtreak', 'bmerry', 'aeros']
pr_nums = ['16445', '16455', '16482', '16485', '13143']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue38242'
versions = ['Python 3.8']

== Context

Andrew and I implemented a new streaming API in asyncio 3.8. The key idea is that there's a single Stream object, combining old StreamReader & StreamWriter APIs. On top of that, Stream.write() and Stream.close() became coroutines. The new API is significantly easier to use, but it required us to:

(a) Make backwards compatible changes to subprocess APIs;
(b) Add two new APIs: asyncio.connect() -> Stream and asyncio.StreamServer
(c) Soft-deprecate asyncio.open_connection() and asyncio.start_serving().

The Trio project considers implementing new Streams API (see [1]). The key idea is to make the core Stream object very simple and then enable building complex protocol pipelines using composition. Want SSL? Add an SSL layer. Have a framed binary protocol? Push a reusable framing layer to the stack and parse the protocol. On top of that, Trio wants us to standardize Streams, so that it's possible to write framework agnostic protocol code using async/await and to even reuse things like SSL implementation.

== Problem

I really like how Trio approaches this.

The asyncio.Stream class we have now is overloaded with functionality. It's not composable. It's internal buffer and APIs are designed to parsing text protocols (i.e. parsing a complex binary protocol requires an entirely different buffer implementation).

Long story short, I think we should revert the new streaming API from the 3.8 branch and see if Trio & asyncio can design a better Streaming API. Otherwise we end up in a weird situation where we added a bunch of new APIs to 3.8 which can be deprecated in 3.9.

Worst case scenario we'll just ship our current versions of Streams in 3.9 (not in 3.8).

Thoughts?

[1] python-trio/trio#1208

Yury asked me to weigh in here, since I guess between him and Andrew there's some uncertainty about whether reverting is the right choice or not. I can't answer that, but I can share some thoughts.

Unfortunately, I wasn't aware of the Stream PR when it was first being written and reviewed; I only discovered the change a month or two ago, by accident. If I had, I'd have said something like:

This is definitely a major improvement over the old API, that fixes some important issues, which is awesome. But, it only fixes some of the issues, not all of them, and it's really difficult to change things in asyncio after they ship. So there's a tricky question: do you want to ship this now so users can start taking advantage of its improvements immediately, or do you want to wait to make sure you don't have to do multiple rounds of changes?

Of course, now we're in a situation where it's already merged, which makes things more awkward. But maybe it will clarify things a bit to do a thought experiment: if the asyncio.Stream API was a PR that hadn't been merged yet and we were considering merging it – would you hit the green merge button, given what we know now, or would you hold off for 3.9?

There is an alternative proposal: let's deprecate and eventually remove streams API entirely (for sockets, pipes, and subprocesses).
By this, we will never have a chance to conflict with trio.

Another option is removing asyncio at all (again to never conflict with trio and possible future awesome libraries) but the ship has sailed maybe.

Reversion is cheap. Published APIs are hard to take back.

If the plan is to deprecate, then anything added for 3.8 should be reverted. There's no point in publishing a new API only to take it away.

That seems a bit extreme, and I don't think conflicts with Trio are the most important motivation here. (And they shouldn't be.) But, we should probably write down what the motivations actually are.

Yury might have a different perspective, but to me the challenge of the asyncio stream API is that it's trying to do three things at once:

Provide a generic interface for representing byte streams
Provide useful higher-level tools for working with byte streams, like readuntil and TLS
Adapt the protocol/transports world into the async/await world

These are all important things that should exist. The problem is that asyncio.Stream tries to do all three at once in a single class, which makes them tightly coupled.

*If* we're going to have a single class that does all those things, then I think the new asyncio.Stream code does that about as well as it can be done.

The alternative is to split up those responsibilities: solve (1) by defining some simple ABCs to represent a generic stream, that are optimized to be easy to implement for lots of different stream types, solve (2) by building higher-level tools that work against the ABC interface so they work on any stream implementation, and then once that's done it should be pretty easy to solve (3) by implementing the new ABC for protocol/transports.

By splitting things up this way, I think we can do a better job at solving all the problems with fewer compromises. For example, there are lots of third-party libraries that use asyncio and want to expose some kind of stream object, like HTTP libraries, SSH libraries, SOCKS libraries, etc., but the current design makes this difficult. Also, building streams on top of protocols/transports adds unnecessary runtime overhead.

A further bonus is that (1) and (2) aren't tied to any async library, so it's possible to borrow the work that Trio's been doing on them, and to potentially share this part of the code between Trio and asyncio.

So that all sounds pretty cool, but what does it have to do with shipping asyncio.Stream in 3.8? The concern is that right now asyncio has two APIs for working with byte streams (protocols/transports + StreamReader/StreamWriter); then 3.8 adds asyncio.Stream; and then the design I'm describing will make a *fourth*, which is a lot. I think we need something *like* asyncio.Stream in the mix, for compatibility and bridging between the two worlds, but right now it's not quite clear how that should look, and pushing it off to 3.9 would give us time to figure out the details for how these things can all fit together best.

I think we need something *like* asyncio.Stream in the mix, for compatibility and bridging between the two worlds, but right now it's not quite clear how that should look, and pushing it off to 3.9 would give us time to figure out the details for how these things can all fit together best.

Sorry, this was unclear. The "two worlds" I mean here are the current asyncio landscape and a future where we have standardized composable ABCs.

I am going to recommend sticking to the status quo, i.e. Andrew's improvements to asyncio.Stream should stay. The rest of this message is an elaboration.

The new perfect composable Streams design is just that -- a design. Many things could go wrong in the implementation. Once it exists as a 3rd party add-on and users agree it's better we *may* decide to deprecate asyncio.Stream. Or not -- there are many examples in the stdlib of modules for which a better 3rd party alternative exists but which are nevertheless useful and not deprecated.

Much of the asyncio universe already exists outside the stdlib -- the perfect composable Streams API should probably never be moved into the stdlib (unless it's so perfect that it never needs to change :-).

Andrew's improvements help current users of asyncio.Stream. If those users eventually want to migrate to the perfect composable Streams API, once it's available, fine. But I don't think we're helping them by not giving them improvements that have already been implemented (and which everyone here agrees are improvements!) right now.

Users of the current asyncio.Stream have a choice -- they can adopt the improvements in 3.8, or they can wait for the perfect design to be implemented. Everybody's constraints are different. Let's not pretend we already know what they should choose.

If in the future we end up changing our mind, that's okay. It's happened before, and we've lived.

I would say that we can reimplement the Stream class on top of new composable streams in the future.
I'd love to do it right now but these streams don't exist yet.

I totally support the idea of new streams design, it looks very attractive. The implementation may take a while and require many labor-hours though.

asyncio is driven by volunteers, mostly by Yuri and I. We constantly don't implement all our *wishes* at the time on next Python feature-freezing but only a subset of them.

Maybe the situation will change in the future but I have no strong confidence that we finish the design of the new streams in 3.9.

Task group is another very required thing. If I choose between groups and steams I probably invest my time into the former.

Task group is another very required thing. If I choose between groups and steams I probably invest my time into the former.

We should get them in 3.9. I'm going to be working on the ExceptionGroup PEP today.

Awesome!

I saw Yury on Saturday, and we spent some time working through the implications of different options here.

For context: the stream ABCs will be a bit different from most third-party code, because their value is proportional to how many projects adopt them. So some kind of central standardization is especially valuable. And, they'll have lots of implementors + lots of users, so they'll be hard to change regardless of whether we standardize them (e.g. even adding new features will be a breaking change). Part of the reason we've been aggressively iterating on them in Trio is because I know that eventually we need to end up with a minimal core that can never change again, so I want to make sure we find the right one :-).

So Yury and I are tentatively thinking we'll make some kind of PEP for the 3.9 timescale, probably just for the core ABCs, maybe in the stdlib or maybe just as an informational PEP that we can use to convince people that this is "the python way" (like the WSGI PEP).

Now, the implications for the asyncio.Stream API in 3.8. This is tricky because we aren't sure of how everything will shake out, so we considered multiple scenarios.

First decision point: will asyncio.Stream implement the ABCs directly, or will you need some kind of adapter? If we go with an adapter, then there's no conflict between the ABC approach and whatever we do in 3.8, because the adapter can always fix things up later. But, it might be nicer to eventually have asyncio.Stream implement the ABC directly, so users can use the recommended API directly without extra fuss, so let's consider that too.

Next decision point: will the byte stream ABC have an __aiter__ that yields chunks? We're pretty sure this is the only place where the ABC *might* conflict with the asyncio.Stream interface. And as Josh Oreman pointed out in the Trio issue thread, we could even avoid this by making the ABC's chunk-iterator be a method like .chunks() instead of naming it __aiter__.

So even with the current asyncio.Stream, there are two potentially workable options. But, they do involve *some* compromises, so what would happen if we wanted asyncio.Stream to implement the ABC directly without and adapter, *and* the ABC uses __aiter__? We can't do that with the current asyncio.Stream. Are there any tweaks we'd want to make to 3.8 to keep our options open here?

The obvious change would be to leave out __aiter__ from asyncio.Stream in 3.8. That would leave all our options open. For sockets this would be easy, because the old functions are still there and still returning StreamReader/StreamWriter objects. For 3.8, we're adding a bunch of new Stream-based APIs, and users won't encounter a Stream until they switch to those. (The new APIs are: connect, connect_read_pipe, connect_write_pipe, connect_unix, StreamServer, UnixStreamServer). The problem is the subprocess functions (create_subprocess_exec, create_subprocess_shell), since they've been changed *in place* to return asyncio.Stream instead of StreamReader/StreamWriter.

We considered the possibility of migrating the existing subprocess functions to a different __aiter__ implementation via a deprecation period, but concluded that this wasn't really workable. So if we want to go down this branch of the decision tree, then 3.8 would have to leave create_subprocess_{exec,shell} as using StreamReader/StreamWriter, and in either 3.8 or 3.9 we'd have to add new subprocess functions that use Stream, like we did for sockets.

Doing this for subprocesses is a bit more complicated than for sockets, because subprocesses have a Process object that holds the stream objects and interacts with them. But looking at the code, I don't see any real blockers.

If we completely separated the old StreamReader/StreamWriter functions from the new Stream functions, then it would also have another advantage: we could clean up several issues with Stream that are only needed for compatibility with the old APIs. In particular, we could get rid of the optional-await hacks on 'write' and 'close', and turn them into regular coroutines.

So I guess this is the real question we have to answer now. Which of these do we pick?

Option 1: Keep the Stream code as-is, and accept that using asyncio.Stream with future ABC-based code will require some compromises

Option 2: Create new functions for spawning subprocesses; revert create_subprocess_{exec,shell} to use StreamReader/StreamWriter; take advantage of the break between old and new APIs to clean up Stream in general; take advantage of that cleanup to remove __aiter__ so the the future ABC-based code won't have to compromise.

I think this is one of those good problems to have, where really we could live with either option :-). Still, we should pick one on purpose instead of by accident.

Yury's preference was "option 1", because he feels compromises for the ABC design aren't that bad, and adding and documenting new subprocess APIs is expensive.

I'm leaning towards "option 2", because we're already paying the cost of adding and documenting a ton of new APIs for sockets. Given that, adding a few more to that list doesn't add that much cost, and it lets us get more value from *all* of the new APIs, by letting us shed more of the legacy backcompat constraints. Right now we don't have any path to fixing 'write'/'close' at all; this would give us one. I'd also be willing to put in some time to actually implement this if that turns out to be the limiting factor.

Since this has been elevated to a release blocker, I wouldn't mind helping to revert this ASAP. I can open a PR to fix it today.

Sure, by all means, any help would be hugely appreciated. Thank you, Kyle.

You'll need to be careful to only revert the new functions & the asyncio.Stream class. Also the new docs. Due to proximity to the deadline, please be prepared that we might need to abandon your pull request if it's not ready by Sunday morning. In which case Andrew or I will do this ourselves.

You'll need to be careful to only revert the new functions & the asyncio.Stream class.

So far the trickiest part has proven to be the tests (specifically test_streams.py) and keeping the deprecation warning for passing explicit loop arguments. I've had to be careful to be certain that no tests were unintentionally removed.

Due to proximity to the deadline, please be prepared that we might need to abandon your pull request if it's not ready by Sunday morning

No problem, I'll make sure to allow anyone with write access (yourself and Andrew) to edit the PR I open directly to make any needed changes. That way, at least any progress I make on it can help reduce the amount of work for you two.

Currently focusing on the Lib/asyncio/* and Lib/test/* changes. Working on doc changes next, but that should be significantly easier.

In addition to 23b4b69 (main commit from Andrew that added asyncio.Stream and new functions), I've also had to remove 4cdbc452ce3 (minor asyncio test change from Pablo) due to it causing issues with the other tests from deleting asyncio.StreamReader and asyncio.StreamWriter.

New changeset 6758e6e by Yury Selivanov in branch 'master':
bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)" (bpo-16482)
6758e6e

New changeset 1c19d65 by Yury Selivanov in branch '3.8':
bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)" (bpo-16482) (bpo-16485)
1c19d65

I've reverted the code. Andrew, would really appreciate if you could quickly do a post commit review.

I've reverted the code. Andrew, would really appreciate if you could quickly do a post commit review.

Oops, I'll reopen it.

Has there been any follow-up work on the discussions here, around implementing a layered byte streaming API? Are there any public discussions one can follow?

Sorry, I think it just died after the revert.

@gvanrossum

@gvanrossum Gotcha, thanks for the update. Not a problem really, just needed to make sure I'm taking an informed design decision in integrating against StreamReader/StreamWriter for now.

Navigation Menu

Repository navigation

Revert the new asyncio Streams API #82423

7 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Revert the new asyncio Streams API #82423

Description

Activity

1st1 commented on Sep 20, 2019

njsmith commented on Sep 20, 2019

asvetlov commented on Sep 21, 2019

rhettinger commented on Sep 21, 2019

njsmith commented on Sep 21, 2019

njsmith commented on Sep 21, 2019

gvanrossum commented on Sep 21, 2019

asvetlov commented on Sep 21, 2019

1st1 commented on Sep 21, 2019

asvetlov commented on Sep 21, 2019

njsmith commented on Sep 24, 2019

7 remaining items

1st1 commented on Sep 27, 2019

aeros commented on Sep 27, 2019

aeros commented on Sep 27, 2019

1st1 commented on Sep 30, 2019

1st1 commented on Sep 30, 2019

1st1 commented on Sep 30, 2019

aeros commented on Sep 30, 2019

aiven-anton commented on Dec 28, 2022

gvanrossum commented on Dec 28, 2022

aiven-anton commented on Jan 2, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions