Skip to content

Support streaming API for AsyncHTTPClient #2209

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

Open
legnaleurc opened this issue Dec 7, 2017 · 2 comments
Open

Support streaming API for AsyncHTTPClient #2209

legnaleurc opened this issue Dec 7, 2017 · 2 comments

Comments

@legnaleurc
Copy link
Contributor

legnaleurc commented Dec 7, 2017

Mostly like this:
https://aiohttp.readthedocs.io/en/stable/client_quickstart.html#streaming-response-content

This could probably solve some usage like #157.

It is hard to control the process in streaming_callback, and it does not support async function as well.

@eulersIDcrisis
Copy link

A seemingly simple fix is to wrap calls here with code like:

class _HTTPConnection(httputil.HTTPMessageDelegate):
    # ...
    def data_received(self, chunk: bytes) -> Optional[Awaitable[None]]:
        if self._should_follow_redirect():
            # We're going to follow a redirect so just discard the body.
            return None
        if self.request.streaming_callback is not None:
            # Should return either 'None' or a Future[None] to throttle future reads.
            return self.request.streaming_callback(chunk)
        self.chunks.append(chunk)
        return None

then to make sure that places that call: delegate.data_received(chunk) do the dance of:

res = delegate.data_received(chunk)
if res:
    await res  # or yield...

Wouldn't it be a bug if this behavior is not be supported, even though the parity of:

@web.stream_request_body
class Handler(tornado.web.RequestHandler):
    # ...
    async def data_received(self, chunk):
        # ....

is?

Additional library support (i.e. using httpx or similar) would work in some cases, but seeing that tornado has a curl client as well as this simple client already, it'd be nice to avoid additional dependencies for simple use-cases.

@eulersIDcrisis
Copy link

If it is any help, I created a PR here: #3471
It is currently in draft, but (after checking the linter state and so forth), I can mark it ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants