[Django] #35148: Missed request cleanup for async views
Django
-----------------------------------------+------------------------
Reporter: Hugo Heyman | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Hello!
''I posted this on django-dev mailing list at first, which was probably
not the correct place for this (sorry), so I'm posting it here instead.''
I've been troubleshooting an issue with lingering idle db connections
(postgres) since upgrading from django 4.2 to 5.0. The issue manifested by
db raising OperationalError FATAL: sorry, too many clients already not
long after deploying the new version. We're running asgi with uvicorn +
gunicorn.
After extensive troubleshooting I installed django from a local repo and
ran a git bisect to pin down the commit where this first occured and ended
up on this commit
https://github.com/django/django/commit/64cea1e48f285ea2162c669208d95188b32bbc82
Since I've recently built something quite async heavy I decided to dig
into how the `ASGIHandler` code works and see if I can understand it and
possibly make a fix if there really is some issue there (I have never
found a real bug in django so I'm still having doubts :D).
The change introduced in Django 5.0 to allow handling of
`asyncio.CancelledError` in views seems to also allow async views with
normal `HttpResponse` handling client disconnects. As a consequence though
the `request_finished` signal may not run leading to things like db
connection not being closed.
What currently seems to be happening in `ASGIHandler` in
`django/core/handlers/asgi.py` goes something like this:
1. Concurrent tasks are started for
- disconnect listener (listen for "http.disconnect" from webserver)
- process_request
2. If disconnect happens first (e.g. browser refresh) `cancel()` is run on
`process_request` task and `request_finished` may not have been triggered
3. db connection for the request is not closed :(
Here are the lines of code where all this happens, marked:
https://github.com/django/django/blob/9c6d7b4a678b7bbc6a1a14420f686162ba9016f5/django/core/handlers/asgi.py#L191-L232
Possible fix? I'm thinking only views that return a
`HTTPStreamingResponse` would need to allow for cancellation cleanup
handling via the view. If so `response = await
self.run_get_response(request)` could run before and outside of any task
so we could check `response.streaming` attribute and only run
`listen_for_disconnect` task when there's a streaming response.
I've made a patch to try the above out and it seems to fix the issue:
https://github.com/HeyHugo/django/commit/e25a1525654e00dcd5b483689ef16e0dc74d32d1
Here's a minimal setup to reproduce the issue via print debugging:
https://github.com/HeyHugo/django_async_issue
--
Ticket URL: <https://code.djangoproject.com/ticket/35148>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Hugo Heyman | Owner: nobody
Component: HTTP handling | Version: 5.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => duplicate
Comment:
Duplicate of #35059.
--
Ticket URL: <https://code.djangoproject.com/ticket/35148#comment:1>
Django
-------------------------------+--------------------------------------
Reporter: Hugo Heyman | Owner: nobody
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Hugo, Does that the proposed [https://github.com/django/django/pull/17675
patch] work for you? Testing would be a huge help.
--
Ticket URL: <https://code.djangoproject.com/ticket/35148#comment:2>
Django
-------------------------------+--------------------------------------
Reporter: Hugo Heyman | Owner: nobody
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Aha oops there was already a ticket and a patch for this.
That patch works great. Tested on both my minimal setup and real project.
--
Ticket URL: <https://code.djangoproject.com/ticket/35148#comment:3>
Django
-------------------------------+--------------------------------------
Reporter: Hugo Heyman | Owner: nobody
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Thanks for checking.
--
Ticket URL: <https://code.djangoproject.com/ticket/35148#comment:4>