Opened 5 years ago
Closed 5 years ago
#31233 closed Cleanup/optimization (fixed)
Improve handling of database connection and cursor resources
Reported by: | Jon Dufresne | Owned by: | Jon Dufresne |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (9)
comment:1 by , 5 years ago
Has patch: | set |
---|
comment:2 by , 5 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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?
comment:3 by , 5 years ago
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.
comment:4 by , 5 years ago
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.
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'>
comment:5 by , 5 years ago
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.
comment:7 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Accepted → Ready for checkin |
https://github.com/django/django/pull/12414