Fix a warning about unfinished task in web_protocol.py by asvetlov · Pull Request #4415 · aio-libs/aiohttp

asvetlov · 2019-11-30T18:12:18Z

webknjaz · 2019-11-30T21:40:15Z

Is it possible to test this?

asvetlov · 2019-11-30T22:19:32Z

Not sure.
Now I see that the fix is incorrect at least.

semiworks · 2020-09-09T07:10:55Z

I think the problem here is, that the RequestHandler.start() function hangs when waiting for self._waiter and the start() function will never finish. In connection_lost() we should cancel self._waiter to let the RequestHandler.start() function return.

diff --git a/libs/aiohttp/aiohttp/web_protocol.py b/libs/aiohttp/aiohttp/web_protocol.py
index 7178781e..8404880e 100644
--- a/libs/aiohttp/aiohttp/web_protocol.py
+++ b/libs/aiohttp/aiohttp/web_protocol.py
@@ -277,6 +277,9 @@ class RequestHandler(BaseProtocol):
         if self._error_handler is not None:
             self._error_handler.cancel()

+        if self._waiter:
+            self._waiter.cancel()
+
         self._task_handler = None

         if self._payload_parser is not None:

asvetlov · 2020-10-18T15:19:48Z

@semiworks you are right, I've updated the patch

aio-libs-github-bot · 2020-10-21T09:34:25Z

💔 Backport was not successful

The PR was attempted backported to the following branches:

❌ 3.7: Commit could not be cherrypicked due to conflicts

`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 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()`.

`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. fixes aio-libs#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

* 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 <[email protected]>

* 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 <[email protected]> (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 <[email protected]> (cherry picked from commit adeece3) Co-authored-by: Hoel IRIS <[email protected]>

Fix a warning about unfinished task in web_protocol.py

36de4a0

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 30, 2019

webknjaz approved these changes Nov 30, 2019

View reviewed changes

webknjaz marked this pull request as draft April 24, 2020 21:48

webknjaz self-requested a review April 24, 2020 21:48

asvetlov added 2 commits October 18, 2020 18:18

More

b00a7c6

Merge branch 'master' into fix-warning

e5c4be3

Fix test

3d13448

asvetlov marked this pull request as ready for review October 21, 2020 07:47

Merge branch 'master' into fix-warning

dc3c6e5

asvetlov added the backport-3.7 label Oct 21, 2020

asvetlov merged commit 6319c25 into master Oct 21, 2020

asvetlov deleted the fix-warning branch October 21, 2020 09:34

asvetlov added a commit that referenced this pull request Oct 21, 2020

Fix a warning about unfinished task in web_protocol.py (#4415)

69de6fe

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

CancelledError #6719

Closed

1 task

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

Fix asyncio.CancelledError again (#6719) #6727

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix a warning about unfinished task in web_protocol.py#4415

Fix a warning about unfinished task in web_protocol.py#4415
asvetlov merged 5 commits intomasterfrom
fix-warning

asvetlov commented Nov 30, 2019

Uh oh!

webknjaz commented Nov 30, 2019

Uh oh!

asvetlov commented Nov 30, 2019

Uh oh!

semiworks commented Sep 9, 2020

Uh oh!

asvetlov commented Oct 18, 2020

Uh oh!

aio-libs-github-bot bot commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

asvetlov commented Nov 30, 2019

Uh oh!

webknjaz commented Nov 30, 2019

Uh oh!

asvetlov commented Nov 30, 2019

Uh oh!

semiworks commented Sep 9, 2020

Uh oh!

asvetlov commented Oct 18, 2020

Uh oh!

aio-libs-github-bot bot commented Oct 21, 2020

💔 Backport was not successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants