Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new closed property to the Connection class, exposing the internal _closed attribute to provide users with a standardized way to check if a connection has been explicitly closed. This aligns with DB-API 2.0 conventions and improves the API's usability.
Key Changes:
- Added a
closedproperty to the Connection class that returns the state of the internal_closedattribute - Added comprehensive test coverage for the new property covering multiple scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mssql_python/connection.py | Added closed property with clear documentation explaining it reflects whether close() was called, not connection health |
| tests/test_003_connection.py | Added four test functions covering basic state checking, idempotent close behavior, context manager usage, and operations on closed connections |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.ddbc_bindings.h: 71.7%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%🔗 Quick Links
|
jahnvi480
changed the title
bewithgaurav
approved these changes
Jan 8, 2026
jahnvi480
changed the title
gargsaumya
approved these changes
Jan 9, 2026
dlevy-msft-sql
added a commit
to dlevy-msft-sql/mssql-python
that referenced
this pull request
Jan 16, 2026
…sor outlives connection - Check connection.closed property before calling SQLFreeHandle in Cursor.close() - Skip handle cleanup if parent connection is already closed (handles are invalid) - Add early sys._is_finalizing() check in Cursor.__del__() - Add 5 new tests covering the exact reproduction scenario from the issue This fixes the case where: 1. Connection is closed (ODBC handles freed) 2. Cursor objects still exist in Python memory 3. GC runs during normal execution (not interpreter shutdown) 4. Cursor.__del__() tries to free invalid ODBC statement handle PR microsoft#361 only checked sys.is_finalizing() which doesn't cover GC during normal execution. This fix uses the Connection.closed property (from PR microsoft#398) to detect closed connections.
dlevy-msft-sql
added a commit
to dlevy-msft-sql/mssql-python
that referenced
this pull request
Jan 16, 2026
…sor outlives connection - Check connection state before calling SQLFreeHandle in Cursor.close() - Default to SAFE behavior: skip handle cleanup if connection is None/gone/closed - Only free handle if we can CONFIRM connection is alive and open - Add early sys._is_finalizing() check in Cursor.__del__() - Add 5 new tests covering the exact reproduction scenario from the issue This fixes the case where: 1. Connection is closed (ODBC handles freed) 2. Cursor objects still exist in Python memory 3. GC runs during normal execution (not interpreter shutdown) 4. Cursor.__del__() tries to free invalid ODBC statement handle Key insight: The previous fix defaulted to 'not closed' when connection was None, which caused crashes when the connection was garbage collected. Now we default to 'closed' (safe) and only free if we can confirm connection is alive and open. PR microsoft#361 only checked sys.is_finalizing() which doesn't cover GC during normal execution. This fix uses the Connection.closed property (from PR microsoft#398) to detect closed connections.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
This pull request introduces a new
closedproperty to the connection class inmssql_python/connection.py, providing a clear and explicit way to check if a connection has been closed. Comprehensive tests have been added to ensure correct behavior of this property under various scenarios, improving reliability and clarity for users of the connection API.Connection class enhancements:
closedproperty to the connection class, which returnsTrueifclose()was called, andFalseotherwise. This property does not indicate connection health, only whetherclose()was explicitly invoked.Test coverage improvements:
closedproperty reflects the connection state before and after callingclose().close()multiple times is safe and theclosedproperty remainsTrue(idempotency).closedproperty when using the connection as a context manager, ensuring it isTrueafter exiting the block.closedproperty accurately reflects the closed state.