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

New style streaming route handler improvements #2067

Closed
ahopkins opened this issue Mar 17, 2021 · 7 comments · Fixed by #2094
Closed

New style streaming route handler improvements #2067

ahopkins opened this issue Mar 17, 2021 · 7 comments · Fixed by #2094

Comments

@ahopkins
Copy link
Member

ahopkins commented Mar 17, 2021

Is your feature request related to a problem? Please describe.
The new streaming API allows streaming responses in the route handler without having to use a callback:

@app.route("/")
async def test(request):
    response = await request.respond(content_type="text/csv")
    await response.send("foo,")
    await response.send("bar")
    await response.send("", True)
    return response

Describe the solution you'd like
A simpler method to closing the stream before returning in place of await response.send("", True)

await response.eof()

Under the hood, eof should simply just be a convenience call to send("", True).

@ajay1mg
Copy link
Contributor

ajay1mg commented Mar 29, 2021

this should work in sanic -> response -> BaseHTTPResponse, right?

    async def eof(self):
        await self.send("", True)

@ahopkins

@ahopkins
Copy link
Member Author

ahopkins commented Mar 29, 2021

Yup. Eventually, we probably can merge HTTPResponse right into BaseHTTPResponse. Until we do that, the eof() should be in the same place as send() since it would simply be a shortcut.

@ahopkins
Copy link
Member Author

ahopkins commented Mar 29, 2021

@ajay1mg Are you working on a PR for this?

If yes, then add a check for self.streaming_fn inside eof(). If it exists, that we should raise an exception since eof should only ever be called when there is no streaming_fn. And, maybe it should be NotImplementedError on StreamingHTTPResponse.

@ajay1mg
Copy link
Contributor

ajay1mg commented Mar 29, 2021

I am working on the PR for this but I am confused with your second comment. HttpResponse class doesn't have any attribute streaming_fn nor does BaseHTTPResponse

@ahopkins
Copy link
Member Author

ahopkins commented Mar 29, 2021

Sorry for not being clear. This is what I meant:

class StreamingHTTPResponse(BaseHTTPResponse):
    def eof(...):
        raise NotImplementedError

This class should not have it.

@ajaygupta2790
Copy link
Contributor

I have added the PR for this from my personal account. didn't realize I was using my org account
#2094

Please let me know if there is anything that need to be added.

@ahopkins
Copy link
Member Author

ahopkins commented Apr 4, 2021

Please let me know if there is anything that need to be added.

Did you see my message about trying to add some tests?

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

Successfully merging a pull request may close this issue.

3 participants