| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
The 1.7.x fix released the GIL around SQLDriverConnect, SQLSetConnectAttr, SQLEndTran and SQLDisconnect, which fixed the plain-connect deadlock through in-process Python TCP forwarders (paramiko + sshtunnel). However, when a parametrized SELECT leaves an open server-side cursor, the connection teardown cascade still blocks: SqlHandle::free() calls SQLFreeHandle on the STMT handle, which sends a SQL_CLOSE-equivalent network packet to the server. With the GIL held, the forwarder thread cannot run sock.sendall and the call deadlocks forever. This change releases the GIL around the three remaining teardown calls that may perform network I/O: * SqlHandle::free() -> SQLFreeHandle(STMT/DBC) * SqlHandle::close_cursor() -> SQLFreeStmt(SQL_CLOSE) * SQLFreeHandle_wrap() -> SQLFreeHandle Each release is gated on PyGILState_Check() AND !is_python_finalizing(), because these paths can also be reached from interpreter-shutdown cleanup where gil_scoped_release would crash with Fatal Python error: PyEval_SaveThread: ... the GIL is released Adds a second regression test (test_param_query_close_through_python_tcp_forwarder_does_not_deadlock) that reproduces the latest @jschuba scenario via an in-process TCP forwarder + parametrized SELECTs + conn.close() under a hard watchdog. Verified the new test hangs (watchdog fires at 30s) without the C++ change and passes (completes in <0.5s) with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 1424-1433 1424 // This is critical when the connection is reached through an in-process
1425 // Python TCP forwarder (e.g. paramiko + sshtunnel) - the forwarder
1426 // thread needs the GIL to push bytes, so holding it here deadlocks
1427 // (issue #565). Only release the GIL if it is actually held AND the
! 1428 // interpreter is not finalizing - gil_scoped_release is unsafe during
! 1429 // shutdown even if PyGILState_Check() reports the GIL as held.
1430 if (!pythonShuttingDown && PyGILState_Check()) {
1431 py::gil_scoped_release release;
1432 SQLFreeHandle_ptr(_type, _handle);
1433 } else {📋 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.connection.connection.cpp: 76.2%
mssql_python.pybind.ddbc_bindings.cpp: 76.3%
mssql_python.row.py: 76.9%
mssql_python.__init__.py: 77.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.6%🔗 Quick Links
|
Sorry, something went wrong.
Audit follow-up to the previous teardown fix. Identified one additional ODBC call site that performs a server round-trip while holding the GIL, which deadlocks when the connection is routed through an in-process Python TCP forwarder (the same #565-family pattern). Fixed call site: * BindParameters() -> SQLDescribeParam Issued by the driver as sp_describe_undeclared_parameters when a parameter has SQL_UNKNOWN_TYPE - hit on every parametrized query that includes a None value. Earlier drafts of this commit also wrapped Connection::isAlive(), Connection::getInfo() and Connection::getAutocommit(), but those were removed after empirical verification: SQL_ATTR_CONNECTION_DEAD and SQL_ATTR_AUTOCOMMIT GETs are driver-cached client-side state per the ODBC spec (MSDN explicitly states SQL_ATTR_CONNECTION_DEAD 'does not query the data source for activity'), and SQLGetInfo for the standard info types (SQL_DBMS_NAME, SQL_DATABASE_NAME, ...) returns data the driver cached at login time. None of these are network calls. Reached only from a Python-callable wrapper with the GIL held, so a plain py::gil_scoped_release is sufficient (no is_python_finalizing guard needed - that's only required for the destructor cascade fixed in the prior commit). Adds test_param_describe_through_python_tcp_forwarder_does_not_deadlock which runs a parametrized SELECT with a None argument through the in-process Python TCP forwarder under a 30s watchdog. Verified: the test hangs (watchdog fires at 30s) without the SQLDescribeParam GIL release and passes (<1s) with it. Full pytest suite: 1767 passed, 0 failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
This PR addresses deadlocks reported in #565 by releasing the Python GIL around specific ODBC calls that can block on server/network round-trips (notably during statement/connection teardown and parameter type introspection), which is critical when traffic is routed through an in-process Python TCP forwarder thread.
Changes:
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mssql_python/pybind/ddbc_bindings.cpp | Releases the GIL around specific blocking ODBC calls implicated in #565 deadlocks. |
| tests/test_023_ssh_tunnel_gil_release.py | Adds subprocess + watchdog regression tests covering teardown and introspection deadlock variants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
…-gil-release-teardown # Conflicts: # mssql_python/pybind/ddbc_bindings.cpp
Add an early SQL_INVALID_HANDLE check in SQLFreeHandle_wrap so a null (None) SqlHandlePtr passed from Python cannot segfault at Handle->get(), matching the pattern used by SQLResetStmt_wrap. Addresses Copilot review feedback on PR #604. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Release mssql-python v1.11.0. [AB#46332](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/46332) ### Summary Version bump to 1.11.0. Updates `mssql_python/__init__.py`, `setup.py`, and `PyPI_Description.md`. Bundled `mssql_py_core` bumped from 0.1.5 to 0.1.6. #### Bug Fixes - **SSH-tunnel / in-process forwarder deadlock** — Release the GIL around blocking ODBC round-trips in the teardown path and `SQLDescribeParam`, so `close()` and parametrized queries with `None` values no longer deadlock in-process TCP forwarder setups (#604, issue #565). - **BINARY/VARBINARY NULL parameters in temp tables and table variables** — Proactively resolve unknown NULL parameter types before binding and emit actionable `setinputsizes()` guidance on `SQL_VARCHAR` fallback (#654, issue #627). - **Context manager transaction semantics** — `Connection.__exit__` now commits on clean exit and rolls back on exception when `autocommit=False`, instead of always rolling back (#639, issue #635). - **macOS Apple Silicon import failure** — `configure_dylibs.sh` rewrites bundled ODBC dylib dependencies to `@loader_path` for every architecture in the universal2 wheel, fixing `import mssql_python` on clean Apple Silicon machines (#661, issue #656). - **Service Principal bulk copy freeze** — Fixed a GIL-deadlock in the Rust core that froze bulk copy under `ActiveDirectoryServicePrincipal` auth; picked up via the `mssql_py_core` 0.1.6 bump (#666, issue #662). #### Version Bump - `mssql_python/__init__.py`: `__version__ = "1.11.0"` - `setup.py`: `version="1.11.0"` - `PyPI_Description.md`: `## What's new in v1.11.0` section refreshed.
| Back | FazBrowse Home | New Git URL |
Work Item / Issue Reference
GitHub Issue: #565
Summary
Fixes two distinct GIL-held ODBC network round-trips that deadlock when the
connection is routed through an in-process Python TCP forwarder (the
SSH-tunnel-style scenario reported on issue #565). When the GIL is held
across a blocking network syscall, the Python-implemented forwarder thread
cannot acquire the GIL to pump bytes, and the whole interpreter wedges.
Two commits, two independent deadlock sites:
1. Teardown path (9a3b84c6)
SqlHandle::free(), SqlHandle::close_cursor() and SQLFreeHandle_wrap()
call SQLFreeHandle / SQLFreeStmt(SQL_CLOSE) while holding the GIL.
For a statement with an open server-side cursor, both of those issue a
network round-trip to close it. Hit on every conn.close() /
cursor.close() after a parametrized query — matches jschuba's latest repro
comment exactly.
Fix: release the GIL around those three calls. Because two of the three
sites are reachable from C++ destructors (which can run during interpreter
finalization), the release is guarded by !is_python_finalizing() && PyGILState_Check() to avoid PyEval_SaveThread aborts during shutdown.
2. Parametrized-query introspection path (898bd29a)
BindParameters() calls SQLDescribeParam while holding the GIL whenever
a parameter has SQL_UNKNOWN_TYPE (e.g. None argument). The MS ODBC
driver implements this as a sp_describe_undeclared_parameters server
round-trip, so it deadlocks the forwarder thread on every parametrized
query that includes a None value.
Fix: release the GIL around the SQLDescribeParam call.
Note on scope: an earlier draft of this audit also wrapped
Connection::isAlive(), Connection::getInfo() and
Connection::getAutocommit(). Those were dropped after empirical
verification — SQL_ATTR_CONNECTION_DEAD and SQL_ATTR_AUTOCOMMIT GETs
are driver-cached client-side state per the ODBC spec (MSDN explicitly
states SQL_ATTR_CONNECTION_DEAD "does not query the data source for
activity"), and SQLGetInfo for the standard info types returns data the
driver cached at login time. None of those are network calls, so the
GIL release was unnecessary.
Test coverage
Two new tests in tests/test_023_ssh_tunnel_gil_release.py, both using
the existing in-process TCP forwarder + 30s watchdog pattern:
reproduces jschuba's repro: parametrized SELECT + fetch + conn.close().
exercises a parametrized SELECT with a None argument (forces
SQLDescribeParam), plus Connection.getinfo(SQL_DBMS_NAME) and
conn.autocommit as local sanity probes.
Both tests reliably hang to the 30s watchdog timeout without the C++
changes and pass in <1s with them. Full pytest suite: 1767 passed,
58 skipped, 0 failed (~114s).