basic request streaming support with flow control

[yunstanford]

basic request streaming support, could be useful in cases like large file upload.

[codecov]

Codecov Report

Merging #1423 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1423      +/-   ##
==========================================
+ Coverage   84.62%   84.74%   +0.11%     
==========================================
  Files          17       17              
  Lines        1704     1717      +13     
  Branches      322      322              
==========================================
+ Hits         1442     1455      +13     
  Misses        203      203              
  Partials       59       59

Impacted Files	Coverage Δ
sanic/app.py	80.48% <ø> (ø)	⬆️
sanic/request.py	86.47% <100%> (+0.83%)	⬆️
sanic/config.py	100% <100%> (ø)	⬆️

---

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 aa0874b...956793e. Read the comment docs.

[yunstanford]

Anyone take a look ?

[abuckenheimer]

@yunstanford can review tomorrow

[harshanarayana]

Would it be possible to add a full-fledged example showcasing the use of this feature so that it can come handy to the end users?

[abuckenheimer]

hmmm I'm wondering if StreamBuffer is a leaky abstraction, I feel like a "Buffer" that defines a read method should take an n bytes argument detailing at most how many bytes the call should return. Without that this becomes a thin wrapper around a Queue and doesn't really look like a "Buffer" or support the same workflows.

This may be a misunderstanding of how on_body gets invoked on my part but it seems like buffer_size actually is a pretty loose limit because there is no control on how big each item on the queue is. I think this solution as stands is a good way of asking for "any amount of bytes as soon as they arrive" but I'm not sure it provides the finer grained traffic control you might expect from this streaming.

TBH I don't have a whole lot of experience with data streaming to the server so I may be overthinking the use case.

[yunstanford]

@abuckenheimer agree in general, but httptools doesn't provide the transport that we can control how many bytes to read from the socket. We can build another layer in StreamBuffer for read_n(bytes), but the data has been feed in memory already at that time.

This is a simple fix for #546, providing basic data flow control.

We can look into httptools later and see if we can do more around that, and vender it if needed.

[abuckenheimer]

Gotcha, I still think REQUEST_BUFFER_QUEUE_SIZE is kind of meaningless though, we need to restrict based on amounts of bytes in the queue somehow. Making an actual async buffer is kinda complicated, I'm fine with moving forward on this as long as there is some sort of "alpha/beta" marker on streaming incoming requests because I have to imagine the API will change.

[yunstanford]

yeah, the API doesn't need change as we can set default value for .read(bytes=100), but we need to document that in the future.

[abuckenheimer]

Each item could potentially be any size in bytes, how do you plan on rechunking the bytes across items in the queue?

Also ideally the default would be return all bytes like a real buffer. I realize this is kind of an anti pattern because your really no longer streaming the request but, I'd expect body = stream.read() to return the whole body. I think n=100 would be easier to misuse than the alternative.

[yunstanford]

We'll need introduce another layer in StreamBuffer for rechunking, but didn't think deep into this. and introduce another API like .read_any(bytes=some) or some similar if you think it's less confusing.

Also in reality, the common use case, like uploading large file, usually is in a loop, users might be not very interested in the exact bytes in each single read.

We can definitely look into improving this in the future as mentioned before (not small amount of work). and we can provide the functionality/support first.

[yunstanford]

@ahopkins any other feedbacks ?

[pypycoder]

@yunstanford @ahopkins Can we get this feature in next release ?

[ahopkins]

👍 Looks good to me.

[ahopkins]

@pypycoder Unfortunately not. We put a freeze on features a few weeks back for the 18.12 release (which is ready to ship). It should be added to 19.03

[andreymal]

@yunstanford @ahopkins this PR broke the graceful shutdown of streaming responses in 19.03.1

(Perhaps I'll create an issue or PR later)

UPD: #1558