Release GIL during blocking ODBC connect/disconnect calls

saurabh500 · Copilot · saurabh500 · commit 3fa89317d1b8 · 2026-04-06T14:21:31.000Z
Release the Python GIL during SQLDriverConnect and SQLDisconnect calls
so that other Python threads can run concurrently during connection I/O
(DNS resolution, TCP handshake, TLS negotiation, SQL Server auth).

Changes:
- connection.cpp: Wrap SQLDriverConnect_ptr in py::gil_scoped_release
- connection.cpp: Conditionally release GIL around SQLDisconnect_ptr
  (guarded by PyGILState_Check for destructor safety)
- connection.cpp: Remove disconnect() from reset() failure paths to
  avoid GIL release inside pool mutex; callers handle cleanup
- connection_pool.cpp: Move connect() outside pool mutex in acquire()
  (reserve slot inside mutex, connect outside, rollback on failure)
- connection_pool.cpp: Move disconnect() outside mutex in release()
- connection_pool.cpp: Release _manager_mutex before calling pool
  acquire/release/close to prevent mutex/GIL lock-ordering deadlocks
- Add test_021_concurrent_connection_perf.py with stress tests proving
  concurrent connection parallelism and no GIL starvation

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp
@@ -85,8 +85,16 @@ void Connection::connect(const py::dict& attrs_before) {
 #else
     connStrPtr = const_cast<SQLWCHAR*>(_connStr.c_str());
 #endif
-    SQLRETURN ret = SQLDriverConnect_ptr(_dbcHandle->get(), nullptr, connStrPtr, SQL_NTS, nullptr,
-                                         0, nullptr, SQL_DRIVER_NOPROMPT);
+    SQLRETURN ret;
+    {
+        // Release the GIL during the blocking ODBC connect call.
+        // SQLDriverConnect involves DNS resolution, TCP handshake, TLS negotiation,
+        // and SQL Server authentication — all pure I/O that doesn't need the GIL.
+        // This allows other Python threads to run concurrently.
+        py::gil_scoped_release release;
+        ret = SQLDriverConnect_ptr(_dbcHandle->get(), nullptr, connStrPtr, SQL_NTS, nullptr,
+                                   0, nullptr, SQL_DRIVER_NOPROMPT);
+    }
     checkError(ret);
     updateLastUsed();
 }
@@ -95,6 +103,11 @@ void Connection::disconnect() {
     if (_dbcHandle) {
         LOG("Disconnecting from database");
 
+        // Check if we hold the GIL so we can conditionally release it.
+        // The GIL is held when called from pybind11-bound methods but may NOT
+        // be held in destructor paths (C++ shared_ptr ref-count drop, shutdown).
+        bool hasGil = PyGILState_Check() != 0;
+
         // CRITICAL FIX: Mark all child statement handles as implicitly freed
         // When we free the DBC handle below, the ODBC driver will automatically free
         // all child STMT handles. We need to tell the SqlHandle objects about this
@@ -135,7 +148,17 @@ void Connection::disconnect() {
             _allocationsSinceCompaction = 0;
         }  // Release lock before potentially slow SQLDisconnect call
 
-        SQLRETURN ret = SQLDisconnect_ptr(_dbcHandle->get());
+        SQLRETURN ret;
+        if (hasGil) {
+            // Release the GIL during the blocking ODBC disconnect call.
+            // This allows other Python threads to run while the network
+            // round-trip completes.
+            py::gil_scoped_release release;
+            ret = SQLDisconnect_ptr(_dbcHandle->get());
+        } else {
+            // Destructor / shutdown path — GIL is not held, call directly.
+            ret = SQLDisconnect_ptr(_dbcHandle->get());
+        }
         checkError(ret);
         // triggers SQLFreeHandle via destructor, if last owner
         _dbcHandle.reset();
@@ -375,7 +398,6 @@ bool Connection::reset() {
                                           (SQLPOINTER)SQL_RESET_CONNECTION_YES, SQL_IS_INTEGER);
     if (!SQL_SUCCEEDED(ret)) {
         LOG("Failed to reset connection (ret=%d). Marking as dead.", ret);
-        disconnect();
         return false;
     }
 
@@ -387,7 +409,6 @@ bool Connection::reset() {
                                 (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);
-        disconnect();
         return false;
     }
 
diff --git a/mssql_python/pybind/connection/connection_pool.cpp b/mssql_python/pybind/connection/connection_pool.cpp
@@ -16,6 +16,7 @@ std::shared_ptr<Connection> ConnectionPool::acquire(const std::wstring& connStr,
                                                     const py::dict& attrs_before) {
     std::vector<std::shared_ptr<Connection>> to_disconnect;
     std::shared_ptr<Connection> valid_conn = nullptr;
+    bool needs_connect = false;
     {
         std::lock_guard<std::mutex> lock(_mutex);
         auto now = std::chrono::steady_clock::now();
@@ -57,16 +58,33 @@ std::shared_ptr<Connection> ConnectionPool::acquire(const std::wstring& connStr,
             }
         }
 
-        // Create new connection if none reusable
+        // 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);
-            valid_conn->connect(attrs_before);
             ++_current_size;
+            needs_connect = true;
         } else if (!valid_conn) {
             throw std::runtime_error("ConnectionPool::acquire: pool size limit reached");
         }
     }
 
+    // Phase 2.5: Connect the new connection outside the mutex.
+    if (needs_connect) {
+        try {
+            valid_conn->connect(attrs_before);
+        } catch (...) {
+            // Connect failed — release the reserved slot
+            {
+                std::lock_guard<std::mutex> lock(_mutex);
+                if (_current_size > 0) --_current_size;
+            }
+            throw;
+        }
+    }
+
     // Phase 3: Disconnect expired/bad connections outside lock
     for (auto& conn : to_disconnect) {
         try {
@@ -79,14 +97,22 @@ std::shared_ptr<Connection> ConnectionPool::acquire(const std::wstring& connStr,
 }
 
 void ConnectionPool::release(std::shared_ptr<Connection> conn) {
-    std::lock_guard<std::mutex> lock(_mutex);
-    if (_pool.size() < _max_size) {
-        conn->updateLastUsed();
-        _pool.push_back(conn);
-    } else {
+    bool should_disconnect = false;
+    {
+        std::lock_guard<std::mutex> lock(_mutex);
+        if (_pool.size() < _max_size) {
+            conn->updateLastUsed();
+            _pool.push_back(conn);
+        } else {
+            should_disconnect = true;
+            if (_current_size > 0)
+                --_current_size;
+        }
+    }
+    // Disconnect outside the mutex to avoid holding it during the
+    // blocking ODBC call (which releases the GIL).
+    if (should_disconnect) {
         conn->disconnect();
-        if (_current_size > 0)
-            --_current_size;
     }
 }
 
@@ -116,21 +142,35 @@ ConnectionPoolManager& ConnectionPoolManager::getInstance() {
 
 std::shared_ptr<Connection> ConnectionPoolManager::acquireConnection(const std::wstring& connStr,
                                                                      const py::dict& attrs_before) {
-    std::lock_guard<std::mutex> lock(_manager_mutex);
-
-    auto& pool = _pools[connStr];
-    if (!pool) {
-        LOG("Creating new connection pool");
-        pool = std::make_shared<ConnectionPool>(_default_max_size, _default_idle_secs);
+    std::shared_ptr<ConnectionPool> pool;
+    {
+        std::lock_guard<std::mutex> lock(_manager_mutex);
+        auto& pool_ref = _pools[connStr];
+        if (!pool_ref) {
+            LOG("Creating new connection pool");
+            pool_ref = std::make_shared<ConnectionPool>(_default_max_size, _default_idle_secs);
+        }
+        pool = pool_ref;
     }
+    // Call acquire() outside _manager_mutex.  acquire() may release the GIL
+    // during the ODBC connect call; holding _manager_mutex across that would
+    // create a mutex/GIL lock-ordering deadlock.
     return pool->acquire(connStr, attrs_before);
 }
 
 void ConnectionPoolManager::returnConnection(const std::wstring& conn_str,
                                              const std::shared_ptr<Connection> conn) {
-    std::lock_guard<std::mutex> lock(_manager_mutex);
-    if (_pools.find(conn_str) != _pools.end()) {
-        _pools[conn_str]->release((conn));
+    std::shared_ptr<ConnectionPool> pool;
+    {
+        std::lock_guard<std::mutex> lock(_manager_mutex);
+        auto it = _pools.find(conn_str);
+        if (it != _pools.end()) {
+            pool = it->second;
+        }
+    }
+    // Call release() outside _manager_mutex to avoid deadlock.
+    if (pool) {
+        pool->release(conn);
     }
 }
 
@@ -141,11 +181,18 @@ void ConnectionPoolManager::configure(int max_size, int idle_timeout_secs) {
 }
 
 void ConnectionPoolManager::closePools() {
-    std::lock_guard<std::mutex> lock(_manager_mutex);
-    for (auto& [conn_str, pool] : _pools) {
-        if (pool) {
-            pool->close();
+    std::vector<std::shared_ptr<ConnectionPool>> pools_to_close;
+    {
+        std::lock_guard<std::mutex> lock(_manager_mutex);
+        for (auto& [conn_str, pool] : _pools) {
+            if (pool) {
+                pools_to_close.push_back(pool);
+            }
         }
+        _pools.clear();
+    }
+    // Close pools outside _manager_mutex to avoid deadlock.
+    for (auto& pool : pools_to_close) {
+        pool->close();
     }
-    _pools.clear();
 }
diff --git a/tests/test_021_concurrent_connection_perf.py b/tests/test_021_concurrent_connection_perf.py
