sanic icon indicating copy to clipboard operation
sanic copied to clipboard

Better request cancel handling

Open ahopkins opened this issue 1 year ago • 3 comments

Currently, there is no way to distinguish between the following:

SCENARIO A: Something in the handler raises a CancelledError

@app.get("/")
async def handler(request: Request):
    raise CancelledError

SCENARIO B: A client cancels a request in the middle of handling

@app.get("/")
async def handler(request: Request):
    for _ in range(10):
        print(".")
        await sleep(1)
    return json(None)

This is particularly noticeable and troublesome if there is an underlying library (like a DB driver) that decides to call CancelledError. It causes a 503 and there really is no way to catch this. The following will NOT work, which is somewhat counterintuitive.

@app.exception(CancelledError)
async def exc_handler(request: Request, exc):
    ...

The idea here is to introduce a new exception: RequestCancelled that will bubble up through the system. It should be catchable on its own, and also CancelledError should also be catchable. Furthermore, it should not create a response object and attempt to push the bytes if the client connection has been lost.

ahopkins avatar Jul 31 '22 19:07 ahopkins

Codecov Report

Base: 87.834% // Head: 87.844% // Increases project coverage by +0.009% :tada:

Coverage data is based on head (398e53e) compared to base (7f894c4). Patch coverage: 90.000% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##              main     #2513       +/-   ##
=============================================
+ Coverage   87.834%   87.844%   +0.009%     
=============================================
  Files           78        78               
  Lines         6461      6466        +5     
  Branches      1246      1247        +1     
=============================================
+ Hits          5675      5680        +5     
  Misses         560       560               
  Partials       226       226               
Impacted Files Coverage Δ
sanic/server/protocols/base_protocol.py 65.384% <66.666%> (+0.449%) :arrow_up:
sanic/exceptions.py 98.924% <100.000%> (+0.035%) :arrow_up:
sanic/http/http1.py 85.211% <100.000%> (ø)
sanic/models/server_types.py 93.023% <100.000%> (+0.166%) :arrow_up:
sanic/server/protocols/http_protocol.py 81.617% <100.000%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Jul 31 '22 19:07 codecov[bot]

It looks good to me. Let's make it ready and run the CI?

ChihweiLHBird avatar Aug 01 '22 05:08 ChihweiLHBird

I need to add a few more unit tests.

ahopkins avatar Aug 01 '22 06:08 ahopkins

Did you check how this affects ASGI, or how are connections lost with that anyway? The catching/handling logic in Sanic is extremely complicated but I see that you did a change to http1 but not to asgi, while both often need identical changes.

Tronic avatar Sep 19 '22 13:09 Tronic