Don't cancel self

[codecov[bot]]

Codecov Report

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

Comparison is base (7fdb432) 100.00% compared to head (aade6b7) 100.00%.

:exclamation: Current head aade6b7 differs from pull request most recent head e39aee1. Consider uploading reports for the commit e39aee1 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #456 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 4 4 Lines 452 454 +2 Branches 52 52 ========================================= + Hits 452 454 +2 ```

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

[Dreamsorcerer]

@jameshilliard in https://github.com/aio-libs/aiohttp-sse/pull/370 mentioned some tracebacks from ping, but I can't reproduce it.

Ah, I'm duplicating myself. The main fix there, was that it caught the exception, which is still happening here. We just exit cleanly now, instead of trying to cancel itself, which is weird.

I also have an alternative suggestion

My gut feeling is that the exception should be handled inside the function, as a normal expectation of that code. This means that other code wouldn't need to know what exception to handle for the normal case. If new code starts awaiting the ping task, they would only need to worry about cancellation, which is a normal thing to worry about in asyncio tasks.

Also, talking about cancellation, the current behaviour is not correct. If we are awaiting on .wait() and the main task gets cancelled, then the cancellation would get suppressed, which is not the behaviour the user would expect. Would you like to take a look at this?

I think we just need to add a if asyncio.current_task().cancelling(): raise inside the wait() method, so it only suppresses cancellations on the ping task. We can probably just do that on Python 3.11+ to keep the implementation simple.

[Olegt0rr]

I think we just need to add a if asyncio.current_task().cancelling(): raise inside the wait() method, so it only suppresses cancellations on the ping task.

No-no-no

This ping task is made only for connection is not killed as not active (e.g. by reverse proxy). As a user of SSE, I don't need to handle any exceptions, cause I can't do anything with it.

Now I have 2 interfaces: await sse.wait() - blocking wait till connection become closed with any reason (browser close, network issues, etc.) sse.is_connected() - non-blocking check of the connection state (connected/disconnected)

As a user of this library, I don't need to bring try/except or suppress to my app for checking connection state.

some_sse_pool = []

async def sse_handler(request):
    """Handle sse and add to pool."""
    async with sse_response(request) as sse:
        # add sse to the pool, so other workers may use it
        some_sse_pool.append(sse)

        # now I just need to wait till connection is closed
        await sse.wait()

    # now I'm sure connection is closed
    some_sse_pool.remove(sse)

[Dreamsorcerer]

I'm not sure you've understood. The problem is that your program won't stop when you explicitly told it to stop.

async def my_task():
    while True:
        ...
        await sse_response.wait()

async def main():
    t = asyncio.create_task(my_task())
    await asyncio.sleep(0.5)
    t.cancel()
    with suppress(asyncio.CancelledError):
        await my_task()

This program will never exit.

[Dreamsorcerer]

No library should be swallowing CancelledError exceptions unconditionally like that, it will just break other people's applications.

[Olegt0rr]

I'm not sure you've understood. The problem is that your program won't stop when you explicitly told it to stop.

Your example is not that someone implemented in real application. You just don't need to make while True with .wait(), depending on method name, description and behaviour. Can you make any SSE-related example?

Philosophy

The _ping_task was made not for the user, but for the library (that's why the name starts with _). It starts implicitly and must ends implicitly, as if it never existed.

Every method called by the user should throw an exception if the method cannot do what it is supposed to do by name+description.

Method .send() sends data using EventSource protocol. If this method can't send data - it should throw an exception!

Method .wait() wait until connection will be closed or other task explicitly call .stop_streaming() method. If the method can't wait, it should throw an exception (for example, if sse has not yet been prepared). But if method is able to wait and connection become closed - it's not an error for this method, it's an expected behavior. At this point, we can consider that the method has fulfilled its purpose and can be completed.

About breaking compatibility

All these years, the library has been working with “Philosophy,” which I wrote about above. Suppression of canceled ping in .wait() was added by @ticosax in PR #20 (2017). Before this PR waiting was based on separate Future object.

So removing of suppression CancelledError in .wait() will break compatibility.

[Olegt0rr]

No library should be swallowing CancelledError exceptions unconditionally like that, it will just break other people's applications.

@Dreamsorcerer , I misunderstood your position and thins you wanna disable ping's suppression )))

Do you wanna make this?

    async def wait(self):
        """EventSourceResponse object is used for streaming data to the client,
        this method returns future, so we can wait until connection will
        be closed or other task explicitly call ``stop_streaming`` method.
        """
        if self._ping_task is None:
            raise RuntimeError("Response is not started")

        with contextlib.suppress(asyncio.CancelledError):
            await self._ping_task

        if asyncio.current_task().cancelling():
            raise asyncio.CancelledError

[Dreamsorcerer]

Exactly, yes. Cancelling .wait() or any task that is calling .wait() should not suppress the cancellation, only cancelling the ping task should get swallowed.