diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index bc2956d7..9515ace2 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -110,11 +110,21 @@ def __init__(self, connection: "Connection", timeout: int = 0) -> None: connection: Database connection object. timeout: Query timeout in seconds """ + # Establish the close() invariant *first*, before any statement that + # can raise (notably ``_initialize_cursor`` below). Setting + # ``closed=False`` (not True) up front means that if ``__init__`` + # fails partway — even after ``hstmt`` was allocated — a subsequent + # ``close()`` / ``__del__`` will still see a consistent view and + # correctly release whatever was allocated. Pairing it with + # ``hstmt=None`` keeps ``close()`` safe when allocation fails before + # an HSTMT exists. + self.closed: bool = False + self.hstmt: Optional[Any] = None + self._connection: "Connection" = connection # Store as private attribute self._timeout: int = timeout self._inputsizes: Optional[List[Union[int, Tuple[Any, ...]]]] = None # self.connection.autocommit = False - self.hstmt: Optional[Any] = None self._initialize_cursor() self.description: Optional[ List[ @@ -134,7 +144,6 @@ def __init__(self, connection: "Connection", timeout: int = 0) -> None: 1 # Default number of rows to fetch at a time is 1, user can change it ) self.buffer_length: int = 1024 # Default buffer length for string data - self.closed: bool = False self._result_set_empty: bool = False # Add this initialization self.last_executed_stmt: str = "" # Stores the last statement executed by this cursor self.is_stmt_prepared: List[bool] = [ @@ -779,7 +788,13 @@ def close(self) -> None: will be raised if any operation (other than close) is attempted with the cursor. This is a deviation from pyodbc, which raises an exception if the cursor is already closed. """ - if self.closed: + # ``closed`` may be missing on the instance only if the object was + # built via ``Cursor.__new__(Cursor)`` (no ``__init__``). Normal + # construction sets ``self.closed = False`` as the first statement + # of ``__init__``, so any partially-initialized instance still has + # the attribute. Treat the no-``__init__`` case as "already closed" + # — there's nothing to release — and let GC reap the object cleanly. + if getattr(self, "closed", True): # Do nothing - not calling _check_closed() here since we want this to be idempotent return @@ -3257,10 +3272,14 @@ def __del__(self): # If interpreter is shutting down, we might not have logging set up import sys - if sys and sys._is_finalizing(): + if sys and sys.is_finalizing(): # Suppress logging during interpreter shutdown return - logger.debug("Exception during cursor cleanup in __del__: %s", e) + # ``logger`` could be torn down or have its handlers closed + # late in interpreter shutdown; guard so the debug call + # cannot itself raise an unraisable exception. + if logger is not None: + logger.debug("Exception during cursor cleanup in __del__: %s", e) def scroll( self, value: int, mode: str = "relative" diff --git a/tests/test_005_connection_cursor_lifecycle.py b/tests/test_005_connection_cursor_lifecycle.py index bad04a22..d68b1960 100644 --- a/tests/test_005_connection_cursor_lifecycle.py +++ b/tests/test_005_connection_cursor_lifecycle.py @@ -402,6 +402,136 @@ def test_cursor_del_unclosed_cursor_cleanup(conn_str): assert "Exception" not in result.stderr +def test_cursor_del_half_initialized_cursor_no_errors(): + """Regression: ``Cursor.__del__`` / ``close()`` must tolerate Cursor instances + missing the ``closed`` attribute (e.g. objects created via ``Cursor.__new__``), + so GC does not emit unraisable exceptions. + + Two bugs used to fire in that path and produce a + ``PytestUnraisableExceptionWarning`` in CI: + * Bug A: ``close()`` did ``if self.closed:`` and raised + ``AttributeError: 'Cursor' object has no attribute 'closed'``; + * Bug B: the ``__del__`` exception handler then did + ``sys._is_finalizing()`` (typo for ``sys.is_finalizing``) and raised + a second AttributeError, masking Bug A. + + NOTE: unraisable exceptions from ``__del__`` flow through + ``sys.unraisablehook`` — NOT through the ``warnings`` module — so + ``warnings.catch_warnings(record=True)`` never sees them. We install + a temporary ``sys.unraisablehook`` to observe them directly. + """ + import gc + from mssql_python.cursor import Cursor + + class _BogusConn: + pass + + # --- Bug A regression guard: explicit close() must tolerate the + # missing ``closed`` attribute. On unfixed cursor.py this raises + # ``AttributeError`` synchronously and the test fails right here. + c1 = Cursor.__new__(Cursor) + c1._connection = _BogusConn() + c1.hstmt = None + assert "closed" not in c1.__dict__, "test fixture must omit 'closed' attribute" + c1.close() + + # --- Bug B regression guard: drive a fresh partial cursor through + # ``__del__`` without calling close() first. On unfixed cursor.py, + # ``__del__`` calls close() -> Bug A AttributeError -> the exception + # handler calls ``sys._is_finalizing()`` -> Bug B AttributeError + # escapes through ``sys.unraisablehook``. On the fixed code path, + # close() succeeds inside __del__ and no unraisable is emitted. + captured = [] + + def _hook(unraisable): + captured.append(unraisable) + + old_hook = sys.unraisablehook + sys.unraisablehook = _hook + try: + c2 = Cursor.__new__(Cursor) + c2._connection = _BogusConn() + c2.hstmt = None + assert "closed" not in c2.__dict__ + del c2 + gc.collect() # belt-and-suspenders; refcount already reached zero. + finally: + sys.unraisablehook = old_hook + + offenders = [ + u + for u in captured + if isinstance(u.exc_value, AttributeError) + and ("closed" in str(u.exc_value) or "_is_finalizing" in str(u.exc_value)) + ] + assert not offenders, ( + f"unexpected unraisable AttributeError from Cursor.__del__: " + f"{[str(u.exc_value) for u in offenders]}" + ) + + +def test_cursor_init_failure_leaves_consistent_state(conn_str, monkeypatch): + """Structural-fix regression: ``Cursor.__init__`` must set + ``self.closed = False`` and ``self.hstmt = None`` *before* any statement + that can raise. If ``_initialize_cursor`` fails (e.g. HSTMT alloc), + the partially-constructed cursor must still be safely closeable and + must not leak a server-side handle on the way to the GC. + + Before the fix the partial cursor had no ``closed`` attribute at all, + causing ``__del__`` -> ``close()`` to raise ``AttributeError`` and + surface as ``PytestUnraisableExceptionWarning`` in CI. + + NOTE: the failed ``Cursor.__init__`` never installs a strong ref anywhere + (``Connection._cursors`` is a ``WeakSet``), so the partial object's + refcount reaches zero the moment the exception unwinds out of + ``conn.cursor()`` — ``__del__`` runs synchronously right there, not on + the next ``gc.collect()``. We therefore wrap ``sys.unraisablehook`` + only around the ``conn.cursor()`` call, and observe unraisables through + the hook rather than through ``warnings.catch_warnings`` (which never + sees them). + """ + from mssql_python import connect + from mssql_python.cursor import Cursor + + conn = connect(conn_str) + try: + boom = RuntimeError("simulated HSTMT allocation failure") + + def _raise(self): + raise boom + + monkeypatch.setattr(Cursor, "_initialize_cursor", _raise) + + captured = [] + + def _hook(unraisable): + captured.append(unraisable) + + old_hook = sys.unraisablehook + sys.unraisablehook = _hook + try: + with pytest.raises(RuntimeError, match="simulated HSTMT"): + conn.cursor() + finally: + sys.unraisablehook = old_hook + + offenders = [u for u in captured if isinstance(u.exc_value, AttributeError)] + assert not offenders, ( + "failed __init__ produced unraisable AttributeError in __del__: " + f"{[str(u.exc_value) for u in offenders]}" + ) + + # Connection must still be usable after the failed cursor creation. + # This implicitly verifies _cursors tracking wasn't corrupted. + monkeypatch.undo() + cur = conn.cursor() + cur.execute("SELECT 1") + assert cur.fetchone()[0] == 1 + cur.close() + finally: + conn.close() + + def test_cursor_operations_after_close_raise_errors(conn_str): """Test that all cursor operations raise appropriate errors after close""" conn = connect(conn_str)