[Django] #31233: Improve handling of database connection and cursor resources
Django
-------------------------------------+-------------------------------------
Reporter: Jon | Owner: nobody
Dufresne |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Some areas of the code do not always close a database connection or cursor
object after use. Instead it waits for the Python garbage collector to
remove it.
Like file resources, database resource ownership and lifetime should be
deliberate and deterministic.
A common example is the `_nodb_connection` property which opens a
connection but is rarely closed.
--
Ticket URL: <https://code.djangoproject.com/ticket/31233>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/12414
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
I think the `_nodb_cursor` cursor approach makes a lot of sense. Have we
experienced with trying to elevated `RessourceWarning`s to errors during
the suite's execution?
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
> Have we experienced with trying to elevated `RessourceWarnings` to
errors during the suite's execution?
That would be great. Unfortunately these typically occur inside a
`__del__` method, so this doesn't work out so well in practice. Here is
the discussion the last time this idea was raised:
https://github.com/django/django/pull/7676#issuecomment-266226305
If we can overcome this limitation, I'm very interested.
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Exceptions within `__del__` are effectively turned into stdout messages
and not raised but elevating them to errors make messages way more useful
and explicit about their origin
e.g.
{{{#!python
warnings.filterwarnings('error', 'unclosed', ResourceWarning)
}}}
Revealed a ton of other wise silenced messages of the following form when
running the suite of a large Django project
{{{
ResourceWarning: unclosed file <_io.TextIOWrapper name='path-to-file.json'
mode='r' encoding='UTF-8'>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
Makes sense. I've done that in PR
https://github.com/django/django/pull/12423. This did not reveal any new
`ResourceWarning` in the test suite for me.
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"2905b41670fda38f970ce4dd1955f486bed928ab" 2905b416]:
{{{
#!CommitTicketReference repository=""
revision="2905b41670fda38f970ce4dd1955f486bed928ab"
Refs #31233 -- Added "error" filter for RuntimeWarning during tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Dufresne
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Jon Dufresne
* status: new => assigned
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Dufresne
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3259983f569151232d8e3b0c3d0de3a858c2b265" 3259983]:
{{{
#!CommitTicketReference repository=""
revision="3259983f569151232d8e3b0c3d0de3a858c2b265"
Fixed #31233 -- Closed database connections and cursors after use.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Dufresne
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f48f671223a20b161ca819cf7d6298e43b8ba5fe" f48f671]:
{{{
#!CommitTicketReference repository=""
revision="f48f671223a20b161ca819cf7d6298e43b8ba5fe"
Refs #31233 -- Changed DatabaseWrapper._nodb_connection to _nodb_cursor().
It is now a method instead of a property and returns a context manager
that yields a cursor on entry and closes the cursor and connection upon
exit.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31233#comment:8>