Skip to content

Commit

Permalink
Fix asyncio.CancelledError again (#6719) (#6727)
Browse files Browse the repository at this point in the history
* 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)
  • Loading branch information
H--o-l authored and patchback[bot] committed May 8, 2022
1 parent 46b9a76 commit ea15256
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 2 deletions.
1 change: 1 addition & 0 deletions CHANGES/6719.bugfix
@@ -0,0 +1 @@
Fix regression where ``asyncio.CancelledError`` occurs on client disconnection.
2 changes: 0 additions & 2 deletions aiohttp/web_protocol.py
Expand Up @@ -311,8 +311,6 @@ def connection_lost(self, exc: Optional[BaseException]) -> None:
exc = ConnectionResetError("Connection lost")
self._current_request._cancel(exc)

if self._task_handler is not None:
self._task_handler.cancel()
if self._waiter is not None:
self._waiter.cancel()

Expand Down

0 comments on commit ea15256

Please sign in to comment.