Fix asyncio.CancelledError again (#6719) #6727

H--o-l · 2022-04-29T14:01:17Z

asyncio.CancelledError() on peer disconnection have been removed by #4080,
but #4415 re-introduced it silently.

self._waiter.cancel() and self._task_handler.cancel() were added by #4415,
but #4415 in fact only needed self._waiter.cancel() (proof below).

So I propose to remove self._task_handler.cancel(), both #4080 and #4415 will
be fixed.

To test that I re-resolved #4080 I used:

async def handle(request):
    try:
        await asyncio.sleep(5)
        await request.text()
    except BaseException as e:
        print(f'base exception {type(e).__name__}')
    return web.Response(text='toto')

curl -X POST -H "Content-Type: text/plain" --data "bouh" localhost:8080

I kill the curl request before the 5 seconds of sleep.
Before this PR I have the following error right after killing the curl:

$ python -Wall -X dev test.py
======== Running on http://0.0.0.0:8080 ========
(Press CTRL+C to quit)
base exception CancelledError

After this commit I have the following error, but only after the 5 seconds
sleep:

$ python -Wall -X dev test.py
======== Running on http://0.0.0.0:8080 ========
(Press CTRL+C to quit)
base exception ConnectionResetError

To test that I didn't re-introduce #4415 I use a basic handle and 30
curl localhost:8080:

Before this commit no issue
If I remove self._task_handler.cancel() no issue

If I remove both self._task_handler.cancel() and self._waiter.cancel():

$ python -Wall -X dev test.py
======== Running on http://0.0.0.0:8080 ========
(Press CTRL+C to quit)
Task was destroyed but it is pending!
task: <Task pending name='Task-4' coro=<RequestHandler.start() done, defined at ...
Task was destroyed but it is pending!
...

So it's OK to remove only self._task_handler.cancel().

There is no documentation or tests to be updated because #4080 already did that
part.

However I guess that for a few people it might be seen as a breaking change,
I'm not sure.

Tested on master and on v3.8.1.

fixes #6719

`asyncio.CancelledError()` on peer disconnection have been removed by aio-libs#4080, but aio-libs#4415 re-introduced it silently. `self._waiter.cancel()` and `self._task_handler.cancel()` were added by aio-libs#4415, but aio-libs#4415 in fact only needed `self._waiter.cancel()` (proof below). So I propose to remove `self._task_handler.cancel()`, both aio-libs#4080 and aio-libs#4415 will be fixed. To test that I re-resolved aio-libs#4080 I used: ```py async def handle(request): try: await asyncio.sleep(5) await request.text() except BaseException as e: print(f'base exception {type(e).__name__}') return web.Response(text='toto') ``` ```console curl -X POST -H "Content-Type: text/plain" --data "bouh" localhost:8080 ``` I kill the curl request before the 5 seconds of sleep. Before this PR I have the following error right after killing the curl: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception CancelledError ``` After this commit I have the following error, but only after the 5 seconds sleep: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception ConnectionResetError ``` To test that I didn't re-introduce aio-libs#4415 I use a basic `handle` and 30 `curl localhost:8080`: - Before this commit no issue - If I remove `self._task_handler.cancel()` no issue - If I remove both `self._task_handler.cancel()` and `self._waiter.cancel()`: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) Task was destroyed but it is pending! task: <Task pending name='Task-4' coro=<RequestHandler.start() done, defined at ... Task was destroyed but it is pending! ... ``` So it's OK to remove only `self._task_handler.cancel()`. There is no documentation or tests to be updated because aio-libs#4080 already did that part. However I guess that for a few people it might be seen as a breaking change, I'm not sure. Tested on master and on v3.8.1. fixes aio-libs#6719

H--o-l · 2022-05-02T09:16:26Z

Hey!

Here is an available reviewer for this PR?

PS: CI seems to fail but I don't understand what is failing (I'm not sure it's related to this PR).

H--o-l · 2022-05-04T08:09:26Z

Hey @Dreamsorcerer!

Any chance you have a bit of time to look at this PR? It's the one for the issue #6719 we discussed together.

Have a great day!

Dreamsorcerer · 2022-05-08T11:26:40Z

Excellent, thanks.

codecov · 2022-05-08T11:36:26Z

Codecov Report

Merging #6727 (1cd798d) into master (0215cdd) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6727      +/-   ##
==========================================
+ Coverage   93.36%   93.44%   +0.08%     
==========================================
  Files         104      104              
  Lines       30631    30629       -2     
  Branches     3077     3076       -1     
==========================================
+ Hits        28599    28622      +23     
+ Misses       1859     1838      -21     
+ Partials      173      169       -4

Flag	Coverage Δ
unit	`93.36% <ø> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/web_protocol.py	`89.68% <ø> (+1.36%)`	⬆️
tests/test_web_functional.py	`98.03% <0.00%> (+0.07%)`	⬆️
tests/test_client_functional.py	`98.49% <0.00%> (+0.29%)`	⬆️
tests/test_client_ws_functional.py	`98.23% <0.00%> (+0.96%)`	⬆️
tests/test_web_websocket_functional.py	`91.09% <0.00%> (+1.22%)`	⬆️

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 0215cdd...1cd798d. Read the comment docs.

patchback · 2022-05-08T11:40:57Z

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/adeece3c1826b150f129e198f69e78a469901b5e/pr-6727

Backported as #6741

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* Fix asyncio.CancelledError again (#6719) `asyncio.CancelledError()` on peer disconnection have been removed by #4080, but #4415 re-introduced it silently. `self._waiter.cancel()` and `self._task_handler.cancel()` were added by #4415, but #4415 in fact only needed `self._waiter.cancel()` (proof below). So I propose to remove `self._task_handler.cancel()`, both #4080 and #4415 will be fixed. To test that I re-resolved #4080 I used: ```py async def handle(request): try: await asyncio.sleep(5) await request.text() except BaseException as e: print(f'base exception {type(e).__name__}') return web.Response(text='toto') ``` ```console curl -X POST -H "Content-Type: text/plain" --data "bouh" localhost:8080 ``` I kill the curl request before the 5 seconds of sleep. Before this PR I have the following error right after killing the curl: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception CancelledError ``` After this commit I have the following error, but only after the 5 seconds sleep: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception ConnectionResetError ``` To test that I didn't re-introduce #4415 I use a basic `handle` and 30 `curl localhost:8080`: - Before this commit no issue - If I remove `self._task_handler.cancel()` no issue - If I remove both `self._task_handler.cancel()` and `self._waiter.cancel()`: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) Task was destroyed but it is pending! task: <Task pending name='Task-4' coro=<RequestHandler.start() done, defined at ... Task was destroyed but it is pending! ... ``` So it's OK to remove only `self._task_handler.cancel()`. There is no documentation or tests to be updated because #4080 already did that part. However I guess that for a few people it might be seen as a breaking change, I'm not sure. Tested on master and on v3.8.1. fixes #6719 * Update 6719.bugfix Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit adeece3)

patchback · 2022-05-08T11:41:06Z

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/adeece3c1826b150f129e198f69e78a469901b5e/pr-6727

Backported as #6742

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* Fix asyncio.CancelledError again (#6719) `asyncio.CancelledError()` on peer disconnection have been removed by #4080, but #4415 re-introduced it silently. `self._waiter.cancel()` and `self._task_handler.cancel()` were added by #4415, but #4415 in fact only needed `self._waiter.cancel()` (proof below). So I propose to remove `self._task_handler.cancel()`, both #4080 and #4415 will be fixed. To test that I re-resolved #4080 I used: ```py async def handle(request): try: await asyncio.sleep(5) await request.text() except BaseException as e: print(f'base exception {type(e).__name__}') return web.Response(text='toto') ``` ```console curl -X POST -H "Content-Type: text/plain" --data "bouh" localhost:8080 ``` I kill the curl request before the 5 seconds of sleep. Before this PR I have the following error right after killing the curl: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception CancelledError ``` After this commit I have the following error, but only after the 5 seconds sleep: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception ConnectionResetError ``` To test that I didn't re-introduce #4415 I use a basic `handle` and 30 `curl localhost:8080`: - Before this commit no issue - If I remove `self._task_handler.cancel()` no issue - If I remove both `self._task_handler.cancel()` and `self._waiter.cancel()`: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) Task was destroyed but it is pending! task: <Task pending name='Task-4' coro=<RequestHandler.start() done, defined at ... Task was destroyed but it is pending! ... ``` So it's OK to remove only `self._task_handler.cancel()`. There is no documentation or tests to be updated because #4080 already did that part. However I guess that for a few people it might be seen as a breaking change, I'm not sure. Tested on master and on v3.8.1. fixes #6719 * Update 6719.bugfix Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit adeece3)

* Fix asyncio.CancelledError again (#6719) `asyncio.CancelledError()` on peer disconnection have been removed by #4080, but #4415 re-introduced it silently. `self._waiter.cancel()` and `self._task_handler.cancel()` were added by #4415, but #4415 in fact only needed `self._waiter.cancel()` (proof below). So I propose to remove `self._task_handler.cancel()`, both #4080 and #4415 will be fixed. To test that I re-resolved #4080 I used: ```py async def handle(request): try: await asyncio.sleep(5) await request.text() except BaseException as e: print(f'base exception {type(e).__name__}') return web.Response(text='toto') ``` ```console curl -X POST -H "Content-Type: text/plain" --data "bouh" localhost:8080 ``` I kill the curl request before the 5 seconds of sleep. Before this PR I have the following error right after killing the curl: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception CancelledError ``` After this commit I have the following error, but only after the 5 seconds sleep: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) base exception ConnectionResetError ``` To test that I didn't re-introduce #4415 I use a basic `handle` and 30 `curl localhost:8080`: - Before this commit no issue - If I remove `self._task_handler.cancel()` no issue - If I remove both `self._task_handler.cancel()` and `self._waiter.cancel()`: ```console $ python -Wall -X dev test.py ======== Running on http://0.0.0.0:8080 ======== (Press CTRL+C to quit) Task was destroyed but it is pending! task: <Task pending name='Task-4' coro=<RequestHandler.start() done, defined at ... Task was destroyed but it is pending! ... ``` So it's OK to remove only `self._task_handler.cancel()`. There is no documentation or tests to be updated because #4080 already did that part. However I guess that for a few people it might be seen as a breaking change, I'm not sure. Tested on master and on v3.8.1. fixes #6719 * Update 6719.bugfix Co-authored-by: Sam Bull <aa6bs0@sambull.org> (cherry picked from commit adeece3) Co-authored-by: Hoel IRIS <hoel.iris@gmail.com>

webknjaz · 2022-09-20T17:28:55Z

@Dreamsorcerer I was under impression that handler cancellation was supposed to have stopped happening only with aiohttp 4+. Is this something else?

webknjaz · 2022-09-20T17:35:18Z

OTOH this seems to have been intended for 3.7, after all: #4771.

Dreamsorcerer · 2022-09-20T19:46:18Z

Yeah, it was in 3.7, then got accidentally disabled in 3.8, causing a regression.

… when client connection closing. (#7056)  ## Related to #6719 #6727. Added a configuration flag for enable request task handler cancelling when client connection closing. After changes in version 3.8.3, there is no longer any way to enable this behaviour. In our services, we want to handle protocol-level errors, for example for canceling the execution of a heavy query in the DBMS if the user's connection is broken. Now I created this PR in order to discuss my solution, of course if I did everything well I will add tests changelog, etc.  ## I guess this breakdown can be solved using the configuration flag that is passed to the Server instance. Of course `AppRunner` and `SiteRunner` can pass this through `**kwargs` too.  ## Related issue number #6719  ## Checklist - [ ] I think the code is well written - [ ] Unit tests for the changes exist - [ ] Documentation reflects the changes - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Sam Bull <aa6bs0@sambull.org>

H--o-l requested a review from asvetlov as a code owner April 29, 2022 14:01

H--o-l force-pushed the fix/cancelled-error branch from b935d8e to 4045423 Compare April 29, 2022 14:03

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 29, 2022

H--o-l mentioned this pull request Apr 29, 2022

CancelledError #6719

Closed

1 task

Update 6719.bugfix

1cd798d

Dreamsorcerer added backport-3.8 backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot labels May 8, 2022

Dreamsorcerer enabled auto-merge (squash) May 8, 2022 11:26

Dreamsorcerer merged commit adeece3 into aio-libs:master May 8, 2022

patchback bot mentioned this pull request May 8, 2022

[PR #6727/adeece3c backport][3.8] Fix asyncio.CancelledError again (#6719) #6741

Merged

patchback bot mentioned this pull request May 8, 2022

[PR #6727/adeece3c backport][3.9] Fix asyncio.CancelledError again (#6719) #6742

Merged

H--o-l deleted the fix/cancelled-error branch May 8, 2022 12:51

pjknkda mentioned this pull request Sep 21, 2022

Peer cancellation policy documentation unclear #5814

Closed

1 task

mosquito mentioned this pull request Oct 28, 2022

Added a configuration flag for enable request task handler cancelling when client connection closing. #7056

Merged

5 tasks

Shulyaka mentioned this pull request Oct 24, 2023

Automation is cancelled on API client disconnect home-assistant/core#101751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix asyncio.CancelledError again (#6719) #6727

Fix asyncio.CancelledError again (#6719) #6727

H--o-l commented Apr 29, 2022

H--o-l commented May 2, 2022

H--o-l commented May 4, 2022

Dreamsorcerer commented May 8, 2022

codecov bot commented May 8, 2022 •

edited

patchback bot commented May 8, 2022 •

edited

patchback bot commented May 8, 2022 •

edited

webknjaz commented Sep 20, 2022

webknjaz commented Sep 20, 2022

Dreamsorcerer commented Sep 20, 2022

Fix asyncio.CancelledError again (#6719) #6727

Fix asyncio.CancelledError again (#6719) #6727

Conversation

H--o-l commented Apr 29, 2022

H--o-l commented May 2, 2022

H--o-l commented May 4, 2022

Dreamsorcerer commented May 8, 2022

codecov bot commented May 8, 2022 • edited

Codecov Report

patchback bot commented May 8, 2022 • edited

Backport to 3.8: 💚 backport PR created

patchback bot commented May 8, 2022 • edited

Backport to 3.9: 💚 backport PR created

webknjaz commented Sep 20, 2022

webknjaz commented Sep 20, 2022

Dreamsorcerer commented Sep 20, 2022

codecov bot commented May 8, 2022 •

edited

patchback bot commented May 8, 2022 •

edited

patchback bot commented May 8, 2022 •

edited