Skip to content

Commit 8e1f8fe

Browse files
subrata-mssubrata-msCopilot
authored
FIX: Preserve Arrow reader fetch exceptions (#712) (#718)
### 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#47281](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/47281) <!-- External contributors: GitHub Issue --> > GitHub Issue: #712 ------------------------------------------------------------------- ### Summary <!-- Insert your summary of changes below. Minimum 10 characters required. --> This pull request improves the robustness and test coverage of the cursor cleanup logic in the `mssql_python` package, particularly around error handling during fetch operations and resource cleanup. It also introduces a build check to ensure that the driver source code compiles cleanly with warnings treated as errors in Python 3.14 and above. **Error handling and cleanup improvements:** * Refactored the cleanup guard in `batch_generator()` in `cursor.py` to only skip cleanup if the cursor is `None`, closed, or has no `hstmt`, improving clarity and correctness. **Test coverage enhancements:** * Added tests in `test_004_cursor_arrow.py` to verify that fetch errors are properly propagated and not masked by defensive cleanup logic, both when cleanup is skipped and after normal cleanup. These tests use fake cursor objects to simulate various error and cleanup scenarios. **Build and compatibility checks:** * Introduced a new test in `test_004_cursor.py` to ensure that `cursor.py` compiles successfully with `SyntaxWarning` promoted to an error, as required by Python 3.14+ (PEP 765). This helps future-proof the codebase against upcoming Python changes. * Added necessary imports (`subprocess`, `sys`, `Path`) in `test_004_cursor.py` to support the new compilation test. <!-- ### 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: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 47c2d0b commit 8e1f8fe

3 files changed

Lines changed: 71 additions & 47 deletions

File tree

‎mssql_python/cursor.py‎

Lines changed: 45 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -3088,54 +3088,52 @@ def batch_generator():
30883088
# body. This is the single canonical cleanup site.
30893089
cur = cursor_ref[0]
30903090
cursor_ref[0] = None
3091-
if cur is None or cur.closed or cur.hstmt is None:
3092-
return
3093-
3094-
# 1) Drain diagnostics produced by the (possibly cancelled)
3095-
# fetch *before* SQL_CLOSE so we don't lose them.
3096-
try:
3097-
cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
3098-
except Exception as e: # pylint: disable=broad-exception-caught
3099-
logger.debug("arrow_reader cleanup: pre-close diag drain failed: %s", e)
3100-
3101-
# 2) Release the server-side cursor & locks while keeping the
3102-
# HSTMT and prepared plan intact, so the parent Cursor can
3103-
# be re-executed.
3104-
try:
3105-
cur.hstmt._close_cursor() # pylint: disable=protected-access
3106-
except Exception as e: # pylint: disable=broad-exception-caught
3107-
# Elevated to WARNING: unlike the diag-drain failures
3108-
# (which only cost us some warning text), a failed
3109-
# SQLFreeStmt(SQL_CLOSE) leaves the server-side cursor
3110-
# and its locks/tempdb resources open on SQL Server
3111-
# until this parent Cursor is closed or re-executed.
3112-
# DEBUG is typically disabled in production, so that
3113-
# leak would be invisible; WARNING makes it visible.
3114-
logger.warning(
3115-
"arrow_reader cleanup: _close_cursor failed (%s); "
3116-
"server-side cursor may remain open until this "
3117-
"Cursor is closed or re-executed",
3118-
e,
3119-
)
3120-
3121-
# 3) Drain diagnostics produced by SQL_CLOSE itself. This
3122-
# runs unconditionally because SQL_CLOSE can return
3123-
# SQL_SUCCESS_WITH_INFO (a *success* code) and still leave
3124-
# warning records on the HSTMT diag stack; the previous
3125-
# "only on failure" path would silently drop those.
3126-
try:
3127-
cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
3128-
except Exception as e: # pylint: disable=broad-exception-caught
3129-
logger.debug("arrow_reader cleanup: post-close diag drain failed: %s", e)
3091+
if not cur.closed and cur.hstmt is not None:
3092+
# 1) Drain diagnostics produced by the (possibly cancelled)
3093+
# fetch *before* SQL_CLOSE so we don't lose them.
3094+
try:
3095+
cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
3096+
except Exception as e: # pylint: disable=broad-exception-caught
3097+
logger.debug("arrow_reader cleanup: pre-close diag drain failed: %s", e)
3098+
3099+
# 2) Release the server-side cursor & locks while keeping the
3100+
# HSTMT and prepared plan intact, so the parent Cursor can
3101+
# be re-executed.
3102+
try:
3103+
cur.hstmt._close_cursor() # pylint: disable=protected-access
3104+
except Exception as e: # pylint: disable=broad-exception-caught
3105+
# Elevated to WARNING: unlike the diag-drain failures
3106+
# (which only cost us some warning text), a failed
3107+
# SQLFreeStmt(SQL_CLOSE) leaves the server-side cursor
3108+
# and its locks/tempdb resources open on SQL Server
3109+
# until this parent Cursor is closed or re-executed.
3110+
# DEBUG is typically disabled in production, so that
3111+
# leak would be invisible; WARNING makes it visible.
3112+
logger.warning(
3113+
"arrow_reader cleanup: _close_cursor failed (%s); "
3114+
"server-side cursor may remain open until this "
3115+
"Cursor is closed or re-executed",
3116+
e,
3117+
)
31303118

3131-
# 4) Reset cursor bookkeeping to a clean "no result set"
3132-
# state. rowcount becomes -1 to signal that the prior
3133-
# result is no longer meaningful.
3134-
try:
3135-
cur._clear_rownumber() # pylint: disable=protected-access
3136-
cur.rowcount = -1
3137-
except Exception as e: # pylint: disable=broad-exception-caught
3138-
logger.debug("arrow_reader cleanup: bookkeeping reset failed: %s", e)
3119+
# 3) Drain diagnostics produced by SQL_CLOSE itself. This
3120+
# runs unconditionally because SQL_CLOSE can return
3121+
# SQL_SUCCESS_WITH_INFO (a *success* code) and still leave
3122+
# warning records on the HSTMT diag stack; the previous
3123+
# "only on failure" path would silently drop those.
3124+
try:
3125+
cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
3126+
except Exception as e: # pylint: disable=broad-exception-caught
3127+
logger.debug("arrow_reader cleanup: post-close diag drain failed: %s", e)
3128+
3129+
# 4) Reset cursor bookkeeping to a clean "no result set"
3130+
# state. rowcount becomes -1 to signal that the prior
3131+
# result is no longer meaningful.
3132+
try:
3133+
cur._clear_rownumber() # pylint: disable=protected-access
3134+
cur.rowcount = -1
3135+
except Exception as e: # pylint: disable=broad-exception-caught
3136+
logger.debug("arrow_reader cleanup: bookkeeping reset failed: %s", e)
31393137

31403138
gen = batch_generator()
31413139
inner = pyarrow.RecordBatchReader.from_batches(schema, gen)

‎tests/test_004_cursor.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010

1111
import pytest
1212
import os
13+
import warnings
1314
from datetime import datetime, date, time, timedelta, timezone
15+
from pathlib import Path
1416
import time as time_module
1517
import decimal
1618
from contextlib import closing
@@ -108,6 +110,15 @@
108110
]
109111

110112

113+
def test_package_sources_compile_with_warnings_as_errors():
114+
"""Every package source must compile when warnings are promoted to errors."""
115+
package_dir = Path(__file__).parents[1] / "mssql_python"
116+
for source in sorted(package_dir.glob("*.py")):
117+
with warnings.catch_warnings():
118+
warnings.simplefilter("error")
119+
compile(source.read_text(encoding="utf-8"), str(source), "exec")
120+
121+
111122
def drop_table_if_exists(cursor, table_name):
112123
"""Drop the table if it exists"""
113124
try:

‎tests/test_004_cursor_arrow.py‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,21 @@ def close(self):
688688
# leak the server-side cursor or crash close()
689689

690690

691+
def test_arrow_reader_propagates_fetch_error_when_parent_cursor_is_closed(conn_str):
692+
"""Closing the parent cursor must not turn the next fetch error into end-of-stream."""
693+
conn = mssql_python.connect(conn_str)
694+
try:
695+
tmp = conn.cursor()
696+
reader = tmp.execute("select top 5 1 a from sys.objects").arrow_reader(batch_size=2)
697+
_ = reader.read_next_batch()
698+
tmp.close()
699+
700+
with pytest.raises(mssql_python.ProgrammingError, match="cursor is closed"):
701+
reader.read_next_batch()
702+
finally:
703+
conn.close()
704+
705+
691706
def test_arrow_reader_getattr_refuses_private_names(cursor: mssql_python.Cursor):
692707
"""__getattr__ refuses leading-underscore names so a partially-constructed
693708
instance during __del__ cannot recurse forever trying to resolve its own

0 commit comments

Comments
 (0)