FIX: prevent AttributeError in __del__ on partially-initialized Cursor - #646
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens mssql_python.cursor.Cursor lifecycle behavior to avoid unraisable exceptions during garbage collection when a Cursor is only partially initialized, and adds regression tests to prevent recurrence of the CI PytestUnraisableExceptionWarning seen in issue #642.
Changes:
- Establishes
Cursorcleanup invariants earlier in__init__(closed=False,hstmt=None) soclose()/__del__can run safely even after initialization failures. - Makes
Cursor.close()tolerant of instances missing theclosedattribute (e.g., objects created viaCursor.__new__). - Fixes
__del__finalization check to usesys.is_finalizing()and guards debug logging during interpreter shutdown; adds lifecycle regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mssql_python/cursor.py |
Makes cursor initialization/cleanup resilient to partial construction and interpreter shutdown edge cases. |
tests/test_005_connection_cursor_lifecycle.py |
Adds regression tests for half-initialized cursor GC paths and failed cursor construction scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@subrata-ms generally looks good to me. The copilot comment on the test is interesting. |
bewithgaurav
left a comment
There was a problem hiding this comment.
fix itself lgtm but the tests aren't defending the same as of now
added a comment that should close that gap, copilot's comments are also working taking a look at
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 3271-3279 3271 # Don't raise an exception in __del__, just log it
3272 # If interpreter is shutting down, we might not have logging set up
3273 import sys
3274
! 3275 if sys and sys.is_finalizing():
3276 # Suppress logging during interpreter shutdown
3277 return
3278 # ``logger`` could be torn down or have its handlers closed
3279 # late in interpreter shutdown; guard so the debug callLines 3277-3285 3277 return
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)
3283
3284 def scroll(
3285 self, value: int, mode: str = "relative"📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.3%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 83.7%
mssql_python.connection.py: 84.7%🔗 Quick Links
|
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Unraisable exceptions from Cursor.__del__ flow through sys.unraisablehook, not through the warnings module, so warnings.catch_warnings(record=True) never sees them — the previous test passed on unfixed cursor.py. Also drop the gc.collect() sync point in the __init__-failure test: Connection._cursors is a WeakSet, so a failed Cursor.__init__ never installs a strong ref anywhere and __del__ runs synchronously on the exception unwind out of conn.cursor(). Wrap the hook around that call only. Verified: on main (buggy cursor.py) the half-initialized test now fails with AttributeError at cursor.py:782; on this branch (fixed cursor.py) it passes clean.
bewithgaurav
left a comment
There was a problem hiding this comment.
the core fix is correct, just need a minor correction on tests
Binding the raised RuntimeError to an enclosing-frame local ('boom = ...')
keeps the exception's __traceback__ alive, which in turn keeps the
partial Cursor's frame alive past the sys.unraisablehook restore. __del__
then fires AFTER the hook is unwound, the hook window closes with no
unraisables captured, and the assertion silently becomes a no-op — same
dead-assertion pattern as the old catch_warnings, just reached via a
different mechanism. The test passed on unfixed cursor.py as a result.
Fix: construct the RuntimeError inline inside _raise so no long-lived
reference to it exists. Once pytest.raises exits, the exception (and its
traceback, and the partial cursor frame) become unreachable and __del__
fires synchronously inside the hook window. Verified: fails on main,
passes on this branch.
Docstring note added so nobody reintroduces the enclosing-frame local.
Work Item / Issue Reference
Summary
This pull request improves the robustness of the
Cursorclass 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:
__init__method incursor.pynow setsself.closed = Falseandself.hstmt = Noneas the very first statements, before any code that might raise an exception, ensuring that even partially-initializedCursorinstances have a consistent state for cleanup. [1] [2]close()method now safely checks for the existence of theclosedattribute usinggetattr(self, "closed", True), preventingAttributeErrorif the attribute is missing (e.g., in half-initialized objects).__del__method now uses the correctsys.is_finalizing()function (instead of the incorrectsys._is_finalizing()) and guards logging calls to prevent unraisable exceptions during interpreter shutdown.Testing and regression prevention:
test_cursor_del_half_initialized_cursor_no_errorsandtest_cursor_init_failure_leaves_consistent_stateto 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.