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.

https://github.com/django/django/blob/335c9c94acf263901fb023404408880245b0c4b4/django/db/backends/base/base.py#L609-L618

Change History (9)

comment:2 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted

I think the _nodb_cursor cursor approach makes a lot of sense. Have we experienced with trying to elevated RessourceWarnings to errors during the suite's execution?

comment:3 by Jon Dufresne, 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 Simon Charette, 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 Jon Dufresne, 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:6 by GitHub <noreply@…>, 5 years ago

In 2905b416:

Refs #31233 -- Added "error" filter for RuntimeWarning during tests.

comment:7 by Mariusz Felisiak, 5 years ago

Owner: changed from nobody to Jon Dufresne
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In f48f671:

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.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 3259983:

Fixed #31233 -- Closed database connections and cursors after use.

Note: See TracTickets for help on using tickets.
Back to Top