Re: [Django] #35059: ASGI server leaves stale DB connections
Django
---------------------------------+--------------------------------------
Reporter: James Thorniley | Owner: nobody
Type: Bug | Status: closed
Component: Uncategorized | Version: 5.0
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by James Thorniley):
git bisect found commit 7cd187a5ba58d7769039f487faeb9a5a2ff05540 however I
think this is a red herring, not sure why it came to that.
Reverting the changes from 64cea1e48f285ea2162c669208d95188b32bbc82 on top
of 5.0 fix the problems, so I think that is actually the problematic
commit.
This commit moved the `send_response` function into a task which can be
cancelled if and when `http.disconnect` is received. However the
`send_response` function was also responsible for calling
`response.close()` which is a sync function called asynchronously using
`sync_to_async`. This introduced a race condition as if the
`http.disconnect` is received during the call to the `sync_to_async`
wrapped code then the `response.close()` can effectively be cancelled and
the associated clean up (triggered by the `request_finished` signal from
inside `response.close()` never gets called.
If created a PR here for review, it seems to solve the issue for me:
https://github.com/django/django/pull/17675
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------+------------------------------------
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* status: closed => new
* severity: Normal => Release blocker
* needs_better_patch: 0 => 1
* component: Uncategorized => HTTP handling
* resolution: needsinfo =>
* stage: Unreviewed => Accepted
Comment:
With the test cases, I think we can accept this as a bug in
64cea1e48f285ea2162c669208d95188b32bbc82. Thanks for the investigation
James.
The `request_finished` signal **should** be dispatched in the disconnect
case.
I left a few initial comments on the PR. Happy to take another look later
on.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:5>
Django
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* owner: nobody => James Thorniley
* needs_better_patch: 1 => 0
* has_patch: 0 => 1
* status: new => assigned
Comment:
Assigning to James following the proposed PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):
* needs_better_patch: 0 => 1
Comment:
Setting patch needs improvements following the latest batch of review
comments made by Carlton.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):
* needs_better_patch: 0 => 1
Comment:
Setting as patch needs improvement while I wait for the author to reply to
my comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I have this same problem, it caused a massive spike in database
connections. I had to revert back to Django 4.2 for now until this is
fixed. I came here to report but found your all already on top of it,
thank you! Can't wait until this is fixed in 5.0.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Replying to [comment:11 Josh Orr]:
> I have this same problem, it caused a massive spike in database
connections. I had to revert back to Django 4.2 for now until this is
fixed. I came here to report but found your all already on top of it,
thank you! Can't wait until this is fixed in 5.0.
Hi Josh, Does that the proposed patch work for you? Testing would be a
huge help.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
#35148 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
This is pretty much ready for checkin pending two question and one
optional test change. I will wait for feedback from either the author or
Carlton to merge ASAP (release is planned for Feb 6th). Thanks everyone
who helped so far, by either building the fix, reviewing, debugging and/or
testing the patch!
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: assigned
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"a43d75e81da783fda08bf8d3493252e3676d11ea" a43d75e]:
{{{#!CommitTicketReference repository=""
revision="a43d75e81da783fda08bf8d3493252e3676d11ea"
Refs #35059 -- Made asgi tests' SignalHandler helper class re-usable by
other tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Component: HTTP handling | Version: 5.0
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"11393ab1316f973c5fbb534305750740d909b4e4" 11393ab]:
{{{#!CommitTicketReference repository=""
revision="11393ab1316f973c5fbb534305750740d909b4e4"
Fixed #35059 -- Ensured that ASGIHandler always sends the request_finished
signal.
Prior to this work, when async tasks that process the request are
cancelled due
to receiving an early "http.disconnect" ASGI message, the request_finished
signal was not being sent, potentially leading to resource leaks (such as
database connections).
This branch ensures that the request_finished signal is sent even in the
case
of early termination of the response.
Regression in 64cea1e48f285ea2162c669208d95188b32bbc82.
Co-authored-by: Natalia <124304+...@users.noreply.github.com>
Co-authored-by: Carlton Gibson <carlton...@noumenal.es>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"f1fbd061ac34f0afdaac4a5c7db099c39b43cb4a" f1fbd061]:
{{{#!CommitTicketReference repository=""
revision="f1fbd061ac34f0afdaac4a5c7db099c39b43cb4a"
[5.0.x] Fixed #35059 -- Ensured that ASGIHandler always sends the
request_finished signal.
Prior to this work, when async tasks that process the request are
cancelled due
to receiving an early "http.disconnect" ASGI message, the request_finished
signal was not being sent, potentially leading to resource leaks (such as
database connections).
This branch ensures that the request_finished signal is sent even in the
case
of early termination of the response.
Regression in 64cea1e48f285ea2162c669208d95188b32bbc82.
Co-authored-by: Natalia <124304+...@users.noreply.github.com>
Co-authored-by: Carlton Gibson <carlton...@noumenal.es>
Backport of 11393ab1316f973c5fbb534305750740d909b4e4 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia <124304+nessita@…>):
In [changeset:"bbb9ef3c62674e94ad8e6556933b112d84891f3d" bbb9ef3]:
{{{#!CommitTicketReference repository=""
revision="bbb9ef3c62674e94ad8e6556933b112d84891f3d"
[5.0.x] Refs #35059 -- Made asgi tests' SignalHandler helper class re-
usable by other tests.
Backport of a43d75e81da783fda08bf8d3493252e3676d11ea from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"f4a08b6ddfcacadfe9ff8364bf1c6c54f5dd370f" f4a08b6d]:
{{{#!CommitTicketReference repository=""
revision="f4a08b6ddfcacadfe9ff8364bf1c6c54f5dd370f"
Refs #35059 -- Used asyncio.Event in ASGITest.test_asyncio_cancel_error to
enforce specific interleaving.
Sleep call leads to a hard to trace error in CI. Using an Event is
more deterministic, and should be less prone to environment
variations.
Bug in 11393ab1316f973c5fbb534305750740d909b4e4.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
{{{#!CommitTicketReference repository=""
revision="38248588f66591d7a9fede0de6eb7f0ae1b6ada9"
[5.1.x] Refs #35059 -- Used asyncio.Event in
Sleep call leads to a hard to trace error in CI. Using an Event is
more deterministic, and should be less prone to environment
variations.
Bug in 11393ab1316f973c5fbb534305750740d909b4e4.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35059#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: James Thorniley | Owner: James
| Thorniley
Type: Bug | Status: closed
Component: HTTP handling | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia <124304+nessita@…>):
{{{#!CommitTicketReference repository=""
revision="dd3d0483ab6953b769077f16828ff88d0c2bffc4"
[5.0.x] Refs #35059 -- Used asyncio.Event in
Sleep call leads to a hard to trace error in CI. Using an Event is
more deterministic, and should be less prone to environment
variations.
Bug in 11393ab1316f973c5fbb534305750740d909b4e4.
Backport of f4a08b6ddfcacadfe9ff8364bf1c6c54f5dd370f from main.
}}}
--