FIX: Release GIL around blocking SQLSetConnectAttr calls (#565)

mssql_python/pybind/connection/connection.cpp

@@ -211,9 +211,16 @@ void Connection::setAutocommit(bool enable) {
     }
     SQLINTEGER value = enable ? SQL_AUTOCOMMIT_ON : SQL_AUTOCOMMIT_OFF;
     LOG("Setting autocommit=%d", enable);
-    SQLRETURN ret =
-        SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT,
-                              reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(value)), 0);
+    SQLRETURN ret;
+    {
+        // Release the GIL during the blocking ODBC call. Holding the GIL
+        // here can deadlock when the network path goes through another
+        // Python thread (e.g. an in-process SSH tunnel via paramiko +
+        // sshtunnel), since that thread also needs the GIL to run.
+        py::gil_scoped_release release;
+        ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT,
+                                    reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(value)), 0);
+    }
     checkError(ret);
     if (value == SQL_AUTOCOMMIT_ON) {
         LOG("Autocommit enabled");
@@ -286,9 +293,15 @@ SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) {
         // Get the integer value
         int64_t longValue = value.cast<int64_t>();
 
-        SQLRETURN ret = SQLSetConnectAttr_ptr(
-            _dbcHandle->get(), attribute,
-            reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(longValue)), SQL_IS_INTEGER);
+        SQLRETURN ret;
+        {
+            // Release the GIL around the ODBC call for consistency with the
+            // other connection-attribute paths; some attributes can block.
+            py::gil_scoped_release release;
+            ret = SQLSetConnectAttr_ptr(
+                _dbcHandle->get(), attribute,
+                reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(longValue)), SQL_IS_INTEGER);
+        }
 
         if (!SQL_SUCCEEDED(ret)) {
             LOG("Failed to set integer attribute=%d, ret=%d", attribute, ret);
@@ -302,11 +315,19 @@ SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) {
 
             SQLPOINTER ptr;
             SQLINTEGER length;
-
-            ptr = reinterpretU16stringAsSqlWChar(this->wstrStringBuffer);
-            length = static_cast<SQLINTEGER>(this->wstrStringBuffer.length() * sizeof(SQLWCHAR));
-
-            SQLRETURN ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), attribute, ptr, length);
+            // Copy to a stack-local buffer so that releasing the GIL below
+            // doesn't expose a race where another thread overwrites the
+            // member wstrStringBuffer (and reallocates) while the ODBC
+            // driver is still reading from ptr.
+            std::u16string localStrBuffer = this->wstrStringBuffer;
+            ptr = reinterpretU16stringAsSqlWChar(localStrBuffer);
+            length = static_cast<SQLINTEGER>(localStrBuffer.length() * sizeof(SQLWCHAR));
+
+            SQLRETURN ret;
+            {
+                py::gil_scoped_release release;
+                ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), attribute, ptr, length);
+            }
             if (!SQL_SUCCEEDED(ret)) {
                 LOG("Failed to set string attribute=%d, ret=%d", attribute, ret);
             } else {
@@ -319,13 +340,18 @@ SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) {
         }
     } else if (py::isinstance<py::bytes>(value) || py::isinstance<py::bytearray>(value)) {
         try {
-            std::string binary_data = value.cast<std::string>();
-            this->strBytesBuffer.clear();
-            this->strBytesBuffer = std::move(binary_data);
-            SQLPOINTER ptr = const_cast<char*>(this->strBytesBuffer.c_str());
-            SQLINTEGER length = static_cast<SQLINTEGER>(this->strBytesBuffer.size());
-
-            SQLRETURN ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), attribute, ptr, length);
+            // Copy to a stack-local buffer so that releasing the GIL
+            // doesn't expose a race where another thread overwrites the
+            // member strBytesBuffer while the driver reads from ptr.
+            std::string localBytesBuffer = value.cast<std::string>();
+            SQLPOINTER ptr = const_cast<char*>(localBytesBuffer.c_str());
+            SQLINTEGER length = static_cast<SQLINTEGER>(localBytesBuffer.size());
+
+            SQLRETURN ret;
+            {
+                py::gil_scoped_release release;
+                ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), attribute, ptr, length);
+            }
             if (!SQL_SUCCEEDED(ret)) {
                 LOG("Failed to set binary attribute=%d, ret=%d", attribute, ret);
             } else {
@@ -376,8 +402,14 @@ bool Connection::reset() {
         ThrowStdException("Connection handle not allocated");
     }
     LOG("Resetting connection via SQL_ATTR_RESET_CONNECTION");
-    SQLRETURN ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_RESET_CONNECTION,
-                                          (SQLPOINTER)SQL_RESET_CONNECTION_YES, SQL_IS_INTEGER);
+    SQLRETURN ret;
+    {
+        // Release the GIL around the ODBC call for consistency with the
+        // other connection-attribute paths; some attributes can block.
+        py::gil_scoped_release release;
+        ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_RESET_CONNECTION,
+                                    (SQLPOINTER)SQL_RESET_CONNECTION_YES, SQL_IS_INTEGER);
+    }
     if (!SQL_SUCCEEDED(ret)) {
         LOG("Failed to reset connection (ret=%d). Marking as dead.", ret);
         return false;
@@ -387,8 +419,11 @@ bool Connection::reset() {
     // Explicitly reset it to the default (SQL_TXN_READ_COMMITTED) to prevent
     // isolation level settings from leaking between pooled connection usages.
     LOG("Resetting transaction isolation level to READ COMMITTED");
-    ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_TXN_ISOLATION,
-                                (SQLPOINTER)SQL_TXN_READ_COMMITTED, SQL_IS_INTEGER);
+    {
+        py::gil_scoped_release release;
+        ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_TXN_ISOLATION,
+                                    (SQLPOINTER)SQL_TXN_READ_COMMITTED, SQL_IS_INTEGER);
+    }
     if (!SQL_SUCCEEDED(ret)) {
         LOG("Failed to reset transaction isolation level (ret=%d). Marking as dead.", ret);
         return false;

mssql_python/pybind/connection/connection_pool.cpp

@@ -17,12 +17,13 @@ std::shared_ptr<Connection> ConnectionPool::acquire(const std::u16string& connSt
     std::vector<std::shared_ptr<Connection>> to_disconnect;
     std::shared_ptr<Connection> valid_conn = nullptr;
     bool needs_connect = false;
+
+    // Phase 1: Prune stale connections (under mutex — no ODBC calls).
     {
         std::lock_guard<std::mutex> lock(_mutex);
         auto now = std::chrono::steady_clock::now();
         size_t before = _pool.size();
 
-        // Phase 1: Remove stale connections, collect for later disconnect
         _pool.erase(std::remove_if(_pool.begin(), _pool.end(),
                                    [&](const std::shared_ptr<Connection>& conn) {
                                        auto idle_time =
@@ -38,40 +39,62 @@ std::shared_ptr<Connection> ConnectionPool::acquire(const std::u16string& connSt
                     _pool.end());
 
         size_t pruned = before - _pool.size();
+        // Decrement _current_size eagerly so new slots can be reserved while
+        // stale connections are being disconnected (Phase 4).  This means
+        // _current_size tracks *reserved capacity* (pooled + checked-out +
+        // in-flight new), not necessarily live ODBC handles.
         _current_size = (_current_size >= pruned) ? (_current_size - pruned) : 0;
+    }
 
-        // Phase 2: Attempt to reuse healthy connections
-        while (!_pool.empty()) {
-            auto conn = _pool.front();
-            _pool.pop_front();
-            if (conn->isAlive()) {
-                if (!conn->reset()) {
-                    to_disconnect.push_back(conn);
-                    --_current_size;
-                    continue;
+    // Phase 2: Pop one candidate at a time and validate it outside the
+    // mutex.  isAlive() and reset() perform ODBC calls that release the
+    // GIL; calling them while holding the mutex would create a mutex/GIL
+    // lock-ordering deadlock when multiple threads acquire concurrently.
+    while (true) {
+        std::shared_ptr<Connection> candidate;
+        {
+            std::lock_guard<std::mutex> lock(_mutex);
+            if (_pool.empty()) {
+                // No more candidates — try to reserve a slot for a new connection.
+                if (_current_size < _max_size) {
+                    valid_conn = std::make_shared<Connection>(connStr, true);
+                    ++_current_size;
+                    needs_connect = true;
+                } else {
+                    // NOTE: Another thread may be validating a popped candidate
+                    // outside the mutex right now.  If that candidate fails, a
+                    // slot will open up — but we can't wait for it here without
+                    // adding a condition-variable retry loop.  This is an
+                    // acceptable trade-off: transient "pool full" errors under
+                    // heavy contention are rare and callers can retry.
+                    throw std::runtime_error("ConnectionPool::acquire: pool size limit reached");
                 }
-                valid_conn = conn;
                 break;
-            } else {
-                to_disconnect.push_back(conn);
-                --_current_size;
             }
+            candidate = _pool.front();
+            _pool.pop_front();
+        }
+
+        // Validate the candidate outside the mutex.
+        try {
+            if (candidate->isAlive() && candidate->reset()) {
+                valid_conn = candidate;
+                break;
+            }
+        } catch (const std::exception& ex) {
+            LOG("Candidate connection validation failed: %s", ex.what());
         }
 
-        // Reserve a slot for a new connection if none reusable.
-        // The actual connect() call happens outside the mutex to avoid
-        // holding the mutex during the blocking ODBC call (which releases
-        // the GIL and could otherwise cause a mutex/GIL deadlock).
-        if (!valid_conn && _current_size < _max_size) {
-            valid_conn = std::make_shared<Connection>(connStr, true);
-            ++_current_size;
-            needs_connect = true;
-        } else if (!valid_conn) {
-            throw std::runtime_error("ConnectionPool::acquire: pool size limit reached");
+        // Candidate is dead or reset failed — mark for disconnect and
+        // decrement the pool size.
+        to_disconnect.push_back(candidate);
+        {
+            std::lock_guard<std::mutex> lock(_mutex);
+            if (_current_size > 0) --_current_size;
         }
     }
 
-    // Phase 2.5: Connect the new connection outside the mutex.
+    // Phase 3: Connect the new connection outside the mutex.
     if (needs_connect) {
         try {
             valid_conn->connect(attrs_before);
@@ -85,7 +108,7 @@ std::shared_ptr<Connection> ConnectionPool::acquire(const std::u16string& connSt
         }
     }
 
-    // Phase 3: Disconnect expired/bad connections outside lock
+    // Phase 4: Disconnect expired/bad connections outside lock.
     for (auto& conn : to_disconnect) {
         try {
             conn->disconnect();