Author njs
Recipients asvetlov, gvanrossum, lukasz.langa, njs, rhettinger, xtreak, yselivanov
Date 2019-09-24.03:37:15
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1569296236.1.0.00811327958962.issue38242@roundup.psfhosted.org>
In-reply-to
Content
> I hope that the Trio project can minimize the number of methods they want to add to those ABCs so that we don't need to duplicate a lot of functionality in asyncio.Stream.  E.g. in the new asyncio.Stream there's a Stream.write() coroutine; in Trio it's Stream.send_all().  I'm not entirely convinced that "send_all()" is a good name, for example, even though I now understand the motivation.  We can discuss that later in a relevant issue though.

Yeah, we definitely need to bikeshed the method names but we should do that separately. The actual methods are mostly settled though, and designed to be as minimal as possible. The complete set is:

- send data
- send EOF
- receive data
- close
- and *maybe* a kind of 'drain' method, which is quasi-optional and I'm hoping we can get rid of it; it's related to some latency/flow control/buffering issues that are too complex to discuss here

And we'll provide standard implementations for iteration and __aenter__/__aexit__.

> Another important point to consider: if the new Trio Stream ABCs are *significantly different* from asyncio.Stream and would require us to alias too many methods or to do heavy refactoring and deprecations, then Trio will have to show some real world usage and traction of its APIs first.

It sounds like the only aliasing will be for the send data method; everything else either has different semantics, or has both the same semantics and the same name.* And there won't be any refactoring needed; the whole goal of this design is to make sure that any reasonable stream implementation can easily provide these methods :-).

*I was going to say the "send EOF" method would also be potentially aliased, but send EOF should be async, because e.g. on a TLS stream it has to send data, and Stream.write_eof is sync. Or maybe we should migrate Stream.write_eof to become async too?

> Nathaniel, I think it's important to emphasize that those compromises should be mutual.  I'm willing to support changing "Stream.close()" to "Stream.aclose()" and to perhaps alias some methods.  We can also implement "Stream.chunks()".  But changing the semantics of "__aiter__" is, unfortunately, not on the table, at least for me.

Let's put __aiter__ aside for a moment, and just think about what's best for asyncio itself. And if that turns out to include breaking compatibility between Stream and StreamReader/StreamWriter, then we can go back to the discussion about __aiter__ :-)

> Here's the plan:
>
> 1. We add "aclose()" and "write()" coroutines to the new "asyncio.Stream()".  It won't have "wait_closed()" or "drain()" or "close()".
>
> 2. We add a _LegacyStream class derived from the new asyncio.Stream.  We will use it for subprocesses. Its "write()" method will return an "OptionallyAwaitable" wrapper that will nudge users to add an await in front of "stdin.write()".  _LegacyStream will be completely backwards compatible.
>
> This path enables us to add a decent new streaming API while retaining consistency and backwards compatibility.

I don't think this is a terrible plan or anything like that. But I'm still confused about why you think it's better than adding new subprocess spawn functions. IIUC, your goal is to make the documentation and deprecations as simple as possible.

If we add two new subprocess functions, then the documentation/deprecations look like this:

- We have to document that there are two different versions of the stream API, the new one and the legacy one
- We have to document how Stream works, and how StreamReader/StreamWriter work
- We have to document that 6 functions that return old-style streams are deprecated (open_connection, start_server, open_unix_connection, start_unix_server, create_subprocess_exec, create_subprocess_shell) and replaced by 8 new functions (connect, StreamServer, connect_unix, UnixStreamServer, connect_read_pipe, connect_write_pipe, and whatever we called the new subprocess functions)

OTOH, with your proposal, we also have a set of deprecations and migrations to do:

- We have to document that there are two different versions of the stream API, the new one and the legacy one
- We have to document how Stream works, and how StreamReader/StreamWriter work
- We have to document that 4 functions that return old-style streams are deprecated (open_connection, start_server, open_unix_connection, start_unix_server) and replaced by 6 new functions (connect, StreamServer, connect_unix, UnixStreamServer, connect_read_pipe, connect_write_pipe)
- We have to document that Process objects use a third kind of stream object that doesn't match either the old or new APIs, and how this one works
- We have to deprecate the no-await write and no-await close (and maybe no-await write_eof)

In addition, deprecating a function is straightforward and well understood: you just issue a deprecation warning, and tools can automatically show the source of the problem and suppress duplicates, or convert it into an error. But deprecating call-without-await is messy – you have to issue the warning from inside __del__, so you can't easily find the source of the problem, suppress duplicates, or convert it into an error.

So like... both of these approaches are definitely possible, but to me it seems like if you look at it holistically, your approach is actually making the documentation and deprecations *more* complicated, not less.
History
Date User Action Args
2019-09-24 03:37:16njssetrecipients: + njs, gvanrossum, rhettinger, asvetlov, lukasz.langa, yselivanov, xtreak
2019-09-24 03:37:16njssetmessageid: <1569296236.1.0.00811327958962.issue38242@roundup.psfhosted.org>
2019-09-24 03:37:16njslinkissue38242 messages
2019-09-24 03:37:15njscreate