Possible bug in response.py when using Sanic + ASGI (chunked stream)

[logtheta]

Describe the bug
In response.py write method, if chunked==true the code will push the data into the stream specifying the len:

if self.chunked:
            await self.protocol.push_data(b"%x\r\n%b\r\n" % (len(data), data))

It looks like that uvicorn and daphne already execute this in httptools._impl.py (uvicorn>protocol>http) resulting in changing what the client will consider the body of the response.
If sanic is not using ASGI obviously we need that operation but if we use ASGI the content is prepared by the corresponding gateway.

How to reproduce
It is enough to stream a generic file and observe the response received by the client. With text files (like in the example) it's not big issue, but if you deal with media files, the decoder on client-side will just fail (I figured this out trying to stream mp4).

main.py

import os
from sanic import Sanic
from sanic.handlers import ContentRangeHandler
from sanic.exceptions import NotFound, HeaderNotFound, InvalidUsage, SanicException
from sanic import Blueprint, response
from aiofiles import os as async_os
from sanic.response import file_stream
import uvicorn

app = Sanic(__name__)

@app.route("/test.txt")
async def handler_file_stream(request):
    return await response.file_stream(
        "./test.txt",
        chunk_size=1024
    )

if __name__ == "__main__":
    #app.run(host="0.0.0.0", port=8000, debug=False)
    uvicorn.run(app, host="0.0.0.0", port=8000, workers=1, log_level="debug")

test.txt

"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut 
labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris 
nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit 
esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt
 in culpa qui officia deserunt mollit anim id est laborum."

Note: setting chunked=True in the response.file_stream() will make the response working but it is not the default value.

Environment:

• OS: Tested with MacOS and Linux Ubuntu 20
• Version 20.9.1, uvicorn 0.12.2, daphne 3.0.0
• Python 3.8

Thanks!

[ashleysommer]

Hi @logtheta
Looks like this is very similar to, but not the same as #1730
#1730 was fixed by #1957 and released in v20.9.1, but looks like it doesn't fix this issue.
I'll look into whether it might be a quick fix.

[ashleysommer]

Here in uvicorn, for reference: https://github.com/encode/uvicorn/blob/6468b70c85e10ae2d405165ca69f2b6a5bd55878/uvicorn/protocols/http/httptools_impl.py#L512

[logtheta]

@ashleysommer Thanks for looking into 🙏 . Yes it looks similar to #1730, I think it is worth to fix it since the chunked feature is very useful, I personally use it for video pseduo streaming.

Thanks again!

[ashleysommer]

Looks like the way we're doing chunked-encoding is completely incompatible with the ASGI-spec here:
https://asgi.readthedocs.io/en/latest/specs/www.html#response-start-send-event

Thats why our current solution works if httpx is the asgi-response-transport, but not if uvicorn or daphne are the transport.
Thats why the tests pass, but we see breakage in real-world applications.

@huge-success/sanic-core-devs See the section in the spec regarding

You may send a Transfer-Encoding header in this message, but the server must ignore it. Servers handle Transfer-Encoding themselves, and may opt to use Transfer-Encoding: chunked if the application presents a response that has no Content-Length set.

So in asgi-mode we need to not set the Transfer-Encoding: chunked header, and to signal to the asgi-transport we're doing chunked mode, we need to not specify a "content-length" header, then the asgi-transport will do its own chunking, based on our subsequent http.response.body messages.

[logtheta]

@ashleysommer I agree, it should be ASGI's responsibility to "prepare" the chunking (when from Sanic we just set the flag). At the beginning I thought it was uvicorn' issue but then I realized that other gateways were doing the same so...I decided to open the issue.

[ashleysommer]

If anything, the bug is in the httpx asgi-response-transport mechanism, because thats what we test against. It doesn't do any chunking at the response level, so that lead us to assume we still do it at the Sanic level.
But response chunking is optional for the ASGI transport, so its not really a bug that it doesn't do it. The httpx transport just gathers up all of the async body parts and sends it when its done, rather than doing chunked transport, which is a perfectly valid way of doing it if the ASGI-transport doesn't support chunked transport.

[ashleysommer]

I've got a fix made, just need to package it up into a nice PR for master, and for the 20.9.x series, and also for the 19.12.x LTS series

[logtheta]

Fantastic! thank you so much, @ashleysommer!

[ahopkins]

#1966 and #1965 merged.

I released v19.12.4. Do we also want to release and intermediary build in 20.9? I am inclined to say no. This has been in there for a long while, and we are not too far away from 20.12 already. If this is holding someone up, they can use the LTS or master for about a month.

[ashleysommer]

@logtheta (jtheta on the forum) was using v20.9.1 and would probably appreciate a v20.9.2 with this fix for their app, but I agree with you, we're only about a month away from the 20.12 release, and v19.12.4 or #master can be used with this fix until then.

[logtheta]

@ashleysommer @ahopkins, very much appreciated, v19.12.4 works as expected.
Thanks