From 6a1a32a2238bb336a0e54a3e6aa3a86bf757436b Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Wed, 30 Apr 2025 02:37:23 +0200 Subject: [PATCH] fix: don't call str(self) or imported objects in __del__ We easily end up trying to access resources already unavailable in the objects representation. In Python 3.14 this seems to happen aggressively (or maybe it's just more visible because testing a dev version). --- .flake8 | 1 - psycopg/psycopg/_connection_base.py | 10 +++---- psycopg/psycopg/_server_cursor.py | 8 ------ psycopg/psycopg/_server_cursor_async.py | 9 ------ psycopg/psycopg/_server_cursor_base.py | 13 +++++++++ psycopg/psycopg/pq/pq_ctypes.py | 38 ++++++++++++++----------- tests/test_connection.py | 6 ++-- tests/test_connection_async.py | 6 ++-- tests/test_cursor_server.py | 4 ++- tests/test_cursor_server_async.py | 4 ++- 10 files changed, 51 insertions(+), 48 deletions(-) diff --git a/.flake8 b/.flake8 index ed086adf0..79e029388 100644 --- a/.flake8 +++ b/.flake8 @@ -12,7 +12,6 @@ per-file-ignores = psycopg/psycopg/errors.py: E125, E128, E302 # Allow concatenated string literals from async_to_sync - psycopg/psycopg/_server_cursor.py: E501 psycopg_pool/psycopg_pool/pool.py: E501 # Pytest's importorskip() getting in the way diff --git a/psycopg/psycopg/_connection_base.py b/psycopg/psycopg/_connection_base.py index 53d06cc4f..1a2b777af 100644 --- a/psycopg/psycopg/_connection_base.py +++ b/psycopg/psycopg/_connection_base.py @@ -8,7 +8,7 @@ from __future__ import annotations import sys import logging -from typing import TYPE_CHECKING, Generic, NamedTuple, TypeAlias +from typing import TYPE_CHECKING, Any, Generic, NamedTuple, TypeAlias from weakref import ReferenceType, ref from warnings import warn from functools import partial @@ -135,7 +135,7 @@ class BaseConnection(Generic[Row]): self._deferrable: bool | None = None self._begin_statement = b"" - def __del__(self) -> None: + def __del__(self, __warn: Any = warn) -> None: # If fails on connection we might not have this attribute yet if not hasattr(self, "pgconn"): return @@ -148,9 +148,9 @@ class BaseConnection(Generic[Row]): if hasattr(self, "_pool"): return - warn( - f"connection {self} was deleted while still open." - " Please use 'with' or '.close()' to close the connection", + __warn( + f"{object.__repr__(self)} was deleted while still open." + " Please use 'with' or '.close()' to close the connection properly", ResourceWarning, ) diff --git a/psycopg/psycopg/_server_cursor.py b/psycopg/psycopg/_server_cursor.py index cfddfd97c..6eefb8510 100644 --- a/psycopg/psycopg/_server_cursor.py +++ b/psycopg/psycopg/_server_cursor.py @@ -10,7 +10,6 @@ psycopg server-side cursor (sync). from __future__ import annotations from typing import TYPE_CHECKING, Any, overload -from warnings import warn from collections.abc import Iterable from . import errors as e @@ -63,13 +62,6 @@ class ServerCursor(ServerCursorMixin["Connection[Any]", Row], Cursor[Row]): ) ServerCursorMixin.__init__(self, name, scrollable, withhold) - def __del__(self) -> None: - if not self.closed: - warn( - f"the server-side cursor {self} was deleted while still open. Please use 'with' or '.close()' to close the cursor properly", - ResourceWarning, - ) - def close(self) -> None: """ Close the current cursor and free associated resources. diff --git a/psycopg/psycopg/_server_cursor_async.py b/psycopg/psycopg/_server_cursor_async.py index 41628fe74..a37084b32 100644 --- a/psycopg/psycopg/_server_cursor_async.py +++ b/psycopg/psycopg/_server_cursor_async.py @@ -7,7 +7,6 @@ psycopg server-side cursor (async). from __future__ import annotations from typing import TYPE_CHECKING, Any, overload -from warnings import warn from collections.abc import Iterable from . import errors as e @@ -62,14 +61,6 @@ class AsyncServerCursor( ) ServerCursorMixin.__init__(self, name, scrollable, withhold) - def __del__(self) -> None: - if not self.closed: - warn( - f"the server-side cursor {self} was deleted while still open." - " Please use 'with' or '.close()' to close the cursor properly", - ResourceWarning, - ) - async def close(self) -> None: """ Close the current cursor and free associated resources. diff --git a/psycopg/psycopg/_server_cursor_base.py b/psycopg/psycopg/_server_cursor_base.py index fef6f9070..b6ca6843f 100644 --- a/psycopg/psycopg/_server_cursor_base.py +++ b/psycopg/psycopg/_server_cursor_base.py @@ -6,6 +6,9 @@ psycopg server-side cursor base objects. from __future__ import annotations +from typing import Any +from warnings import warn + from . import errors as e from . import pq, sql from .abc import ConnectionType, Params, PQGen, Query @@ -44,6 +47,16 @@ class ServerCursorMixin(BaseCursor[ConnectionType, Row]): self._iter_rows: list[Row] | None = None self._page_pos = 0 + def __del__(self, __warn: Any = warn) -> None: + if self.closed: + return + + __warn( + f"{object.__repr__(self)} was deleted while still open." + " Please use 'with' or '.close()' to close the cursor properly", + ResourceWarning, + ) + def __repr__(self) -> str: # Insert the name as the second word parts = super().__repr__().split(None, 1) diff --git a/psycopg/psycopg/pq/pq_ctypes.py b/psycopg/psycopg/pq/pq_ctypes.py index 7c16451e3..cf9684f5e 100644 --- a/psycopg/psycopg/pq/pq_ctypes.py +++ b/psycopg/psycopg/pq/pq_ctypes.py @@ -27,9 +27,6 @@ from .. import errors as e from .misc import ConninfoOption, PGnotify, PGresAttDesc, _clean_error_message from .misc import connection_summary from ._enums import ConnStatus, ExecStatus, Format, Trace - -# Imported locally to call them from __del__ methods -from ._pq_ctypes import PQcancelFinish, PQclear, PQfinish, PQfreeCancel, PQstatus from .._encodings import pg2pyenc if TYPE_CHECKING: @@ -41,6 +38,10 @@ logger = logging.getLogger("psycopg") OK = ConnStatus.OK +# note_on_del: functions called on __del__ are imported as local values to +# avoid warnings on interpreter shutdown in case the module is gc'd before the +# object is destroyed. + def version() -> int: """Return the version number of the libpq currently loaded. @@ -95,10 +96,10 @@ class PGconn: self._procpid = getpid() - def __del__(self) -> None: + def __del__(self, __getpid: Callable[[], int] = getpid) -> None: # Close the connection only if it was created in this process, # not if this object is being GC'd after fork. - if getpid() == self._procpid: + if __getpid() == self._procpid: # see note_on_del self.finish() def __repr__(self) -> str: @@ -125,10 +126,10 @@ class PGconn: def connect_poll(self) -> int: return self._call_int(impl.PQconnectPoll) - def finish(self) -> None: + def finish(self, __PQfinish: Any = impl.PQfinish) -> None: self._pgconn_ptr, p = None, self._pgconn_ptr if p: - PQfinish(p) + __PQfinish(p) @property def pgconn_ptr(self) -> int | None: @@ -206,8 +207,8 @@ class PGconn: return self._call_bytes(impl.PQoptions) @property - def status(self) -> int: - return PQstatus(self._pgconn_ptr) + def status(self, __PQstatus: Any = impl.PQstatus) -> int: + return __PQstatus(self._pgconn_ptr) @property def transaction_status(self) -> int: @@ -828,13 +829,16 @@ class PGresult: def __repr__(self) -> str: cls = f"{self.__class__.__module__}.{self.__class__.__qualname__}" - status = ExecStatus(self.status) - return f"<{cls} [{status.name}] at 0x{id(self):x}>" + try: + status = ExecStatus(self.status).name + except ValueError: + status = f"{self.status} (status unknown)" + return f"<{cls} [{status}] at 0x{id(self):x}>" - def clear(self) -> None: + def clear(self, __PQclear: Any = impl.PQclear) -> None: self._pgresult_ptr, p = None, self._pgresult_ptr if p: - PQclear(p) + __PQclear(p) # see note_on_del @property def pgresult_ptr(self) -> int | None: @@ -1000,7 +1004,7 @@ class PGcancelConn: self._ensure_pgcancelconn() impl.PQcancelReset(self.pgcancelconn_ptr) - def finish(self) -> None: + def finish(self, __PQcancelFinish: Any = impl.PQcancelFinish) -> None: """ Free the data structure created by `PQcancelCreate()`. @@ -1010,7 +1014,7 @@ class PGcancelConn: """ self.pgcancelconn_ptr, p = None, self.pgcancelconn_ptr if p: - PQcancelFinish(p) + __PQcancelFinish(p) # see note_on_del def _ensure_pgcancelconn(self) -> None: if not self.pgcancelconn_ptr: @@ -1032,7 +1036,7 @@ class PGcancel: def __del__(self) -> None: self.free() - def free(self) -> None: + def free(self, __PQfreeCancel: Any = impl.PQfreeCancel) -> None: """ Free the data structure created by :pq:`PQgetCancel()`. @@ -1042,7 +1046,7 @@ class PGcancel: """ self.pgcancel_ptr, p = None, self.pgcancel_ptr if p: - PQfreeCancel(p) + __PQfreeCancel(p) # see note_on_del def cancel(self) -> None: """Requests that the server abandon processing of the current command. diff --git a/tests/test_connection.py b/tests/test_connection.py index 90aa0ddf3..ad3a262e0 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -197,13 +197,13 @@ def test_connection_warn_close(conn_cls, dsn, recwarn, gc_collect): conn = conn_cls.connect(dsn) del conn gc_collect() - assert "IDLE" in str(recwarn.pop(ResourceWarning).message) + assert conn_cls.__name__ in str(recwarn.pop(ResourceWarning).message) conn = conn_cls.connect(dsn) conn.execute("select 1") del conn gc_collect() - assert "INTRANS" in str(recwarn.pop(ResourceWarning).message) + assert conn_cls.__name__ in str(recwarn.pop(ResourceWarning).message) conn = conn_cls.connect(dsn) try: @@ -212,7 +212,7 @@ def test_connection_warn_close(conn_cls, dsn, recwarn, gc_collect): pass del conn gc_collect() - assert "INERROR" in str(recwarn.pop(ResourceWarning).message) + assert conn_cls.__name__ in str(recwarn.pop(ResourceWarning).message) with conn_cls.connect(dsn) as conn: pass diff --git a/tests/test_connection_async.py b/tests/test_connection_async.py index 89af8475a..7eb5a27e3 100644 --- a/tests/test_connection_async.py +++ b/tests/test_connection_async.py @@ -192,13 +192,13 @@ async def test_connection_warn_close(aconn_cls, dsn, recwarn, gc_collect): conn = await aconn_cls.connect(dsn) del conn gc_collect() - assert "IDLE" in str(recwarn.pop(ResourceWarning).message) + assert aconn_cls.__name__ in str(recwarn.pop(ResourceWarning).message) conn = await aconn_cls.connect(dsn) await conn.execute("select 1") del conn gc_collect() - assert "INTRANS" in str(recwarn.pop(ResourceWarning).message) + assert aconn_cls.__name__ in str(recwarn.pop(ResourceWarning).message) conn = await aconn_cls.connect(dsn) try: @@ -207,7 +207,7 @@ async def test_connection_warn_close(aconn_cls, dsn, recwarn, gc_collect): pass del conn gc_collect() - assert "INERROR" in str(recwarn.pop(ResourceWarning).message) + assert aconn_cls.__name__ in str(recwarn.pop(ResourceWarning).message) async with await aconn_cls.connect(dsn) as conn: pass diff --git a/tests/test_cursor_server.py b/tests/test_cursor_server.py index 13b08006b..7bcc30529 100644 --- a/tests/test_cursor_server.py +++ b/tests/test_cursor_server.py @@ -274,7 +274,9 @@ def test_warn_close(conn, recwarn, gc_collect): cur.execute("select generate_series(1, 10) as bar") del cur gc_collect() - assert ".close()" in str(recwarn.pop(ResourceWarning).message) + msg = str(recwarn.pop(ResourceWarning).message) + assert conn.server_cursor_factory.__name__ in msg + assert ".close()" in msg def test_execute_reuse(conn): diff --git a/tests/test_cursor_server_async.py b/tests/test_cursor_server_async.py index 725c4b64b..f3c57d56b 100644 --- a/tests/test_cursor_server_async.py +++ b/tests/test_cursor_server_async.py @@ -280,7 +280,9 @@ async def test_warn_close(aconn, recwarn, gc_collect): await cur.execute("select generate_series(1, 10) as bar") del cur gc_collect() - assert ".close()" in str(recwarn.pop(ResourceWarning).message) + msg = str(recwarn.pop(ResourceWarning).message) + assert aconn.server_cursor_factory.__name__ in msg + assert ".close()" in msg async def test_execute_reuse(aconn): -- 2.47.3