Refs #36315 -- Replaced manual task and cancellation handling with TaskGroup in ASGIHandler.

[graingert]

Trac ticket number

ticket-36315

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.

Checklist

• This PR targets the main branch.
• The commit message is written in past tense, mentions the ticket number, and ends with a period.
• I have checked the "Has patch" ticket flag in the Trac system.
• I have added or updated relevant tests.
• I have added or updated relevant docs, including release notes if applicable.
• I have attached screenshots in both light and dark modes for any UI changes.

[hartungstenio]

Wouldn't simply cancelling the background task achieve the same result, without triggering the exception handling mechanism. It would also make this code more readable, since it seems like the exception is being raised only to cancel the task.

                    async with asyncio.TaskGroup() as tg:
                        disconnect_task = tg.create_task(self.listen_for_disconnect(receive))
                        response = await self.run_get_response(request)
                        await self.send_response(response, send)
                        disconnect_task.cancel()
                except* RequestAborted:
                    pass

[graingert]

This is the recommended way to terminate a TaskGroup https://docs.python.org/3/library/asyncio-task.html#terminating-a-task-group

[hartungstenio]

The recommendation is to create an "exception-raising task".

But in this case, semantically, we don't want to end the TaskGroup (like, there are many tasks running, and want to finish them all), we want to stop listening for disconnections (which happens to be the only running task).

[carltongibson]

Hey @graingert. Thanks for your patience here. I finally got the asgiref, Channels, Daphne, etc backlog worked through and your PRs here are next on my list.

[carltongibson]

OK, yep. I think this makes sense. Thanks @graingert, for both the effort and the patience. 👍

[Arfey]

We can remove body_file.close()

#optional

[graingert]

I want the body to close before waiting for send_response in this case

[Arfey]

should we add CancelledError here?

[graingert]

No it will be consumed by the TaskGroup

[Arfey]

Looks like no

import asyncio


class RequestProcessed(Exception):
    pass


class RequestAborted(Exception):
    pass


async def listen_for_disconnect():
    await asyncio.sleep(10)


async def run_get_response():
    await asyncio.sleep(0.2)


async def handle_request():
    print("[handler] start")
    try:
        try:
            async with asyncio.TaskGroup() as tg:
                tg.create_task(listen_for_disconnect())
                await run_get_response()
                raise RequestProcessed()
        except* (RequestProcessed, RequestAborted):
            print("[handler] handled RequestProcessed/Aborted")
    except BaseExceptionGroup as eg:
        print(f"[handler] caught BaseExceptionGroup: {eg!r}")
        if len(eg.exceptions) == 1:
            raise eg.exceptions[0]
        raise

    print("send signal about end request")


async def main():
    task = asyncio.create_task(handle_request())

    # Cancel after a short delay to simulate disconnect
    await asyncio.sleep(0.1)
    print("[main] cancelling request...")
    task.cancel()

    try:
        await task
    except asyncio.CancelledError:
        print("[main] request was cancelled (outside)")

asyncio.run(main())

output

[handler] start
[main] cancelling request...
[main] request was cancelled (outside)

example with asyncio.CancelledError

async def handle_request():
    print("[handler] start")
    try:
        try:
            async with asyncio.TaskGroup() as tg:
                tg.create_task(listen_for_disconnect())
                await run_get_response()
                raise RequestProcessed()
        except* (RequestProcessed, RequestAborted, asyncio.CancelledError):
            print("[handler] handled RequestProcessed/Aborted")
    except BaseExceptionGroup as eg:
        print(f"[handler] caught BaseExceptionGroup: {eg!r}")
        if len(eg.exceptions) == 1:
            raise eg.exceptions[0]
        raise

    print("send signal about end request")

output

[handler] start
[main] cancelling request...
[handler] handled RequestProcessed/Aborted
send signal about end request

[graingert]

This CancelledError should be propagated because it comes from a manual cancel call unrelated to the TaskGroup. Generally you should not catch CancelledError and replace it unless you caused the cancellation

[Arfey]

First of all, thank you for your MR. It will definitely help me in the future 😌

You’re right about the propagation. So, the better approach is probably to wrap everything in a try/finally block:

try:
    ...
finally:
    if response is None:
        await signals.request_finished.asend(sender=self.__class__)
    else:
        await sync_to_async(response.close)()

The request_finished signal is used for connection cleanup, and if I don’t release the connection back to the pool, it could cause issues later.

what do u think?

[Arfey]

I don't think we should revert the current MR (it doesn't break the existing codebase), but we need to find a proper approach for handling the cleanup. We can move this discussion elsewhere.

[carltongibson]

OK, so the signals.request_finished.asend is still hit on the tests checking cancellation.

e.g. see asgi.tests.ASGITest.test_asyncio_cancel_error.

@Arfey If you could add a test case demonstrating a change in behaviour then we can certainly tweak.

[Arfey]

Thanks.

The asgi.tests.ASGITest.test_asyncio_cancel_error covers two cases:

• The request cycle completes normally when no disconnect is sent.
• The request cycle is interrupted when a disconnect occurs before the view responds.

So, it tests cancellation caused by a disconnection event. However, we discovered a potential cleanup issue if something higher up cancels handle_request.

demonstrating a change in behaviour then we can certainly tweak.

The behaviour remains the same. We already had the same potential issue in the previous version of the code.

If you could add a test case

Sure

[Arfey]

https://github.com/Arfey/django/pull/2/files

It's not a problem for now, since we don't use async database connections. The threaded version doesn't have this issue, as it handles connection closing both before and after each request. But for pure async connections, it could become an issue.

[carltongibson]

Great. I'll look at that. Thanks.

Not sure how such a case would come up currently. We can likely discuss elsewhere as you say.