Skip to content

Conversation

@patchback
Copy link
Contributor

@patchback patchback bot commented May 8, 2022

This is a backport of PR #6727 as merged into master (adeece3).

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

* 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)
@patchback patchback bot requested a review from asvetlov as a code owner May 8, 2022 11:41
@Dreamsorcerer Dreamsorcerer enabled auto-merge (squash) May 8, 2022 11:44
@Dreamsorcerer Dreamsorcerer merged commit 2c2ba54 into 3.9 May 8, 2022
@Dreamsorcerer Dreamsorcerer deleted the patchback/backports/3.9/adeece3c1826b150f129e198f69e78a469901b5e/pr-6727 branch May 8, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants