we dont close on GC connections that were detached (and weren't closed). should we?

Offen
  • Stimmt etwas nicht?
#13,570 7 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Maintainer antworten meist innerhalb von 1 Tag

Dieses Issue hat noch niemand übernommen.

Bewertung

Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Anfängerfreundlichkeit
48/100
Issue-Typ
Bug
Klarheit
Größtenteils klar
Aktivitätsstatus
Aktiv
Tech-Stack
python, sqlite
Bereich
database

Rechercherichtung

Lies _ConnectionFairy.detach(), _finalize_fairy() und die im Bericht erwähnte Einrichtung von weakref.finalize. Verwende die bereitgestellten synchronen und asyncio-Reproduktionen, um das Verhalten zu überprüfen. Die Aufgabe gilt als abgeschlossen, wenn getrennte Verbindungen während der Garbage Collection geschlossen werden und die asyncio-Warnung zur Garbage Collection einheitlich behandelt wird.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

bug connection pool
Describe the bug

A connection that has been detached from its pool with
Connection.detach() / PoolProxiedConnection.detach() and is then
garbage collected without being explicitly closed never has its DBAPI
connection closed. The connection is leaked until the DBAPI itself
reclaims it, if it ever does.

Detaching hands ownership of the DBAPI connection to the fairy, and the
explicit close path handles this correctly: _ConnectionFairy._checkin()
calls _finalize_fairy() with connection_record=None, which takes the
detach = connection_record is None branch and calls
pool._close_connection().

The garbage collection path never reaches that branch. The finalizer
established for the fairy at checkout closes over the record, and
_ConnectionFairy.detach() clears _ConnectionRecord.fairy_ref, so when
the fairy is later collected the cleanup is skipped entirely. The
connection is not closed, and for asyncio dialects the
"garbage collector is trying to clean up non-checked-in connection"
warning that a non-detached abandoned connection would produce is not
emitted either.

This is long standing behavior, reproduced unchanged on 1.4, 2.0 and
current main.

Optional link from https://docs.sqlalchemy.org which documents the behavior that is expected

https://docs.sqlalchemy.org/en/20/core/connections.html#sqlalchemy.engine.Connection.detach

SQLAlchemy Version in Use

2.1.0b4, also 2.0 and 1.4

DBAPI (i.e. the database driver)

sqlite3, aiosqlite

Database Vendor and Major Version

SQLite

Python Version

3.14

Operating system

Linux

To Reproduce
import gc
import sqlite3

from sqlalchemy import create_engine
from sqlalchemy import text

closed = []


class TrackingConnection:
    """wrap a DBAPI connection so we can see close() being called"""

    def __init__(self, real):
        self.real = real

    def close(self):
        closed.append(self)
        self.real.close()

    def __getattr__(self, name):
        return getattr(self.real, name)


def creator():
    return TrackingConnection(sqlite3.connect(":memory:"))


engine = create_engine("sqlite://", creator=creator)

# 1. detach, then close explicitly -> DBAPI connection is closed
conn = engine.connect()
conn.execute(text("select 1"))
conn.detach()
conn.close()
print("detach() + close()    -> DBAPI close() called:", len(closed) == 1)

# 2. detach, then just drop it -> DBAPI connection is leaked
closed.clear()
conn = engine.connect()
conn.execute(text("select 1"))
conn.detach()
del conn
gc.collect()
print("detach() + gc         -> DBAPI close() called:", len(closed) == 1)

The asyncio equivalent, showing that the GC warning is also skipped for a
detached connection:

import asyncio
import gc
import warnings

from sqlalchemy import text
from sqlalchemy.ext.asyncio import create_async_engine


async def main():
    e = create_async_engine("sqlite+aiosqlite://")
    for label, do_detach in (("not detached", False), ("detached", True)):
        conn = await e.connect()
        await conn.execute(text("select 1"))
        raw = await conn.get_raw_connection()
        if do_detach:
            raw.detach()
        del conn, raw
        with warnings.catch_warnings(record=True) as w:
            warnings.simplefilter("always")
            gc.collect()
        msgs = [
            str(x.message) for x in w if "garbage collector" in str(x.message)
        ]
        print(f"  {label:14s} -> gc warning emitted: {bool(msgs)}")


asyncio.run(main())
Error
detach() + close()    -> DBAPI close() called: True
detach() + gc         -> DBAPI close() called: False

  not detached   -> gc warning emitted: True
  detached       -> gc warning emitted: False
Additional context

Noticed while reviewing
https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6932, which
converts the fairy's garbage collection cleanup to weakref.finalize().
That change does not alter this behavior, it only makes it more visible:
where the previous code left a weakref callback in place that returned
early, _ConnectionFairy.detach() now cancels the finalizer outright.

A fix would need the detached fairy to keep a finalizer that closes over
connection_record=None, so that collection takes the same
detach/_close_connection() branch that the explicit close path takes.
This is not a two line change: _finalize_fairy() asserts a non-null
record in two places, and the second of those builds a throwaway
_ConnectionFairy to run the reset with, which currently requires a
record.

Vorherrschende Sprache
Python
Sterne
12.3k
Forks
1.8k
Ø Merge
2 T. 10 Std.
Gemergte PRs (30 T.)
9

Entwicklungsumgebung

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus sqlalchemy/sqlalchemy

Alle Issues in sqlalchemy/sqlalchemy

Ähnliche Issues

Weitere Issues zu Python

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.