Repository navigation
Reviewing the streaming API #588
Description
Right now we issue streaming requests using...
response = await client.get(url, stream=True)And have the following stream-specific methods...
.read().raw().stream().stream_text().stream_lines().close()
For the 1.0 release I think we should consider? droping stream=True, as it makes it exceptionally easy to leave unclosed connections hanging around.
I'm interested in if we might want to use the following instead...
async with client.stream('GET', url) as response: # Can only be used as a context manager.
...That way users never have to think about "did I call close on the response", or leave open connections hanging around accidentally.
This is related to #393, but it's a stricter API, that doesn't rely on "users might use a response as a context manger". And also makes it far clearer where and when the streaming methods should be used. Ie. they only make sense inside a .stream() block.
I also think we could rationalize the streaming methods, so...
.read().stream_raw().stream_bytes().stream_text().stream_lines().close()
Activity
florimondmanca commentedon Dec 4, 2019
Thanks for bringing this up! I remember we mentioned it briefly elsewhere but it's good it has its own issue now.
I agree with the idea of a dedicated method, because passing
stream=Trueinduces an important change in behavior that should be better reflected in the API.I also agree on enforcing context manager usage.
Thinking about alternatives, the fact that we have "one method per HTTP verb" for the non-streaming case and "one method accepting the HTTP verb" for the streaming case might look a bit odd.
(Strictly speaking, users can use
.request("GET", ...)for the non-streaming case, but we don't really advertise that possibility e.g. in the docs.)The only constructive idea I might have regarding this would be…
But a) flat is better tha nested and b) that would require us to implement yet another host of methods for each HTTP verb.
So, I think
.stream("<method", url, **kwargs)is fine; at least much better than.get(url, stream=True).💯, yes.
lovelydinosaur commentedon Dec 4, 2019
Indeed yeah.
Options I can see here are...
Provide nested
.get()and pals...Explicit request methods, for the streaming case...
Support either one or two postional args for the streaming case...
lovelydinosaur commentedon Dec 4, 2019
Another take could be that we enforce that responses must be used as a context manager when
stream=Trueis used, so...Although I. think that feels a bit weirdly ambiguous to me.
florimondmanca commentedon Dec 4, 2019
That’s the most reasonable option, IMO.
lovelydinosaur commentedon Dec 5, 2019
Okay, so here's something super nice that the more constrained variant of the streaming API can potentially give us...
.request(),.get(),.post()and pals return aResponseinstance, which doesn't actually include the streaming methods at all..stream()return anAsyncResponseinstance, which does.Why is that a nice thing? Well, for one, we're more tightly scoping the "do some I/O with this thing" to a very particular case. You can only perform I/O on an
AsyncResponse- aResponseis just a plain old bit of state+interface.This also means that
Responseis pickleable, whereAsyncResponseis not, which is a really clear boundary-line.The biggest thing tho' is that once we've got a
SyncClientand anAsyncClientthey actually return the same type for almost all operations, except the more snazzy "I need to control the I/O directly" case. (We won't needSyncRequestandAsyncRequest, but I won't go into that now.)There's one other bit of API that would need finessing in order to achieve that, which would be to drop the one other bit of I/O on the response, which is
response.next(), for calling the next redirect in a chain. Instead that ought to just be something plainer, which returns a redirect response instance, that can be sent explicitly.We won't go into any of that initially - Let's just get the streaming API squared away first - but wanted to share what I'm thinking there.
florimondmanca commentedon Dec 5, 2019
Or...
StreamingResponse? If « async » there really just means « this response may still do I/O » then this would make even more sense to me.But yeah; at the moment we have a rather strange « read the response content, unless we’re steaming » bit in the client. Switching to a clear distinction between the two will definitely make things cleaner. We might even just get the entire content from inside the actual connections, and pass plain bytes the
Response?florimondmanca commentedon Dec 5, 2019
@tomchristie FYI, I'm looking into this at the moment. :-)