Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better request cancel handling #2513

Merged
merged 7 commits into from
Sep 19, 2022
Merged

Better request cancel handling #2513

merged 7 commits into from
Sep 19, 2022

Conversation

ahopkins
Copy link
Member

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.

@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

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

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%) ⬆️
sanic/exceptions.py 98.924% <100.000%> (+0.035%) ⬆️
sanic/http/http1.py 85.211% <100.000%> (ø)
sanic/models/server_types.py 93.023% <100.000%> (+0.166%) ⬆️
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.

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

@ChihweiLHBird
Copy link
Member

ChihweiLHBird commented Aug 1, 2022

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

@ahopkins
Copy link
Member Author

ahopkins commented Aug 1, 2022

I need to add a few more unit tests.

ChihweiLHBird
ChihweiLHBird previously approved these changes Aug 1, 2022
@ahopkins ahopkins marked this pull request as ready for review September 18, 2022 20:38
@ahopkins ahopkins requested a review from a team as a code owner September 18, 2022 20:38
sanic/http/http1.py Outdated Show resolved Hide resolved
sanic/http/http1.py Outdated Show resolved Hide resolved
@ahopkins ahopkins merged commit 389363a into main Sep 19, 2022
@ahopkins ahopkins deleted the request-cancelled branch September 19, 2022 13:04
@Tronic
Copy link
Member

Tronic commented Sep 19, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants