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

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

Conversation

patchback[bot]
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 <aa6bs0@sambull.org>
(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:43
@Dreamsorcerer Dreamsorcerer merged commit 20c9365 into 3.8 May 8, 2022
@Dreamsorcerer Dreamsorcerer deleted the patchback/backports/3.8/adeece3c1826b150f129e198f69e78a469901b5e/pr-6727 branch May 8, 2022 22:06
@H--o-l
Copy link
Contributor

H--o-l commented May 13, 2022

@Dreamsorcerer Thanks for the review and merge! A release of aiohttp v3.8.2 with this change would be useful for me in production, do you have an expected release date for aiohttp v3.8.2?

@Dreamsorcerer
Copy link
Member

I've never done a release for aiohttp before, so I'm hoping someone else will come back to it. If they're still away in a while, then I'll try and figure out a 3.8 release.

@H--o-l
Copy link
Contributor

H--o-l commented Jun 8, 2022

Hi @Dreamsorcerer!

It's been a month since the merge of this PR, maybe a new release could be considered?

(Sorry to bother you with that, thanks again for your help on the subject)

@H--o-l
Copy link
Contributor

H--o-l commented Jul 25, 2022

Hi, @Dreamsorcerer @asvetlov!

Sorry to bother you, but can you tell me if there is still no new released planned in the coming weeks?

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.

None yet

2 participants