feat: fixes exception due to unread bytes in stream

[imgurbot12]

Hi there! I noticed that the stream handlers for sanic seem to raise an exception when stream data is not read entirely or the request is cancelled for whatever reason, which was annoying me slightly so I found an easy fix and am hoping to get it merged into master.

I have a test script to replicate the error. There are two functions in the example server instance.

1. works will run just fine without any tracebacks because it reads all of the stream data
2. errror will raise a traceback due to the fact that the request object gets deleted after request completion, but the byte-reader is still active because there is still data in the buffer

Fix Description:
I fixed it by simply adding an additional check that will ignore any request operations if the request object is no longer present
allowing the remaining bytes in the buffer to simply be discarded.

Example Traceback:

Task exception was never retrieved
future: <Task finished name='Task-6' coro=<HttpProtocol.stream_append() done, defined at /home/andrew/Desktop/code/github/sanic/sanic/server.py:416> exception=AttributeError("'NoneType' object has no attribute 'stream'")>
Traceback (most recent call last):
  File "/home/andrew/Desktop/code/github/sanic/sanic/server.py", line 419, in stream_append
    if self.request.stream.is_full():
AttributeError: 'NoneType' object has no attribute 'stream'

Sample Script for Replication of Error:

from sanic import Sanic
from sanic.response import json

app = Sanic(__name__)

#NOTE: this will raise a traceback with the stream left unread
@app.route('/error', methods=['POST'], stream=True)
async def error(request):
    return json({'Hello': 'World!'})

#NOTE: this works just fine because the stream content is read completley
@app.route('/works', methods=['POST'], stream=True)
async def works(request):
    while True:
        body = await request.stream.read()
        if body is None:
            break
    return json({'Hello': 'World!'})

app.run(host='127.0.0.1', port=8000)

Sample Requests Made w/ Curl:

curl -d 'this is a test' -v 'http://127.0.0.1:8000/error'
curl -d 'this is a test' -v 'http://127.0.0.1:8000/works'

[codecov]

Codecov Report

Merging #1897 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1897   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files          27       27           
  Lines        3099     3100    +1     
  Branches      554      555    +1     
=======================================
+ Hits         2857     2858    +1     
  Misses        169      169           
  Partials       73       73           

Impacted Files	Coverage Δ
sanic/response.py	98.08% <ø> (ø)
sanic/router.py	97.07% <ø> (ø)
sanic/testing.py	96.25% <ø> (ø)
sanic/worker.py	82.78% <ø> (ø)
sanic/server.py	79.92% <100.00%> (+0.03%)	⬆️

---

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 58e4087...5890d0c. Read the comment docs.

[Tronic]

Note: this is already fixed in #1876 (streaming branch) which prints a proper diagnostic and no traceback:

[2020-07-26 13:50:46 +0300] [17097] [INFO] Goin' Fast @ http://127.0.0.1:8000
[2020-07-26 13:50:46 +0300] [17097] [INFO] Starting worker [17097]
[2020-07-26 13:50:56 +0300] - (sanic.access)[INFO][127.0.0.1:49252]: POST http://127.0.0.1:8000/error  200 -1
[2020-07-26 13:50:56 +0300] [17097] [ERROR] <Request: POST /error> body not consumed.
[2020-07-26 13:50:56 +0300] - (sanic.access)[INFO][127.0.0.1:49253]: POST http://127.0.0.1:8000/works  200 -1

This PR is still useful because the streaming branch is postponed until Sanic 21.03 or so.

[ahopkins]

Yes we still need a fix for the current branch to patch between 20.9 and 20.12 since that branch is set for 21.3

[Tronic]

@imgurbot12 Could you add tests so that this has 100 % patch coverage?

[imgurbot12]

Hi @Tronic, very sorry about the delay. I'm afraid I'm not very familiar with this sort of testing coverage benchmark. What tests do I need to improve/write to add coverage for the single line that I've added? I can't seem to find any tests in sanic/tests that actually run stream_append directly.

[Tronic]

Can you add another function in tests/test_request_stream.py that would hit this case? I think the test_request_stream_handle_exception function is similar to what you need to test, just give yours another name and modify so that it hits the bug and is handled in your patch.

Coverage comes automatically from all lines of code actually hit by at least a single test case.

[imgurbot12]

hrm. Well it seems like the unit-tests passed. I'm not sure whats wrong with the linter that's causing it to error now

[ahopkins]

Run make fix-import commit that and we should be good. I am going to bring this current to master. I saw that there was a recent update to black. Perhaps your PR just happens to be the first to run into those changes.

[imgurbot12]

@ahopkins That should hopefully be the last of the automated changes lol. Thanks

[ahopkins]

@ahopkins That should hopefully be the last of the automated changes lol. Thanks

Thanks for this. Will get it merged once the tests are done.

[imgurbot12]

@ahopkins That should hopefully be the last of the automated changes lol. Thanks

Thanks for this. Will get it merged once the tests are done.

Yeah no problem! Glad to help out :)

[Tronic]

Tests should use the test client whenever possible, so that they don't break when server's internal implementation is modified as long as the external functionality stays the same.