FIX: Fix mixed cursor cleanup shutdown crash - #772
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The added assertion can pass without actually proving the Cursor.__del__/close() path executed (due to _cursors being a WeakSet), so the test may not validate the intended shutdown-crash scenario.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the connection/cursor lifecycle test suite to more explicitly exercise mixed cursor cleanup paths, especially cleanup via garbage collection and __del__, to help prevent shutdown-time crashes/regressions in cursor finalization.
Changes:
- Extend the
test_mixed_cursor_cleanup_scenariossubprocess script to delete an unclosed cursor, force GC, and assert the connection’s cursor tracking set is empty. - Add extra end-of-script cleanup (
del+gc.collect()) to reduce lingering references in the subprocess. - Assert the new “PASS: Cursor del cleaned up cursor” marker appears in subprocess output.
File summaries
| File | Description |
|---|---|
| tests/test_005_connection_cursor_lifecycle.py | Strengthens cursor lifecycle coverage by forcing GC-driven cleanup in a subprocess and asserting expected cleanup markers. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The change is test-only, directly addresses the previously identified coverage gap by asserting Cursor.close() is invoked via __del__, and appears low-risk and self-contained.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The test enhancement directly addresses the prior gap (WeakSet-only assertions) by instrumenting Cursor.close() and validating the destructor-driven cleanup path in a subprocess.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
Approved. No actionable findings in the cursor-cleanup test change. The latest commit only merges main; the PR patch is unchanged from the reviewed version.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 77.5%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.4%
mssql_python.logging.py: 85.5%
mssql_python.helpers.py: 89.3%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
Jahnvi Thakkar (jahnvi480)
left a comment
Jahnvi Thakkar (jahnvi480)
left a comment
There was a problem hiding this comment.
LGTM! Approving
[AB#48121](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/48121) ### Summary #### Enhancements - Add selectable native ODBC providers (#730). - Route `setinputsizes()` through the native C++ execution pipeline (#736). - Accept `memoryview` values in `Binary()` (#741). - Expose SQL Server type constants at module level (#764). #### Bug Fixes - Prevent concurrent logging deadlocks (#678). - Vendor the correct `mssql_py_core` architecture in Windows ARM64 wheels (#737). - Resolve bundled Windows driver and authentication DLLs from package-local directories (#735). - Bind `Decimal` parameters consistently as `SQL_NUMERIC` (#742). - Use ODBC 3.x parameter types (#758). - Decode `SQL_DATABASE_NAME` metadata (#771). - Prevent shutdown crashes during mixed cursor cleanup (#772). Bumps the package version from 1.14.0 to 1.15.0 and refreshes the PyPI release summary. --------- Co-authored-by: Gaurav Sharma <sharmag@microsoft.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Work Item / Issue Reference
Summary
This pull request enhances the
test_mixed_cursor_cleanup_scenariostest to more thoroughly verify cursor cleanup behavior, particularly focusing on cleanup via garbage collection and the__del__method. The changes ensure that cursors are properly removed from the connection when deleted and that the test asserts this behavior explicitly.Improvements to cursor cleanup verification:
cursor2and forced garbage collection to test that the cursor's__del__method cleans up the cursor, followed by an assertion that the connection's cursor list is empty.gcmodule and perform additional cleanup of cursor and connection objects at the end of the test, ensuring no lingering references. [1] [2]Test assertion enhancements: