Don't swallow asyncio.CancelledError

[Olegt0rr]

What do these changes do?

Makes this lib use more expectable behaviour. Read more info in related issue.

Are there changes in behavior for the user?

No

Related issue number

Fixes #458

Checklist

• [x] I think the code is well written
• [x] Unit tests for the changes exist
• [x] Documentation reflects the changes

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (86e103c) 100.00% compared to head (f7811a9) 100.00%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #459 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 4 4 Lines 454 474 +20 Branches 52 54 +2 ========================================= + Hits 454 474 +20 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Dreamsorcerer]

Now, the question is whether we can figure out a test for this. I'm curious that the coverage is already 100% though. Maybe if we can figure out which test is hitting that line, we can just tweak the asserts in that test?

[Olegt0rr]

Now, the question is whether we can figure out a test for this. I'm curious that the coverage is already 100% though. Maybe if we can figure out which test is hitting that line, we can just tweak the asserts in that test?

Just found test called test_ping_auto_close EventSourceResponse.__aexit__ calls await self.wait() and then our condition makes True

Cause there was raised an exception

[Olegt0rr]

I'll add additional test to explicitly check swallowing case

[Dreamsorcerer]

Just found test called test_ping_auto_close

The client closes the connection immediately (after receiving headers, which is done when entering async with), which then causes the handler to get cancelled. So, that's why it gets cancelled. Because the transport has already been closed though, the first time it will yield to the event loop in order for the cancellation to happen is when exiting the async with, which is why it still has 100% coverage.

[Olegt0rr]

@Dreamsorcerer ,

I prepared test. But I need help))

My test passes with changes from this PR. Without changes it hangs all testing process.

I've tried to add timeout for pytest... After timeout test passes :(

Dunno how to make it fail on timeout

async def test_cancelled_not_swallowed(aiohttp_client: ClientFixture) -> None:
    """Test asyncio.CancelledError is not swallowed by .wait().

    Relates to:
    https://github.com/aio-libs/aiohttp-sse/issues/458
    """
    async def endless_task(sse: EventSourceResponse) -> None:
        while True:
            await sse.wait()

    async def handler(request: web.Request) -> EventSourceResponse:
        async with sse_response(request) as sse:
            task = asyncio.create_task(endless_task(sse))
            await asyncio.sleep(0)
            task.cancel()

            await task

        return sse  # pragma: no cover

    app = web.Application()
    app.router.add_route("GET", "/", handler)

    client = await aiohttp_client(app)

    async with client.get("/") as response:
        assert 200 == response.status

[Dreamsorcerer]

Without changes it hangs all testing process.

Now you see why it's a problem. :P A timeout uses cancellation, so the old code is swallowing and there is no way you can write a cleanly failing test.

A timeout around the client.get() code will ensure the client request is cancelled and the test function reaches the end, but that handler will never stop running, which may cause it to hang on cleanup.

So, this test is probably fine as it is.

[Dreamsorcerer]

In order to assert something properly though, you probably want to nonlocal task and then assert task.cancelled() or something at the end of the test.

[Olegt0rr]

This test never passes if swallowing was turned on again and if pytest timeout will be always unset. But if for some reason pytest timeout will be set - this test will become useless, cause it will be anyway green

[Dreamsorcerer]

This test never passes if swallowing was turned on again and if pytest timeout will be always unset. But if for some reason pytest timeout will be set - this test will become useless, cause it will be anyway green

Not sure if I understand you. It's expected that the test will never complete. If a pytest timeout is used, it should certainly fail the CI if the test hasn't completed. But, I wouldn't worry about that too much.

[Olegt0rr]

If a pytest timeout is used, it should certainly fail

How to make this? I have all green tests with timeout setting. After timeout this test become marked as passed.

[Dreamsorcerer]

If a pytest timeout is used, it should certainly fail

How to make this? I have all green tests with timeout setting. After timeout this test become marked as passed.

Well, I'm not sure what timeout you're using. pytest doesn't have a timeout feature by default. The CI is also set to timeout after 15 mins anyway, so I wouldn't worry about adding a timeout directly in the test.

[Olegt0rr]

So, this test will hang every pipeline for 15 mins? 0_o

I've tried: https://pypi.org/project/pytest-timeout/

[Dreamsorcerer]

So, this test will hang every pipeline for 15 mins? 0_o

Oh, I forgot that we only fixed in 3.11+. Just add a @pytest.mark.skipif(sys.version_info < (3, 11), reason=".cancelling() missing in older versions")

[Olegt0rr]

Ah, already added :)