Skip to content

Commit 60642be

Browse files
saurabh500Copilot
andcommitted
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>
1 parent 83995e4 commit 60642be

File tree

3 files changed

+35
-21
lines changed

3 files changed

+35
-21
lines changed

mssql_python/pybind/connection/connection.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,13 @@ void Connection::disconnect() {
159159
// Destructor / shutdown path — GIL is not held, call directly.
160160
ret = SQLDisconnect_ptr(_dbcHandle->get());
161161
}
162-
checkError(ret);
162+
// In destructor/shutdown paths, suppress errors to avoid
163+
// std::terminate() if this throws during stack unwinding.
164+
if (hasGil) {
165+
checkError(ret);
166+
} else if (!SQL_SUCCEEDED(ret)) {
167+
LOG("SQLDisconnect failed in destructor/shutdown path; ignoring error");
168+
}
163169
// triggers SQLFreeHandle via destructor, if last owner
164170
_dbcHandle.reset();
165171
} else {

mssql_python/pybind/connection/connection_pool.cpp

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -105,14 +105,19 @@ void ConnectionPool::release(std::shared_ptr<Connection> conn) {
105105
_pool.push_back(conn);
106106
} else {
107107
should_disconnect = true;
108-
if (_current_size > 0)
109-
--_current_size;
110108
}
111109
}
112110
// Disconnect outside the mutex to avoid holding it during the
113111
// blocking ODBC call (which releases the GIL).
114112
if (should_disconnect) {
115-
conn->disconnect();
113+
try {
114+
conn->disconnect();
115+
} catch (const std::exception& ex) {
116+
LOG("ConnectionPool::release: disconnect failed: %s", ex.what());
117+
}
118+
std::lock_guard<std::mutex> lock(_mutex);
119+
if (_current_size > 0)
120+
--_current_size;
116121
}
117122
}
118123

@@ -181,18 +186,14 @@ void ConnectionPoolManager::configure(int max_size, int idle_timeout_secs) {
181186
}
182187

183188
void ConnectionPoolManager::closePools() {
184-
std::vector<std::shared_ptr<ConnectionPool>> pools_to_close;
185-
{
186-
std::lock_guard<std::mutex> lock(_manager_mutex);
187-
for (auto& [conn_str, pool] : _pools) {
188-
if (pool) {
189-
pools_to_close.push_back(pool);
190-
}
189+
std::lock_guard<std::mutex> lock(_manager_mutex);
190+
// Keep _manager_mutex held for the full close operation so that
191+
// acquireConnection()/returnConnection() cannot create or use pools
192+
// while closePools() is in progress.
193+
for (auto& [conn_str, pool] : _pools) {
194+
if (pool) {
195+
pool->close();
191196
}
192-
_pools.clear();
193-
}
194-
// Close pools outside _manager_mutex to avoid deadlock.
195-
for (auto& pool : pools_to_close) {
196-
pool->close();
197197
}
198+
_pools.clear();
198199
}

tests/test_009_pooling.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -283,16 +283,23 @@ def test_pool_release_overflow_disconnects_outside_mutex(conn_str):
283283
284284
When a connection is returned to a pool that is already at max_size,
285285
the connection must be disconnected. This exercises the overflow path in
286-
ConnectionPool::release() (connection_pool.cpp lines 107-110) where
287-
should_disconnect is set and disconnect happens outside the mutex.
286+
ConnectionPool::release() (connection_pool.cpp) where should_disconnect
287+
is set and disconnect happens outside the mutex.
288+
289+
With the current pool semantics, max_size limits total concurrent
290+
connections, so we acquire two connections with max_size=2, then shrink
291+
the pool to max_size=1 before returning them. The second close hits
292+
the overflow path.
288293
"""
289-
pooling(max_size=1, idle_timeout=30)
294+
pooling(max_size=2, idle_timeout=30)
290295

291-
# Open two connections — both succeed because the pool issues slots
292296
conn1 = connect(conn_str)
293297
conn2 = connect(conn_str)
294298

295-
# Close conn1 first — returned to the pool (pool now has 1 idle entry)
299+
# Shrink idle capacity so first close fills the pool and second overflows
300+
pooling(max_size=1, idle_timeout=30)
301+
302+
# Close conn1 — returned to the pool (pool now has 1 idle entry)
296303
conn1.close()
297304

298305
# Close conn2 — pool is full (1 idle already), so this connection

0 commit comments

Comments
 (0)