Ignore writing headers when in ASGI mode #1957
Conversation
@Tronic Which issue is that? Trying to replicate the issue. |
|
After approvals and passing tests, I will tag and push. After that I will "update branch", which will invalidate the approvals, so they will need to be submitted again. |
|
Just curious, why did Multidict need to be updated in order to pass tests? I thought our pinned version of multidict worked fine? I saw the same change in #1956. |
|
It has to do with the new change to PIP that will not let you install conflicting versions of packages. Something else was using a higher pinned version so PIP now fails. |
|
Luckily upgrading to v5 had no impacts and tests all ran fine. |
|
FYI - the appveyor tests will start passing again once I pull master back in here before merge to master. |
|
Go ahead and merge master, double check tests, then merge and release 20.9.1 |
|
OK.... pulling master in. |
Codecov Report
@@ Coverage Diff @@
## master #1957 +/- ##
==========================================
+ Coverage 92.27% 92.47% +0.19%
==========================================
Files 29 29
Lines 3236 3242 +6
Branches 570 571 +1
==========================================
+ Hits 2986 2998 +12
+ Misses 171 167 -4
+ Partials 79 77 -2
Continue to review full report at Codecov.
|
This is something that I came across during the streaming development, and frankly I cannot quite remember for sure if there was a bug in mainline or only in an earlier version of the streaming branch. Since I cannot find such issue now (open or closed), I'm inclined to think it was the latter. |
|
👍 I tried testing it pretty hard, and with the two additional tests, I think the coverage is pretty decent there. Soon enough we will be able to merge |
| @pytest.mark.asyncio | ||
| async def test_chunked_streaming_returns_correct_content_asgi(streaming_app): | ||
| request, response = await streaming_app.asgi_client.get("/") | ||
| assert response.text == "4\r\nfoo,\r\n3\r\nbar\r\n0\r\n\r\n" |
There was a problem hiding this comment.
So turns out this is wrong.
The response.text should be "foo,bar" just like in "test_chunked_streaming_returns_correct_content" non-asgi test.
The bug #1964 is causing the extra lengths and \r\n to be added by Sanic and by the ASGI handler.
There was a problem hiding this comment.
Actually, in the test this is correct because the the httpx asgi-response transport does not add in the chunking markers, so we need this in out response.text to verify that chunked mode was used correctly by the sanic-side stream fn.
But its still broken if using a real ASGI transport like Uvicorn of Daphne, because they do put in the chunking markers.
There was a problem hiding this comment.
Continuing conversation in #1964
This bug is fixed in #1876. In the interim, this resolves the issue and should be applied to both 19.12 and 20.9.
This resolves #1730 and also resolves #1653.
This branch can be tagged and pushed as
v20.9.1. For 19.12, I will submit a seperate PR to the LTS branch.