FIX: prevent AttributeError in __del__ on partially-initialized Cursor

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"

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)