Address review comments: error handling, pool accounting, test fix

- connection.cpp: Suppress checkError() in destructor/shutdown path to avoid std::terminate() during stack unwinding - connection_pool.cpp release(): Move _current_size decrement after successful disconnect to keep pool accounting consistent on failure - connection_pool.cpp closePools(): Keep _manager_mutex held during close to prevent races with new pool creation - test_009_pooling.py: Fix overflow test to use max_size=2 for acquisition then shrink to max_size=1 before release, matching actual pool semantics where max_size limits concurrent connections Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
microsoft · saurabh500 · Apr 15, 2026 · Apr 6, 2026 · Apr 6, 2026 · Apr 6, 2026
commit 60642be1e7f5fde82327f7c863a07ba0ab6871cd
diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp
@@ -159,7 +159,13 @@ void Connection::disconnect() {
             // Destructor / shutdown path — GIL is not held, call directly.
             ret = SQLDisconnect_ptr(_dbcHandle->get());
         }
-        checkError(ret);
+        // In destructor/shutdown paths, suppress errors to avoid
+        // std::terminate() if this throws during stack unwinding.
+        if (hasGil) {
+            checkError(ret);
+        } else if (!SQL_SUCCEEDED(ret)) {
+            LOG("SQLDisconnect failed in destructor/shutdown path; ignoring error");
+        }
         // triggers SQLFreeHandle via destructor, if last owner
         _dbcHandle.reset();
     } else {

diff --git a/mssql_python/pybind/connection/connection_pool.cpp b/mssql_python/pybind/connection/connection_pool.cpp
@@ -105,14 +105,19 @@ void ConnectionPool::release(std::shared_ptr<Connection> conn) {
             _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();
+        try {
+            conn->disconnect();
+        } catch (const std::exception& ex) {
+            LOG("ConnectionPool::release: disconnect failed: %s", ex.what());
+        }
+        std::lock_guard<std::mutex> lock(_mutex);
+        if (_current_size > 0)
+            --_current_size;
     }
 }
 
@@ -181,18 +186,14 @@ void ConnectionPoolManager::configure(int max_size, int idle_timeout_secs) {
 }
 
 void ConnectionPoolManager::closePools() {
-    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);
-            }
+    std::lock_guard<std::mutex> lock(_manager_mutex);
+    // Keep _manager_mutex held for the full close operation so that
+    // acquireConnection()/returnConnection() cannot create or use pools
+    // while closePools() is in progress.
+    for (auto& [conn_str, pool] : _pools) {
+        if (pool) {
+            pool->close();
         }
-        _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_009_pooling.py b/tests/test_009_pooling.py
@@ -283,16 +283,23 @@ def test_pool_release_overflow_disconnects_outside_mutex(conn_str):
 
     When a connection is returned to a pool that is already at max_size,
     the connection must be disconnected. This exercises the overflow path in
-    ConnectionPool::release() (connection_pool.cpp lines 107-110) where
-    should_disconnect is set and disconnect happens outside the mutex.
+    ConnectionPool::release() (connection_pool.cpp) where should_disconnect
+    is set and disconnect happens outside the mutex.
+
+    With the current pool semantics, max_size limits total concurrent
+    connections, so we acquire two connections with max_size=2, then shrink
+    the pool to max_size=1 before returning them. The second close hits
+    the overflow path.
     """
-    pooling(max_size=1, idle_timeout=30)
+    pooling(max_size=2, idle_timeout=30)
 
-    # Open two connections — both succeed because the pool issues slots
     conn1 = connect(conn_str)
     conn2 = connect(conn_str)
 
-    # Close conn1 first — returned to the pool (pool now has 1 idle entry)
+    # Shrink idle capacity so first close fills the pool and second overflows
+    pooling(max_size=1, idle_timeout=30)
+
+    # Close conn1 — returned to the pool (pool now has 1 idle entry)
     conn1.close()
 
     # Close conn2 — pool is full (1 idle already), so this connection