DatabaseWrapper are not GC and connections are not closed

[Fábio Domingues]

I have found possible memory leak when investigating why database connections where not closed after the thread/coroutine ended (https://code.djangoproject.com/ticket/33497 and ​https://github.com/django/django/pull/17275), the DatabaseWrapper ends up in a circular reference with DatabaseFeatures/DatabaseCreation/DatabaseClient/... and the GC could not cleanup the objects.
I tryed use WeakRef from the "child" objects to the wrapper or deleting the __dict__ and the wrapper was collected and the connections was closed.

[Mariusz Felisiak]

Fabio, thanks for the report. Is there a reason to open a new ticket and don't add your findings in the #33497? As far as I'm aware, this is a duplicate.

[Fábio Domingues]

Replying to Mariusz Felisiak:

Fabio, thanks for the report. Is there a reason to open a new ticket and don't add your findings in the #33497? As far as I'm aware, this is a duplicate.

Fixing this memory leak will not solve the problem of the persistent connections not been reuse, only the fact that they will not pill up (and starve the DB) if inaccessible.
But I'm new here, still learning. Should I move the findings to #33497?

[Mariusz Felisiak]

We can keep this separately.

Tentatively accepted, I'd like to check out the patch proposal.

[Carlton Gibson]

...I'd like to check out the patch proposal.

Yes, please.

Any chance you could give a minimal example showing the behaviour here, to look at?
Various folks have reported leaky behaviour on Channels/Daphne, but we've not been able to pin it down.

… ends up in a circular reference ... and the GC could not cleanup the objects.

Just out of interest, could you try throwing in a gc.collect() — that should get it — unless we never let the DatabaseWrapper go, which is why it would be nice to look at the example — but WeakRef is hopefully right in this case.

[Fábio Domingues]

Tested using Uvicorn and Daphne, the database engine should not matter, I tested with PostgreSQL, CONN_MAX_AGE could be any value and the effect is the same using async or sync views.

get_refs = lambda: [o for o in gc.get_objects() if isinstance(o, BaseDatabaseWrapper)]
plot_refs = lambda: objgraph.show_backrefs(get_refs(), filename="sample-graph.png")

def sync_view(request):
    MyModel.objects.count()
    # gc.collect()
    # plot_refs()
    return HttpResponse(f"Number of BaseDatabaseWrapper instances: {len(get_refs())}")

async def async_view(request):
    await MyModel.objects.acount()
    # gc.collect()
    # plot_refs()
    return HttpResponse(f"Number of BaseDatabaseWrapper instances: {len(get_refs())}")

On each refresh you should see the number of instances increase. If you plot the references you would see many diagrams like the one in the attachment. Throwing gc.collect() actually get it, but it take some milliseconds, even if the underlying database connection is already closed.

[Fábio Domingues]

One more detail. Using objgraph.show_growth without doing gc.collect give us this on each new request:

list                       1990       +10
dict                       6798        +3
deque                        20        +2
lock                         43        +2
OrderedDict                  11        +2
partial                      24        +2
ReferenceType              3848        +1
DatabaseWrapper               6        +1
DatabaseClient                6        +1
DatabaseCreation              6        +1
DatabaseFeatures              6        +1
DatabaseIntrospection         6        +1
DatabaseOperations            6        +1
BaseDatabaseValidation        6        +1
DatabaseErrorWrapper          5        +1
PGconn                        5        +1
Connection                    5        +1
PrepareManager                5        +1
AdaptersMap                   7        +1
TypesRegistry                 7        +1

with the patch the output of show_growth is empty.

[Fábio Domingues]

Possible approach.

[Florian Apolloner]

Throwing gc.collect() actually get it, but it take some milliseconds, even if the underlying database connection is already closed.

with "actually get it" do you mean that it cleans it up properly? If yes there does not seem to be real leak here since garbage collection usually runs every now and then (or do I miss something). It might still be beneficial to improve the performance here but that might require a test or two :D

[Mariusz Felisiak]

Marking as needs improvement per Florian's comment.