WIP: Streaming

[Tronic]

This is an effort the clean up server code and make streaming requests and responses first class citizens, with non-streaming implemented on top of them instead of having duplicate code and separate modes for them.

So far request streaming is looked at, and it seems to work fairly well. 7 tests fail, which might be due to expectation that all of request body would be read prior to sending response because the new code sends a response as soon as possible.

A possible bug discovered: I think the old code would get confused if more than one request was sent without waiting for the prior ones be responded first. Additionally, such parallel streaming responses could get entirely mixed up. Although pipelining in such a way is non-standard, it should now be fixed.

This is NOT complete or meant to be merged yet, but the idea is to finish the request handling, and then try to attempt streaming responses without callbacks as prototyped earlier in the Trio branch.

Testing for the advantageous:

pip install git+https://github.com/Tronic/sanic@streaming

Bug reports resolved in this:
Fix #1722
Fix #1730
Fix #1785
Fix #1790
Fix #1804

[Tronic]

Status update: rewriting the server with async instead of a lot of callbacks made the code a lot easier to work with and actually faster despite also removing the MagicHttp stack (because it doesn't support async). The current pure Python parser pushes 50000 req/s where the master branch only does 44000 req/s.

Quite a bit of work remains to be done, especially if all corner cases are to be covered. A new parser means that it is very difficult to make it behave identically to the old one. At this point I've got 6 failing tests and 636 passing.

[vltr]

Wow! You took out httptools and was still able to get better performance from Sanic using plain Python? That's quite unexpected but really interesting ... I'll take a closer look on the code when I can. Great job!

[codecov]

Codecov Report

Merging #1791 into master will decrease coverage by 2.07%.
The diff coverage is 80.05%.

@@            Coverage Diff             @@
##           master    #1791      +/-   ##
==========================================
- Coverage   92.17%   90.09%   -2.08%     
==========================================
  Files          23       24       +1     
  Lines        2312     2525     +213     
  Branches      424      475      +51     
==========================================
+ Hits         2131     2275     +144     
- Misses        141      190      +49     
- Partials       40       60      +20     

Impacted Files	Coverage Δ
sanic/compat.py	81.25% <20.00%> (-4.96%)	⬇️
sanic/errorpages.py	93.47% <40.00%> (-6.53%)	⬇️
sanic/http.py	76.44% <76.44%> (ø)
sanic/response.py	96.92% <84.21%> (-1.15%)	⬇️
sanic/app.py	91.37% <88.09%> (-0.89%)	⬇️
sanic/asgi.py	92.20% <96.29%> (+1.39%)	⬆️
sanic/headers.py	100.00% <100.00%> (ø)
sanic/request.py	99.60% <100.00%> (-0.03%)	⬇️
sanic/testing.py	98.01% <100.00%> (+0.04%)	⬆️
sanic/router.py	96.09% <0.00%> (-0.98%)	⬇️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36ffaa7...36ffaa7. Read the comment docs.

[vltr]

So ... Since we might be moving away from httptools, one thing comes to mind: we are now parsing all data by ourselves and there are some specific cases that httptools (or, more specifically, http-parser) would handle for us (and have that available with its own unit tests), like buffer overflows, well formed but incomplete requests and so on. I believe that we should tackle at least a handful of these tests into Sanic to avoid introducing new bugs at the cost of the void left by httptools | http-parser.

[Tronic]

Agreed, we definitely need to comprehensive testing of request handling. There are also interesting cases around streaming, like if a handler finishes its response before request body is even (fully) sent. I'd appreciate it if someone else was willing to write tests because I'll probably overlook the assumptions done in my implementation.

Next milestones:

• Maybe get rid of StreamingBuffer class as it is already degraded to a simple wrapper.
• Implement request.respond(...) for spawning streaming responses
• Implement StreamingHttpResponse and simple responses on top of that

[Tronic]

All responses made streaming now, too. That was quite a bit more work than anticipated, and there is still work to be done but this is taking good shape now, and doesn't break existing interfaces much. Looks like this could be aimed for Sanic 20.06 release, possibly with some deprecations to prepare for it in 20.03.

Next milestones:

• Resolve remaining test failures
• Improve request.respond usability?
• Fix response middleware handling (how exactly, remains an issue)
• Try to simplify and de-duplicate error handling (especially so in app.py)
• Fix curl is not agree wit return in chunk mode #1722 and Error handlers are synchronous #1785

[vltr]

Agreed, we definitely need to comprehensive testing of request handling. There are also interesting cases around streaming, like if a handler finishes its response before request body is even (fully) sent. I'd appreciate it if someone else was willing to write tests because I'll probably overlook the assumptions done in my implementation.

Awesome work, @Tronic ! Regarding the parser (yet), I see that you already transferred most of the logic to the http.py module, but to create tests regarding HTTP parsing only we would need to break it down a little bit more (more specifically take out the parsing logic to not be intertwined with the protocol logic). Do you think that's "doable" - even if this requires a plethora arguments in function calls?

[Tronic]

@vltr The accessing protocol members from Http is only a temporary workaround until tests are passing and the code can be cleaned up some more. I also considered sans-I/O implementation but async I/O calls allow for automatic flow control without extra tasks for copying data, and presumably higher performance.

After cleanup you should be able to insert mock functions send and receive_more for unit testing the Http class, without dependencies to Protocol (or to asyncio, for that matter).

[Tronic]

For some reason on Windows the payload too large test fails because the test client cannot read the response. I suspected a deadlock between writing request and reading response blocking but increasing buffer sizes didn't help, and I can't currently think of other reasons why this might be happening.

[Tronic]

I've traced how that goes: the HTTP handler function writes its response to asyncio protocol and exits cleanly, after which transport.close() is called. According to docs this function should write out any pending data before actually closing.

My bet is that this failure occurs because httpx is still trying to send the request and doesn't notice that there is a response coming. Adding a delay allows for the request to flow into Sanic's receive buffer, after which httpx manages to read the response without any error. I do wonder how this is supposed to be solved...

[Tronic]

Yep, confirmed working, although that is not really a fix, just a hack around httpx in this particular test where the request happens to fit in network buffers despite being larger than request maximum size.

[Tronic]

Deadlocks are possible in bidirectional streaming handlers if the HTTP client does not support streaming but insists on sending all of request before reading any response. This will lead to request timeout but Sanic could also detect when this happens and issue a proper diagnostic. Unfortunately the problem itself cannot be avoided but it is up to the application whether or not to implement this kind of handlers.

This does not affect:

• Normal handling (because all of request is read first)
• Streaming in most cases

Non-streaming clients fail:

• When request is too large: we won't receive infinite amounts of data
  • Sanic instantly sends a 413 PayloadTooLarge and disconnects (all-ok)
  • Client gets a network error on send and never reads the response
• Streaming handlers with large request and large response
  • Small request or small response fits in network buffers and avoids the deadlock.
  • Client and server both stuck trying to send until response timeout

The problems are not new to this branch, and also occur with any other HTTP server implementing this functionality. Notably, browsers don't do bidirectional streaming but they have large network buffers to avoid any issues with moderately sized bodies. Curl, Nginx, Express and others implement proper bidirectional streaming.

[ahopkins]

@Tronic Can you squash these commits?

[Tronic]

@ahopkins You mean rebase and force-push? And all of them or just the merge commits? There's further work to be done on this prior to merge to master, so probably such merging should only be done then; and I believe this can be done more cleanly in Github UI while merging.

[Tronic]

Since the new policy is not to introduce any breaking changes in .09 and .12 releases, I guess this should target 20.06 (due to the delay with 20.03 I would been more comfortable with 20.09). To make that happen, I'll need help with proper code review, API discussion and writing comprehensive tests. Is anyone up for that?

Also, can I expect 20.06 to be delayed at least until the end for June, or better yet, July?

[ahopkins]

1. I will help with the review
2. Ideally we'd be able to publish notice with some warnings. Not really applicable for a change like this.
3. Our choice is to make it for this next release, or wait until March.
4. Given the nature of this past spring release, and the short notice on declaring depreciation schedule, I'm okay with releasing in July. @huge-success/sanic-release-managers? If there is consensus, I'll post to the Sanic front page with an announcement.

I think this is a big enough change that as many @huge-success/sanic-core-devs thay put their eyes on this and chime in the better.

[Tronic]

Closing PR as development is moved to huge-success/sanic:streaming