Skip to content

Commit dbbcd02

Browse files
subrata-mssubrata-mssaurabh500Copilot
authored
FIX: prevent AttributeError in __del__ on partially-initialized Cursor (#646)
### Work Item / Issue Reference <!-- IMPORTANT: Please follow the PR template guidelines below. For mssql-python maintainers: Insert your ADO Work Item ID below For external contributors: Insert Github Issue number below Only one reference is required - either GitHub issue OR ADO Work Item. --> <!-- mssql-python maintainers: ADO Work Item --> > [AB#45943](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/45943) <!-- External contributors: GitHub Issue --> > GitHub Issue: #642 ------------------------------------------------------------------- ### Summary <!-- Insert your summary of changes below. Minimum 10 characters required. --> This pull request improves the robustness of the `Cursor` class initialization and cleanup logic, ensuring that partially-initialized or half-constructed cursor objects are handled safely and do not cause unraisable exceptions during garbage collection. It also adds regression tests to prevent recurrence of related bugs. **Initialization and cleanup robustness:** * The `__init__` method in `cursor.py` now sets `self.closed = False` and `self.hstmt = None` as the very first statements, before any code that might raise an exception, ensuring that even partially-initialized `Cursor` instances have a consistent state for cleanup. [[1]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280R113-L117) [[2]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280L137) * The `close()` method now safely checks for the existence of the `closed` attribute using `getattr(self, "closed", True)`, preventing `AttributeError` if the attribute is missing (e.g., in half-initialized objects). * The `__del__` method now uses the correct `sys.is_finalizing()` function (instead of the incorrect `sys._is_finalizing()`) and guards logging calls to prevent unraisable exceptions during interpreter shutdown. **Testing and regression prevention:** * Adds `test_cursor_del_half_initialized_cursor_no_errors` and `test_cursor_init_failure_leaves_consistent_state` to ensure that half-initialized cursors do not raise unraisable exceptions during garbage collection and that failed initialization leaves the cursor and connection in a consistent, recoverable state. <!-- ### PR Title Guide > For feature requests FEAT: (short-description) > For non-feature requests like test case updates, config updates , dependency updates etc CHORE: (short-description) > For Fix requests FIX: (short-description) > For doc update requests DOC: (short-description) > For Formatting, indentation, or styling update STYLE: (short-description) > For Refactor, without any feature changes REFACTOR: (short-description) > For performance improvements PERF: (short-description) > For release related changes, without any feature changes RELEASE: #<RELEASE_VERSION> (short-description) ### Contribution Guidelines External contributors: - Create a GitHub issue first: https://github.com/microsoft/mssql-python/issues/new - Link the GitHub issue in the "GitHub Issue" section above - Follow the PR title format and provide a meaningful summary mssql-python maintainers: - Create an ADO Work Item following internal processes - Link the ADO Work Item in the "ADO Work Item" section above - Follow the PR title format and provide a meaningful summary --> --------- Co-authored-by: subrata-ms <subrata@microsoft.com> Co-authored-by: Saurabh Singh <saurabh500@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 4bc56b4 commit dbbcd02

2 files changed

Lines changed: 163 additions & 5 deletions

File tree

‎mssql_python/cursor.py‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,11 +110,21 @@ def __init__(self, connection: "Connection", timeout: int = 0) -> None:
110110
connection: Database connection object.
111111
timeout: Query timeout in seconds
112112
"""
113+
# Establish the close() invariant *first*, before any statement that
114+
# can raise (notably ``_initialize_cursor`` below). Setting
115+
# ``closed=False`` (not True) up front means that if ``__init__``
116+
# fails partway — even after ``hstmt`` was allocated — a subsequent
117+
# ``close()`` / ``__del__`` will still see a consistent view and
118+
# correctly release whatever was allocated. Pairing it with
119+
# ``hstmt=None`` keeps ``close()`` safe when allocation fails before
120+
# an HSTMT exists.
121+
self.closed: bool = False
122+
self.hstmt: Optional[Any] = None
123+
113124
self._connection: "Connection" = connection # Store as private attribute
114125
self._timeout: int = timeout
115126
self._inputsizes: Optional[List[Union[int, Tuple[Any, ...]]]] = None
116127
# self.connection.autocommit = False
117-
self.hstmt: Optional[Any] = None
118128
self._initialize_cursor()
119129
self.description: Optional[
120130
List[
@@ -134,7 +144,6 @@ def __init__(self, connection: "Connection", timeout: int = 0) -> None:
134144
1 # Default number of rows to fetch at a time is 1, user can change it
135145
)
136146
self.buffer_length: int = 1024 # Default buffer length for string data
137-
self.closed: bool = False
138147
self._result_set_empty: bool = False # Add this initialization
139148
self.last_executed_stmt: str = "" # Stores the last statement executed by this cursor
140149
self.is_stmt_prepared: List[bool] = [
@@ -779,7 +788,13 @@ def close(self) -> None:
779788
will be raised if any operation (other than close) is attempted with the cursor.
780789
This is a deviation from pyodbc, which raises an exception if the cursor is already closed.
781790
"""
782-
if self.closed:
791+
# ``closed`` may be missing on the instance only if the object was
792+
# built via ``Cursor.__new__(Cursor)`` (no ``__init__``). Normal
793+
# construction sets ``self.closed = False`` as the first statement
794+
# of ``__init__``, so any partially-initialized instance still has
795+
# the attribute. Treat the no-``__init__`` case as "already closed"
796+
# — there's nothing to release — and let GC reap the object cleanly.
797+
if getattr(self, "closed", True):
783798
# Do nothing - not calling _check_closed() here since we want this to be idempotent
784799
return
785800

@@ -3257,10 +3272,14 @@ def __del__(self):
32573272
# If interpreter is shutting down, we might not have logging set up
32583273
import sys
32593274

3260-
if sys and sys._is_finalizing():
3275+
if sys and sys.is_finalizing():
32613276
# Suppress logging during interpreter shutdown
32623277
return
3263-
logger.debug("Exception during cursor cleanup in __del__: %s", e)
3278+
# ``logger`` could be torn down or have its handlers closed
3279+
# late in interpreter shutdown; guard so the debug call
3280+
# cannot itself raise an unraisable exception.
3281+
if logger is not None:
3282+
logger.debug("Exception during cursor cleanup in __del__: %s", e)
32643283

32653284
def scroll(
32663285
self, value: int, mode: str = "relative"

‎tests/test_005_connection_cursor_lifecycle.py‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,145 @@ def test_cursor_del_unclosed_cursor_cleanup(conn_str):
402402
assert "Exception" not in result.stderr
403403

404404

405+
def test_cursor_del_half_initialized_cursor_no_errors():
406+
"""Regression: ``Cursor.__del__`` / ``close()`` must tolerate Cursor instances
407+
missing the ``closed`` attribute (e.g. objects created via ``Cursor.__new__``),
408+
so GC does not emit unraisable exceptions.
409+
410+
Two bugs used to fire in that path and produce a
411+
``PytestUnraisableExceptionWarning`` in CI:
412+
* Bug A: ``close()`` did ``if self.closed:`` and raised
413+
``AttributeError: 'Cursor' object has no attribute 'closed'``;
414+
* Bug B: the ``__del__`` exception handler then did
415+
``sys._is_finalizing()`` (typo for ``sys.is_finalizing``) and raised
416+
a second AttributeError, masking Bug A.
417+
418+
NOTE: unraisable exceptions from ``__del__`` flow through
419+
``sys.unraisablehook`` — NOT through the ``warnings`` module — so
420+
``warnings.catch_warnings(record=True)`` never sees them. We install
421+
a temporary ``sys.unraisablehook`` to observe them directly.
422+
"""
423+
import gc
424+
from mssql_python.cursor import Cursor
425+
426+
class _BogusConn:
427+
pass
428+
429+
# --- Bug A regression guard: explicit close() must tolerate the
430+
# missing ``closed`` attribute. On unfixed cursor.py this raises
431+
# ``AttributeError`` synchronously and the test fails right here.
432+
c1 = Cursor.__new__(Cursor)
433+
c1._connection = _BogusConn()
434+
c1.hstmt = None
435+
assert "closed" not in c1.__dict__, "test fixture must omit 'closed' attribute"
436+
c1.close()
437+
438+
# --- Bug B regression guard: drive a fresh partial cursor through
439+
# ``__del__`` without calling close() first. On unfixed cursor.py,
440+
# ``__del__`` calls close() -> Bug A AttributeError -> the exception
441+
# handler calls ``sys._is_finalizing()`` -> Bug B AttributeError
442+
# escapes through ``sys.unraisablehook``. On the fixed code path,
443+
# close() succeeds inside __del__ and no unraisable is emitted.
444+
captured = []
445+
446+
def _hook(unraisable):
447+
captured.append(unraisable)
448+
449+
old_hook = sys.unraisablehook
450+
sys.unraisablehook = _hook
451+
try:
452+
c2 = Cursor.__new__(Cursor)
453+
c2._connection = _BogusConn()
454+
c2.hstmt = None
455+
assert "closed" not in c2.__dict__
456+
del c2
457+
gc.collect() # belt-and-suspenders; refcount already reached zero.
458+
finally:
459+
sys.unraisablehook = old_hook
460+
461+
offenders = [
462+
u
463+
for u in captured
464+
if isinstance(u.exc_value, AttributeError)
465+
and ("closed" in str(u.exc_value) or "_is_finalizing" in str(u.exc_value))
466+
]
467+
assert not offenders, (
468+
f"unexpected unraisable AttributeError from Cursor.__del__: "
469+
f"{[str(u.exc_value) for u in offenders]}"
470+
)
471+
472+
473+
def test_cursor_init_failure_leaves_consistent_state(conn_str, monkeypatch):
474+
"""Structural-fix regression: ``Cursor.__init__`` must set
475+
``self.closed = False`` and ``self.hstmt = None`` *before* any statement
476+
that can raise. If ``_initialize_cursor`` fails (e.g. HSTMT alloc),
477+
the partially-constructed cursor must still be safely closeable and
478+
must not leak a server-side handle on the way to the GC.
479+
480+
Before the fix the partial cursor had no ``closed`` attribute at all,
481+
causing ``__del__`` -> ``close()`` to raise ``AttributeError`` and
482+
surface as ``PytestUnraisableExceptionWarning`` in CI.
483+
484+
NOTE: the failed ``Cursor.__init__`` never installs a strong ref anywhere
485+
(``Connection._cursors`` is a ``WeakSet``), so the partial object's
486+
refcount reaches zero the moment the exception unwinds out of
487+
``conn.cursor()`` — ``__del__`` runs synchronously right there, not on
488+
the next ``gc.collect()``. We therefore wrap ``sys.unraisablehook``
489+
only around the ``conn.cursor()`` call, and observe unraisables through
490+
the hook rather than through ``warnings.catch_warnings`` (which never
491+
sees them).
492+
493+
NOTE 2: the ``RuntimeError`` MUST be constructed inline inside
494+
``_raise`` — do NOT bind it to a local outside the function. A
495+
local like ``boom = RuntimeError(...)`` in the enclosing frame keeps
496+
the raised exception's ``__traceback__`` alive, which in turn keeps
497+
the partial ``Cursor`` frame alive past the ``sys.unraisablehook``
498+
restore, so ``__del__`` never fires inside the hook window and the
499+
assertion silently becomes a no-op (passes on unfixed cursor.py).
500+
"""
501+
from mssql_python import connect
502+
from mssql_python.cursor import Cursor
503+
504+
conn = connect(conn_str)
505+
try:
506+
507+
def _raise(self):
508+
# Instantiate the exception inline; see NOTE 2 in the docstring
509+
# for why we must not bind this to an enclosing-frame local.
510+
raise RuntimeError("simulated HSTMT allocation failure")
511+
512+
monkeypatch.setattr(Cursor, "_initialize_cursor", _raise)
513+
514+
captured = []
515+
516+
def _hook(unraisable):
517+
captured.append(unraisable)
518+
519+
old_hook = sys.unraisablehook
520+
sys.unraisablehook = _hook
521+
try:
522+
with pytest.raises(RuntimeError, match="simulated HSTMT"):
523+
conn.cursor()
524+
finally:
525+
sys.unraisablehook = old_hook
526+
527+
offenders = [u for u in captured if isinstance(u.exc_value, AttributeError)]
528+
assert not offenders, (
529+
"failed __init__ produced unraisable AttributeError in __del__: "
530+
f"{[str(u.exc_value) for u in offenders]}"
531+
)
532+
533+
# Connection must still be usable after the failed cursor creation.
534+
# This implicitly verifies _cursors tracking wasn't corrupted.
535+
monkeypatch.undo()
536+
cur = conn.cursor()
537+
cur.execute("SELECT 1")
538+
assert cur.fetchone()[0] == 1
539+
cur.close()
540+
finally:
541+
conn.close()
542+
543+
405544
def test_cursor_operations_after_close_raise_errors(conn_str):
406545
"""Test that all cursor operations raise appropriate errors after close"""
407546
conn = connect(conn_str)

0 commit comments

Comments
 (0)