FIX: Release GIL around teardown SQLFreeHandle/SQLFreeStmt (#565)

[Saurabh Singh (saurabh500)]

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:

• test_param_query_close_through_python_tcp_forwarder_does_not_deadlock
  reproduces jschuba's repro: parametrized SELECT + fetch + conn.close().
• test_param_describe_through_python_tcp_forwarder_does_not_deadlock
  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).

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 92%

🎯 Overall Coverage 80%

📈 Total Lines Covered:6746 out of 8334 📁 Project:mssql-python

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql_python/pybind/ddbc_bindings.cpp (92.6%): Missing lines 1428-1429

Summary

• Total: 27 lines
• Missing: 2 lines
• Coverage: 92%

mssql_python/pybind/ddbc_bindings.cpp

Lines 1424-1433

1424// This is critical when the connection is reached through an in-process1425// Python TCP forwarder (e.g. paramiko + sshtunnel) - the forwarder1426// thread needs the GIL to push bytes, so holding it here deadlocks1427// (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.1430if (!pythonShuttingDown && PyGILState_Check()) {
1431 py::gil_scoped_release release;
1432SQLFreeHandle_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

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Copilot]

Pull request overview

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:

• Release the GIL around SQLDescribeParam during parameter binding when inferring unknown parameter types.
• Release the GIL around teardown calls (SQLFreeHandle, SQLFreeStmt(SQL_CLOSE)) to avoid deadlocking in close/free paths.
• Add two new watchdog-based regression tests that reproduce the SSH-tunnel / in-process forwarder deadlock scenarios.

Reviewed 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.