From 84d8c2cddfa6d5612cb8a28fbc640e52cc1da6b9 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Fri, 25 Sep 2026 17:29:16 +0100 Subject: [PATCH 01/11] PERF: Revert pooled connection cleanup from PR #777 This reverts commit 2a86fc1f0306accecdbc6c563ca2408fad9e1fd6 while preserving subsequent result-metadata changes. Restores the prior pooling behavior; the pooled-transaction correctness issue fixed by #777 will need a replacement fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 6 - mssql_python/connection.py | 32 +- mssql_python/pybind/connection/connection.cpp | 251 ++---- mssql_python/pybind/connection/connection.h | 21 +- .../pybind/connection/connection_pool.cpp | 44 +- .../pybind/connection/connection_pool.h | 14 +- mssql_python/pybind/ddbc_bindings.cpp | 206 +++-- mssql_python/pybind/ddbc_bindings.h | 14 +- tests/test_006_exceptions.py | 98 --- tests/test_009_pooling.py | 824 +----------------- 10 files changed, 216 insertions(+), 1294 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cac01d4d..4d50047c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,12 +104,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), before; users should call `cursor.setinputsizes()` to work around this. ### Fixed -- **GH-754:** Pooled connections are now rolled back and restored to autocommit - mode before being parked. This prevents an empty transaction from remaining - visible on an idle SQL Server session after `Connection.close()`. Abandoned - native connections also roll back pending work before disconnecting during - normal object destruction. Statement-handle allocation and cleanup are - synchronized with disconnect, including cleanup invoked by cursor finalizers. - Bounded text fetched as UTF-16 now preserves leading U+FEFF and U+FFFE as payload rather than treating them as byte-order markers. This corrects row-wise `fetchone()`, `fetchmany()`, and `fetchall()` results, including diff --git a/mssql_python/connection.py b/mssql_python/connection.py index 1984f4979..295ae6ed7 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -2194,35 +2194,21 @@ def close(self) -> None: # Close the connection even if cursor cleanup had issues try: if self._conn: - autocommit_error = None - rollback_error = None - manual_commit = False - try: - manual_commit = not self._conn.get_autocommit() - except RuntimeError as e: - autocommit_error = e - if manual_commit: - # End caller work before native close. Pooled connections are - # additionally restored to autocommit by native check-in, - # which atomically discards them if sanitation fails. + if not self.autocommit: + # If autocommit is disabled, rollback any uncommitted changes + # This is important to ensure no partial transactions remain + # For autocommit True, this is not necessary as each statement is + # committed immediately logger.debug("Rolling back uncommitted changes before closing connection.") try: self._conn.rollback() except RuntimeError as e: - rollback_error = e + # Handle C++ layer RuntimeError with proper DB-API exception mapping + _raise_connection_error(e) # TODO: Check potential race conditions in case of multithreaded scenarios # Close the connection - try: - self._conn.close(manual_commit and rollback_error is None) - except RuntimeError as e: - _raise_connection_error(e) - finally: - self._conn = None - if rollback_error is not None: - # Preserve prior DB-API error mapping after deterministic cleanup. - _raise_connection_error(rollback_error) - if autocommit_error is not None: - _raise_connection_error(autocommit_error) + self._conn.close() + self._conn = None except Exception as e: logger.error(f"Error closing database connection: {e}") # Re-raise the connection close error as it's more critical diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 9c4304d2f..0fcedc8b9 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -5,7 +5,6 @@ #include "connection/connection_pool.h" #include "utf_utils.h" #include -#include #include #include #include @@ -19,17 +18,6 @@ #include "logger_bridge.hpp" #include "performance_counter.hpp" -static bool isPythonFinalizing() { - if (Py_IsInitialized() == 0) { - return true; - } -#if PY_VERSION_HEX >= 0x030D0000 - return Py_IsFinalizing() != 0; -#else - return _Py_IsFinalizing() != 0; -#endif -} - static SqlHandlePtr getEnvHandle() { static SqlHandlePtr envHandle = []() -> SqlHandlePtr { LOG("Allocating ODBC environment handle"); @@ -64,8 +52,8 @@ Connection::Connection(const std::u16string& conn_str, bool use_pool) allocateDbcHandle(); } -Connection::~Connection() noexcept { - disconnectNoThrow(); +Connection::~Connection() { + disconnect(); // fallback if user forgets to disconnect } // Allocates connection handle @@ -113,7 +101,7 @@ void Connection::connect(const py::dict& attrs_before) { updateLastUsed(); } -void Connection::disconnect(bool rollbackBeforeDisconnect) { +void Connection::disconnect() { PERF_TIMER("Connection::disconnect"); clearResultMetadata(); // Determine GIL state once, up front. disconnect() runs both from @@ -126,75 +114,47 @@ void Connection::disconnect(bool rollbackBeforeDisconnect) { // Py_IsInitialized() is checked first: after Py_Finalize() the interpreter is // gone and PyGILState_Check() is unreliable, so treat "not initialized" as // "no GIL" and skip all Python calls. (#671 follow-up) - bool hasGil = !isPythonFinalizing() && PyGILState_Check() != 0; + bool hasGil = Py_IsInitialized() != 0 && PyGILState_Check() != 0; if (_dbcHandle) { if (hasGil) { LOG("Disconnecting from database"); } - std::vector childHandles; + // 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 + // so they don't try to free the handles again during their destruction. + + // THREAD-SAFETY: Lock mutex to safely access _childStatementHandles + // This protects against concurrent allocStatementHandle() calls or GC finalizers size_t originalSize = 0, afterCompactSize = 0, badHandleCount = 0; - auto disconnectNative = [&]() { - // Serialize explicit child free() calls as well as destruction. - // This lock must be released before reacquiring the GIL or logging. - std::lock_guard cleanupLock(_cleanupState->mutex); - { - std::lock_guard lock(_childHandlesMutex); - originalSize = _childStatementHandles.size(); - _childStatementHandles.erase( - std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(), - [](const std::weak_ptr& wp) { return wp.expired(); }), - _childStatementHandles.end()); - afterCompactSize = _childStatementHandles.size(); - childHandles.reserve(afterCompactSize); - for (auto& weakHandle : _childStatementHandles) { - if (auto handle = weakHandle.lock()) { - if (handle->type() != SQL_HANDLE_STMT) { - ++badHandleCount; - continue; - } - childHandles.push_back(std::move(handle)); + { + std::lock_guard lock(_childHandlesMutex); + + // First compact: remove expired weak_ptrs (they're already destroyed) + originalSize = _childStatementHandles.size(); + _childStatementHandles.erase( + std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(), + [](const std::weak_ptr& wp) { return wp.expired(); }), + _childStatementHandles.end()); + afterCompactSize = _childStatementHandles.size(); + + for (auto& weakHandle : _childStatementHandles) { + if (auto handle = weakHandle.lock()) { + // SAFETY ASSERTION: Only STMT handles should be in this vector + // This is guaranteed by allocStatementHandle() which only creates STMT handles + // If this assertion fails, it indicates a serious bug in handle tracking + if (handle->type() != SQL_HANDLE_STMT) { + ++badHandleCount; + continue; // Skip marking to prevent leak } - } - } - if (rollbackBeforeDisconnect) { - // Explicit SQL transactions need manual mode for SQLEndTran. - // Never turn autocommit on here: that could commit abandoned work. - SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, - reinterpret_cast(SQL_AUTOCOMMIT_OFF), 0); - SQLEndTran_ptr(SQL_HANDLE_DBC, _dbcHandle->get(), SQL_ROLLBACK); - } - SQLRETURN result = SQLDisconnect_ptr(_dbcHandle->get()); - if (SQL_SUCCEEDED(result)) { - // Also cover children whose weak_ptr expired as their destructor - // began waiting for this gate: they cannot appear in the snapshot. - _cleanupState->disconnected = true; - std::lock_guard lock(_childHandlesMutex); - for (const auto& handle : childHandles) { handle->markImplicitlyFreed(); } - _childStatementHandles.clear(); - _allocationsSinceCompaction = 0; } - return result; - }; + _childStatementHandles.clear(); + _allocationsSinceCompaction = 0; + } // Release lock before potentially slow SQLDisconnect call - SQLRETURN ret; - if (hasGil) { - py::gil_scoped_release release; - ret = disconnectNative(); - } else { - ret = disconnectNative(); - } - if (!SQL_SUCCEEDED(ret)) { - if (hasGil) { - checkError(ret); - } else { - std::fputs("mssql-python: native disconnect failed\n", stderr); - } - // Keep ownership and child-handle tracking intact for a cleanup retry. - return; - } // Log after releasing _childHandlesMutex (#671): LOG()/LOG_ERROR() acquire // the GIL and must not run while a native mutex is held. Also gated on // hasGil so the GIL-less destructor / shutdown path never tries to log. @@ -208,6 +168,26 @@ void Connection::disconnect(bool rollbackBeforeDisconnect) { } } + 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()); + } + // 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)) { + // Intentionally no LOG() here: LOG() acquires the GIL internally + // via py::gil_scoped_acquire, which is unsafe during interpreter + // shutdown or stack unwinding (can deadlock or call std::terminate). + } // triggers SQLFreeHandle via destructor, if last owner _dbcHandle.reset(); } else if (hasGil) { @@ -215,39 +195,6 @@ void Connection::disconnect(bool rollbackBeforeDisconnect) { } } -void Connection::disconnectNoThrow() noexcept { - try { - if (isPythonFinalizing()) { - abandonDuringFinalization(); - return; - } - if (!_dbcHandle) { - return; - } - // disconnect() already supports GIL-less cleanup. Drop the GIL once so - // neither its diagnostics nor handle destruction can enter Python. - if (PyGILState_Check()) { - py::gil_scoped_release release; - disconnect(true); - } else { - disconnect(true); - } - } catch (...) { - std::fputs("mssql-python: unexpected failure during native connection cleanup\n", stderr); - } -} - -void Connection::abandonDuringFinalization() noexcept { - { - std::lock_guard lock(_childHandlesMutex); - _childStatementHandles.clear(); - _allocationsSinceCompaction = 0; - } - // SqlHandle::free() already suppresses SQLFreeHandle during finalization. - // Clearing the shared pointer leaves process teardown to the operating system. - _dbcHandle.reset(); -} - // TODO(microsoft): Add an exception class in C++ for error handling, // DB spec compliant void Connection::checkError(SQLRETURN ret) const { @@ -360,30 +307,22 @@ bool Connection::getAutocommit() const { SqlHandlePtr Connection::allocStatementHandle() { PERF_TIMER("Connection::allocStatementHandle"); + if (!_dbcHandle) { + ThrowStdException("Connection handle not allocated"); + } + updateLastUsed(); LOG("Allocating statement handle"); - // Keep the wrapper outside the lock scope: unwinding a failed registration - // frees the statement through the same cleanup gate. - SqlHandlePtr stmtHandle; + SQLHANDLE stmt = nullptr; + SQLRETURN ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &stmt); + checkError(ret); + auto stmtHandle = std::make_shared(static_cast(SQL_HANDLE_STMT), stmt); + + // THREAD-SAFETY: Lock mutex before modifying _childStatementHandles + // This protects against concurrent disconnect() or allocStatementHandle() calls, + // or GC finalizers running from different threads bool compacted = false; size_t compactBefore = 0, compactAfter = 0; { - py::gil_scoped_release release; - std::lock_guard cleanupLock(_cleanupState->mutex); - if (_cleanupState->disconnected || !_dbcHandle) { - ThrowStdException("Connection handle not allocated"); - } - updateLastUsed(); - SQLHANDLE stmt = nullptr; - SQLRETURN ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &stmt); - if (!SQL_SUCCEEDED(ret)) { - // Snapshot diagnostics before disconnect can overwrite/free the DBC. - ErrorInfo err = SQLReadError(SQL_HANDLE_DBC, _dbcHandle->get(), ret); - ThrowStdException(err.sqlState.length() == 5 - ? "SQLSTATE:" + err.sqlState + ":" + err.ddbcErrorMsg - : err.ddbcErrorMsg); - } - stmtHandle = std::make_shared(static_cast(SQL_HANDLE_STMT), - stmt, _cleanupState); std::lock_guard lock(_childHandlesMutex); // Track this child handle so we can mark it as implicitly freed when connection closes @@ -649,26 +588,6 @@ bool Connection::reset() { return true; } -void Connection::prepareForPool(bool transactionAlreadyRolledBack) { - if (!_dbcHandle) { - ThrowStdException("Connection handle not allocated"); - } - - // Explicit BEGIN TRANSACTION is valid while ODBC autocommit is on, but - // SQLEndTran does not end that transaction until the connection enters - // manual-commit mode. - if (getAutocommit()) { - setAutocommit(false); - } - if (!transactionAlreadyRolledBack) { - rollback(); - } - // The SQL Server ODBC driver can leave an empty transaction visible after - // SQLEndTran while manual-commit mode remains enabled, so always park the - // physical connection in autocommit mode. - setAutocommit(true); -} - void Connection::updateLastUsed() { _lastUsed = std::chrono::steady_clock::now(); } @@ -736,8 +655,7 @@ ConnectionHandle::ConnectionHandle(const std::u16string& connStr, bool usePool, PERF_TIMER("ConnectionHandle::ConnectionHandle"); if (_usePool) { _conn = ConnectionPoolManager::getInstance().acquireConnection(_connStr, attrsBefore, - _poolKey, tokenFactory, - &_originPool); + _poolKey, tokenFactory); // acquireConnection returns nullptr when pooling was disabled out from // under us (a disable_pooling() won the race). Fall back to a non-pooled // connection and flip _usePool so close() disconnects it directly rather @@ -765,42 +683,17 @@ ConnectionHandle::ConnectionHandle(const std::u16string& connStr, bool usePool, ConnectionHandle::~ConnectionHandle() { if (_conn) { - if (isPythonFinalizing()) { - _conn->abandonDuringFinalization(); - _conn = nullptr; - return; - } - try { - // Discard ends abandoned work without returning this connection to - // the pool or entering Python from a native destructor. - ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn); - } catch (...) { - std::fputs("mssql-python: failed to release native connection pool capacity\n", stderr); - _conn->disconnectNoThrow(); - } + close(); } } -void ConnectionHandle::close(bool transactionAlreadyRolledBack) { +void ConnectionHandle::close() { PERF_TIMER("ConnectionHandle::close"); if (!_conn) { ThrowStdException("Connection object is not initialized"); } if (_usePool) { - try { - _conn->prepareForPool(transactionAlreadyRolledBack); - } catch (...) { - // Never retain a connection whose transaction state could not be - // sanitized. Discarding also releases this connection's reserved - // pool capacity. Preserve the original check-in error. - try { - ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn); - } catch (...) { - } - _conn = nullptr; - throw; - } - ConnectionPoolManager::getInstance().returnConnection(_poolKey, _originPool, _conn); + ConnectionPoolManager::getInstance().returnConnection(_poolKey, _conn); } else { _conn->disconnect(); } @@ -840,12 +733,10 @@ bool ConnectionHandle::getAutocommit() const { SqlHandlePtr ConnectionHandle::allocStatementHandle() { PERF_TIMER("ConnectionHandle::allocStatementHandle"); - // close() can detach _conn while allocation waits without the GIL. - auto conn = _conn; - if (!conn) { + if (!_conn) { ThrowStdException("Connection object is not initialized"); } - return conn->allocStatementHandle(); + return _conn->allocStatementHandle(); } py::object Connection::getInfo(SQLUSMALLINT infoType) const { diff --git a/mssql_python/pybind/connection/connection.h b/mssql_python/pybind/connection/connection.h index f43613842..c9df35206 100644 --- a/mssql_python/pybind/connection/connection.h +++ b/mssql_python/pybind/connection/connection.h @@ -32,19 +32,13 @@ class Connection { public: Connection(const std::u16string& connStr, bool fromPool); - ~Connection() noexcept; + ~Connection(); // Establish the connection using the stored connection string. void connect(const py::dict& attrs_before = py::dict()); // Disconnect and free the connection handle. - void disconnect(bool rollbackBeforeDisconnect = false); - - // Roll back and disconnect without Python callbacks or escaping exceptions. - void disconnectNoThrow() noexcept; - - // Relinquish native handles without ODBC calls during interpreter finalization. - void abandonDuringFinalization() noexcept; + void disconnect(); // Commit the current transaction. void commit(); @@ -59,7 +53,6 @@ class Connection { bool getAutocommit() const; bool isAlive() const; bool reset(); - void prepareForPool(bool transactionAlreadyRolledBack = false); void updateLastUsed(); std::chrono::steady_clock::time_point lastUsed() const; @@ -137,13 +130,8 @@ class Connection { // Prevents data races between allocStatementHandle() and disconnect(), // or concurrent GC finalizers running from different threads mutable std::mutex _childHandlesMutex; - // Child wrappers retain this gate even after the Connection is destroyed. - const std::shared_ptr _cleanupState = - std::make_shared(); }; -class ConnectionPool; - class ConnectionHandle { public: ConnectionHandle(const std::u16string& connStr, bool usePool, @@ -152,7 +140,7 @@ class ConnectionHandle { const py::object& tokenFactory = py::object()); ~ConnectionHandle(); - void close(bool transactionAlreadyRolledBack = false); + void close(); void commit(); void rollback(); void setAutocommit(bool enabled); @@ -172,7 +160,4 @@ class ConnectionHandle { // Entra access-token auth so distinct identities never share a pool. // Empty is never stored; the ctor falls back to _connStr. std::u16string _poolKey; - // Identifies the exact pool generation that issued _conn. A weak reference - // prevents a checked-out connection from keeping a disabled pool alive. - std::weak_ptr _originPool; }; diff --git a/mssql_python/pybind/connection/connection_pool.cpp b/mssql_python/pybind/connection/connection_pool.cpp index 3ea3ccc5f..831a01db2 100644 --- a/mssql_python/pybind/connection/connection_pool.cpp +++ b/mssql_python/pybind/connection/connection_pool.cpp @@ -329,15 +329,6 @@ void ConnectionPool::release(std::shared_ptr conn) { } } -void ConnectionPool::discard(std::shared_ptr conn) { - { - std::lock_guard lock(_mutex); - if (_current_size > 0) - --_current_size; - } - conn->disconnectNoThrow(); -} - bool ConnectionPool::canEvict() { std::lock_guard lock(_mutex); // Never evict while any connection is checked out or in-flight. Reserved @@ -397,9 +388,7 @@ ConnectionPoolManager& ConnectionPoolManager::getInstance() { std::shared_ptr ConnectionPoolManager::acquireConnection(const std::u16string& connStr, const py::dict& attrs_before, const std::u16string& pool_key, - const py::object& token_factory, - std::weak_ptr* - originating_pool) { + const py::object& token_factory) { PERF_TIMER("ConnectionPoolManager::acquireConnection"); // Key the pool by pool_key when provided (identity-aware), // else fall back to the connection string (legacy behavior). @@ -461,9 +450,6 @@ std::shared_ptr ConnectionPoolManager::acquireConnection(const std:: created = true; } pool = pool_ref; - if (originating_pool) { - *originating_pool = pool; - } } // Log after releasing _manager_mutex (#671): LOG() acquires the GIL, and // holding a native mutex across a GIL acquisition deadlocks a thread that @@ -488,18 +474,18 @@ std::shared_ptr ConnectionPoolManager::acquireConnection(const std:: return pool->acquire(connStr, attrs_before, token_factory); } -void ConnectionPoolManager::returnConnection( - const std::u16string& pool_key, const std::weak_ptr& originating_pool, - const std::shared_ptr conn) { - std::shared_ptr pool = originating_pool.lock(); - bool registered = false; +void ConnectionPoolManager::returnConnection(const std::u16string& pool_key, + const std::shared_ptr conn) { + std::shared_ptr pool; { std::lock_guard lock(_manager_mutex); auto it = _pools.find(pool_key); - registered = pool && it != _pools.end() && it->second == pool; + if (it != _pools.end()) { + pool = it->second; + } } // Call release() outside _manager_mutex to avoid deadlock. - if (registered) { + if (pool) { pool->release(conn); } else { // No pool is registered under this key (e.g. the pool was lazily @@ -520,20 +506,6 @@ void ConnectionPoolManager::returnConnection( } } -void ConnectionPoolManager::discardConnection( - const std::weak_ptr& originating_pool, - const std::shared_ptr conn) { - if (!conn) { - return; - } - std::shared_ptr pool = originating_pool.lock(); - if (pool) { - pool->discard(conn); - } else { - conn->disconnectNoThrow(); - } -} - void ConnectionPoolManager::configure(int max_size, int idle_timeout_secs) { std::lock_guard lock(_manager_mutex); _default_max_size = max_size; diff --git a/mssql_python/pybind/connection/connection_pool.h b/mssql_python/pybind/connection/connection_pool.h index 6f67eab56..edc87c865 100644 --- a/mssql_python/pybind/connection/connection_pool.h +++ b/mssql_python/pybind/connection/connection_pool.h @@ -34,9 +34,6 @@ class ConnectionPool { // Returns a connection to the pool for reuse void release(std::shared_ptr conn); - // Permanently removes a checked-out connection and releases its capacity. - void discard(std::shared_ptr conn); - // Closes all connections in the pool, releasing resources void close(); @@ -74,8 +71,7 @@ class ConnectionPoolManager { std::shared_ptr acquireConnection( const std::u16string& conn_str, const py::dict& attrs_before = py::dict(), const std::u16string& pool_key = std::u16string(), - const py::object& token_factory = py::object(), - std::weak_ptr* originating_pool = nullptr); + const py::object& token_factory = py::object()); // Arms (true) or disarms (false) new-pool creation. Disarming, done under // _manager_mutex, guarantees that any acquireConnection() serialized after @@ -84,13 +80,7 @@ class ConnectionPoolManager { // Returns a connection to its original pool, identified by pool_key // (the same key passed to acquireConnection). - void returnConnection(const std::u16string& pool_key, - const std::weak_ptr& originating_pool, - std::shared_ptr conn); - - // Discards a connection from the exact pool generation that issued it. - void discardConnection(const std::weak_ptr& originating_pool, - std::shared_ptr conn); + void returnConnection(const std::u16string& pool_key, std::shared_ptr conn); // Closes all pools and their connections void closePools(); diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index f410631d4..36d06122e 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -1588,9 +1588,7 @@ void DriverLoader::loadDriver() { } // SqlHandle definition -SqlHandle::SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle, - std::shared_ptr cleanupState) - : _type(type), _handle(rawHandle), _cleanupState(std::move(cleanupState)) {} +SqlHandle::SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle) : _type(type), _handle(rawHandle) {} SqlHandle::~SqlHandle() { if (_handle) { @@ -1598,13 +1596,6 @@ SqlHandle::~SqlHandle() { } } -std::unique_lock SqlHandle::lockForCleanup() const { - if (_cleanupState) { - return std::unique_lock(_cleanupState->mutex); - } - return {}; -} - SQLHANDLE SqlHandle::get() const { return _handle; } @@ -1638,73 +1629,84 @@ void SqlHandle::markImplicitlyFreed() { * If you need destruction logs, use explicit close() methods instead. */ void SqlHandle::free() { - freeHandle(); -} - -SQLRETURN SqlHandle::freeHandle() { PERF_TIMER("SqlHandle::free"); resultMetadata.clear(); - bool pythonShuttingDown = is_python_finalizing(); - bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC; + if (_handle && SQLFreeHandle_ptr) { + // GH-610: Clear describe cache to prevent memory leak. + describeCache.clear(); + + // Check if Python is shutting down using centralized helper function + bool pythonShuttingDown = is_python_finalizing(); + + // RESOURCE LEAK MITIGATION: + // When handles are skipped during shutdown, they are not freed, which could + // cause resource leaks. However, this is mitigated by: + // 1. Python-side atexit cleanup (in __init__.py) that explicitly closes all + // connections before shutdown, ensuring handles are freed in correct order + // 2. OS-level cleanup at process termination recovers any remaining resources + // 3. This tradeoff prioritizes crash prevention over resource cleanup, which + // is appropriate since we're already in shutdown sequence + bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC; #ifdef _WIN32 - // The static ENV is destroyed during DLL_PROCESS_DETACH, after Python - // finalization. Calling ODBC then can access already-torn-down SSPI state. - skipDuringShutdown = skipDuringShutdown || _type == SQL_HANDLE_ENV; + // The static ENV is destroyed during DLL_PROCESS_DETACH, after Python + // finalization. Calling ODBC then can access already-torn-down SSPI state. + skipDuringShutdown = skipDuringShutdown || _type == SQL_HANDLE_ENV; #endif - if (pythonShuttingDown && skipDuringShutdown) { - // Do not wait for another thread's ODBC cleanup during finalization. - // Process teardown owns any resources not released by atexit cleanup. - _handle = nullptr; - return SQL_SUCCESS; - } - - auto freeNative = [this]() -> SQLRETURN { - auto cleanupLock = lockForCleanup(); - if (!_handle || !SQLFreeHandle_ptr) { - return SQL_INVALID_HANDLE; + if (pythonShuttingDown && skipDuringShutdown) { + _handle = nullptr; // Mark as freed to prevent double-free attempts + return; } - describeCache.clear(); - if (_implicitly_freed || (_cleanupState && _cleanupState->disconnected)) { - _handle = nullptr; - return SQL_SUCCESS; + + // CRITICAL FIX: Check if handle was already implicitly freed by parent handle + // When Connection::disconnect() frees the DBC handle, the ODBC driver automatically + // frees all child STMT handles. We track this state to avoid double-free attempts. + // This approach avoids calling ODBC functions on potentially-freed handles, which + // would cause use-after-free errors. + if (_implicitly_freed) { + _handle = nullptr; // Just clear the pointer, don't call ODBC functions + return; } - SQLRETURN ret = SQLFreeHandle_ptr(_type, _handle); - if (SQL_SUCCEEDED(ret)) { - _handle = nullptr; + + // Handle is valid and not implicitly freed, proceed with normal freeing. + // Release the GIL during the blocking ODBC call (SQLFreeHandle on a STMT + // with an open server-side cursor, or on a DBC, performs network I/O). + // This is critical when the connection is reached through an in-process + // Python TCP forwarder (e.g. paramiko + sshtunnel) - the forwarder + // thread needs the GIL to push bytes, so holding it here deadlocks + // (issue #565). Only release the GIL if it is actually held AND the + // interpreter is not finalizing - gil_scoped_release is unsafe during + // shutdown even if PyGILState_Check() reports the GIL as held. + if (!pythonShuttingDown && PyGILState_Check()) { + py::gil_scoped_release release; + SQLFreeHandle_ptr(_type, _handle); + } else { + SQLFreeHandle_ptr(_type, _handle); } - return ret; - }; - // The same gate is held through SQLDisconnect and child invalidation. - // Release the GIL before waiting, and unlock before reacquiring it. - if (!pythonShuttingDown && PyGILState_Check()) { - py::gil_scoped_release release; - return freeNative(); + _handle = nullptr; } - return freeNative(); } void SqlHandle::close_cursor() { resultMetadata.clear(); - if (is_python_finalizing()) { + if (_type != SQL_HANDLE_STMT || !_handle) { return; } - auto closeNative = [this]() -> SQLRETURN { - auto cleanupLock = lockForCleanup(); - if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed || - (_cleanupState && _cleanupState->disconnected)) { - return SQL_SUCCESS; - } - if (!SQLFreeStmt_ptr) { - ThrowStdException("SQLFreeStmt function not loaded"); - } - return SQLFreeStmt_ptr(_handle, SQL_CLOSE); - }; + if (_implicitly_freed) { + return; + } + if (!SQLFreeStmt_ptr) { + ThrowStdException("SQLFreeStmt function not loaded"); + } + // Release the GIL during the blocking SQLFreeStmt(SQL_CLOSE) network + // round-trip; see issue #565 (in-process forwarder deadlock). + // Skip GIL release when the GIL isn't held or the interpreter is + // finalizing - gil_scoped_release is unsafe in shutdown. SQLRETURN ret; - if (PyGILState_Check()) { + if (!is_python_finalizing() && PyGILState_Check()) { py::gil_scoped_release release; - ret = closeNative(); + ret = SQLFreeStmt_ptr(_handle, SQL_CLOSE); } else { - ret = closeNative(); + ret = SQLFreeStmt_ptr(_handle, SQL_CLOSE); } if (ret != SQL_SUCCESS && ret != SQL_SUCCESS_WITH_INFO) { ThrowStdException("SQLFreeStmt(SQL_CLOSE) failed"); @@ -1713,25 +1715,44 @@ void SqlHandle::close_cursor() { void SqlHandle::cancel() { resultMetadata.clear(); - if (is_python_finalizing()) { + // SQLCancel is intentionally lenient: it is a no-op on non-STMT handles, + // already-freed handles, or if the driver does not expose it. This lets + // _ArrowReader.close() call it unconditionally without coordinating with + // the fetch thread. The GIL is released so a blocked fetch thread can + // observe the cancel and return. + // + // Cross-thread invariant (why no mutex is needed): + // The only cross-thread pattern this driver blesses is exactly the one + // ODBC blesses: cancel() may be called from a thread *other than* the + // fetch thread to unblock an in-flight SQLFetch/SQLExecute on the same + // HSTMT. Per the ODBC spec, SQLCancel (with the SQLGetDiagRec/Field + // family) is the only entry point safe to call across threads on the + // same statement handle. All other operations on a Cursor/SqlHandle + // are single-owner: per DB API 2.0 and the Cursor thread-safety note + // in cursor.py, callers must not share a Cursor for its lifecycle + // operations (execute/fetch/close/free) across threads. Under that + // contract, free() / close_cursor() / SQLFreeHandle can never be in + // flight on this handle concurrently with cancel(), so the read of + // _handle above and the SQLCancel_ptr(h) call below cannot race a + // free() that clears _handle. + // + // A std::mutex here would only close the cancel()-vs-free() window; + // it would NOT close the (equally real) free()-vs-fetch window + // without also locking every fetch — which would serialize network + // I/O and defeat the whole point of cross-thread cancel. The right + // place to defend against a misuse (Cursor shared across threads for + // close vs. reader-cancel) is at the Python Cursor layer, not here. + if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed) { return; } - // Fetch/execute do not take this cleanup gate, so cross-thread cancellation - // can still interrupt them. Reader finalizers must not cancel a freed handle. - auto cancelNative = [this]() -> SQLRETURN { - auto cleanupLock = lockForCleanup(); - if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed || !SQLCancel_ptr || - (_cleanupState && _cleanupState->disconnected)) { - return SQL_SUCCESS; - } - return SQLCancel_ptr(_handle); - }; + if (!SQLCancel_ptr) { + return; + } + SQLHANDLE h = _handle; SQLRETURN ret; - if (PyGILState_Check()) { + { py::gil_scoped_release release; - ret = cancelNative(); - } else { - ret = cancelNative(); + ret = SQLCancel_ptr(h); } // SQLCancel may return SQL_SUCCESS_WITH_INFO when there was nothing to // cancel; that is fine. We only throw on hard failure. @@ -1913,22 +1934,19 @@ SQLRETURN SQLColumns_wrap(SqlHandlePtr StatementHandle, const py::object& catalo ErrorInfo SQLCheckError_Wrap(SQLSMALLINT handleType, SqlHandlePtr handle, SQLRETURN retcode) { PERF_TIMER("SQLCheckError_Wrap"); LOG("SQLCheckError: Checking ODBC errors - handleType=%d, retcode=%d", handleType, retcode); - if (retcode != SQL_INVALID_HANDLE && !SQL_SUCCEEDED(retcode) && !SQLGetDiagRec_ptr) { - LOG("SQLCheckError: SQLGetDiagRec function pointer not initialized, loading driver"); - DriverLoader::getInstance().loadDriver(); - } - return SQLReadError(handleType, handle ? handle->get() : nullptr, retcode); -} - -ErrorInfo SQLReadError(SQLSMALLINT handleType, SQLHANDLE rawHandle, SQLRETURN retcode) { ErrorInfo errorInfo; - if (retcode == SQL_INVALID_HANDLE || !rawHandle) { + if (retcode == SQL_INVALID_HANDLE) { + LOG("SQLCheckError: SQL_INVALID_HANDLE detected - handle is invalid"); errorInfo.ddbcErrorMsg = "Invalid handle!"; return errorInfo; } + assert(handle != 0); + SQLHANDLE rawHandle = handle->get(); if (!SQL_SUCCEEDED(retcode)) { if (!SQLGetDiagRec_ptr) { - ThrowStdException("SQLGetDiagRec function pointer not initialized"); + LOG("SQLCheckError: SQLGetDiagRec function pointer not " + "initialized, loading driver"); + DriverLoader::getInstance().loadDriver(); // Load the driver } SQLWCHAR sqlState[6], message[SQL_MAX_MESSAGE_LENGTH_SQLSERVER]; @@ -6342,16 +6360,25 @@ SQLRETURN SQLFreeHandle_wrap(SQLSMALLINT HandleType, SqlHandlePtr Handle) { LOG("SQLFreeHandle_wrap: Free SQL handle type=%d", HandleType); // Guard against a null/None handle being passed from Python - dereferencing // Handle->get() on a null shared_ptr would segfault. - if (!Handle || HandleType != Handle->type()) { + if (!Handle || !Handle->get()) { return SQL_INVALID_HANDLE; } - if (!SQLFreeHandle_ptr) { + if (!SQLAllocHandle_ptr) { LOG("SQLFreeHandle_wrap: Function pointer not initialized. Loading the " "driver."); DriverLoader::getInstance().loadDriver(); // Load the driver } - SQLRETURN ret = Handle->freeHandle(); + // Release the GIL during the blocking SQLFreeHandle network round-trip + // (see issue #565 - in-process Python TCP forwarder deadlock). + // Skip GIL release in shutdown paths where it would crash. + SQLRETURN ret; + if (!is_python_finalizing() && PyGILState_Check()) { + py::gil_scoped_release release; + ret = SQLFreeHandle_ptr(HandleType, Handle->get()); + } else { + ret = SQLFreeHandle_ptr(HandleType, Handle->get()); + } if (!SQL_SUCCEEDED(ret)) { LOG("SQLFreeHandle_wrap: SQLFreeHandle failed with error code - %d", ret); return ret; @@ -6471,8 +6498,7 @@ PYBIND11_MODULE(ddbc_bindings, m) { const py::object&>(), py::arg("conn_str"), py::arg("use_pool"), py::arg("attrs_before") = py::dict(), py::arg("pool_key") = std::u16string(), py::arg("token_factory") = py::none()) - .def("close", &ConnectionHandle::close, - py::arg("transaction_already_rolled_back") = false, "Close the connection") + .def("close", &ConnectionHandle::close, "Close the connection") .def("commit", &ConnectionHandle::commit, "Commit the current transaction") .def("rollback", &ConnectionHandle::rollback, "Rollback the current transaction") .def("set_autocommit", &ConnectionHandle::setAutocommit) diff --git a/mssql_python/pybind/ddbc_bindings.h b/mssql_python/pybind/ddbc_bindings.h index 32d9f8067..ebec0c79d 100644 --- a/mssql_python/pybind/ddbc_bindings.h +++ b/mssql_python/pybind/ddbc_bindings.h @@ -285,20 +285,13 @@ struct DescribedParamInfo { SQLSMALLINT decimalDigits; }; -struct ConnectionCleanupState { - std::mutex mutex; - bool disconnected = false; // Protected by mutex, shared with every child. -}; - class SqlHandle { public: - SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle, - std::shared_ptr cleanupState = nullptr); + SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle); ~SqlHandle(); SQLHANDLE get() const; SQLSMALLINT type() const; void free(); - SQLRETURN freeHandle(); void close_cursor(); // Cancel an in-progress statement (SQLCancel). Safe to call from a // thread other than the one running the fetch — this is the *only* @@ -333,12 +326,9 @@ class SqlHandle { ResultMetadataCache resultMetadata; private: - // The caller must release the GIL before waiting for native cleanup. - std::unique_lock lockForCleanup() const; SQLSMALLINT _type; SQLHANDLE _handle; bool _implicitly_freed = false; // Tracks if handle was freed by parent - std::shared_ptr _cleanupState; }; using SqlHandlePtr = std::shared_ptr; @@ -349,8 +339,6 @@ struct ErrorInfo { std::string ddbcErrorMsg; }; ErrorInfo SQLCheckError_Wrap(SQLSMALLINT handleType, SqlHandlePtr handle, SQLRETURN retcode); -// Driver must be initialized; reads diagnostics without Python logging/callbacks. -ErrorInfo SQLReadError(SQLSMALLINT handleType, SQLHANDLE handle, SQLRETURN retcode); // Thread-safe decimal separator accessor class class ThreadSafeDecimalSeparator { diff --git a/tests/test_006_exceptions.py b/tests/test_006_exceptions.py index 371b9662f..b0d117761 100644 --- a/tests/test_006_exceptions.py +++ b/tests/test_006_exceptions.py @@ -276,104 +276,6 @@ def test_connect_runtime_error_mapped_to_correct_dbapi_exception(): assert not isinstance(exc_info.value, RuntimeError) -def test_close_cleans_up_after_rollback_failure(): - """A rollback error must not defer native close to object destruction.""" - from unittest.mock import MagicMock, patch - - mock_conn = MagicMock() - mock_conn.get_autocommit.return_value = False - mock_conn.rollback.side_effect = RuntimeError("SQLSTATE:08S01:Communication link failure") - - with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): - conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") - - with pytest.raises(OperationalError, match="Communication link failure"): - conn.close() - - mock_conn.rollback.assert_called_once_with() - mock_conn.close.assert_called_once_with(False) - assert conn._conn is None - assert conn.closed - - -def test_close_cleans_up_after_autocommit_read_failure(): - """An autocommit read error must not bypass native close and handle release.""" - from unittest.mock import MagicMock, patch - - mock_conn = MagicMock() - mock_conn.get_autocommit.side_effect = RuntimeError("SQLSTATE:08S01:Communication link failure") - - with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): - conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") - - with pytest.raises(OperationalError, match="Communication link failure"): - conn.close() - - mock_conn.rollback.assert_not_called() - mock_conn.close.assert_called_once_with(False) - assert conn._conn is None - assert conn.closed - - -def test_close_reports_successful_rollback_to_native_cleanup(): - """Native pool cleanup must not repeat a successful Python rollback.""" - from unittest.mock import MagicMock, patch - - mock_conn = MagicMock() - mock_conn.get_autocommit.return_value = False - - with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): - conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") - - conn.close() - - mock_conn.rollback.assert_called_once_with() - mock_conn.close.assert_called_once_with(True) - - -def test_autocommit_close_delegates_transaction_cleanup_to_native(): - """Autocommit may still contain an explicit SQL transaction.""" - from unittest.mock import MagicMock, patch - - mock_conn = MagicMock() - mock_conn.get_autocommit.return_value = True - - with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): - conn = connect( - "Server=testserver;Database=mydb;Trusted_Connection=yes;", - autocommit=True, - ) - - conn.close() - - mock_conn.rollback.assert_not_called() - mock_conn.close.assert_called_once_with(False) - - -@pytest.mark.parametrize("preclose_failure", ["autocommit", "rollback"]) -def test_native_close_error_takes_precedence_over_preclose_failure(preclose_failure): - """The native close error wins, but the wrapper still releases its handle.""" - from unittest.mock import MagicMock, patch - - mock_conn = MagicMock() - if preclose_failure == "autocommit": - mock_conn.get_autocommit.side_effect = RuntimeError("SQLSTATE:08S01:Autocommit read failed") - else: - mock_conn.get_autocommit.return_value = False - mock_conn.rollback.side_effect = RuntimeError("SQLSTATE:08S01:Rollback failed") - mock_conn.close.side_effect = RuntimeError("SQLSTATE:08003:Native close failed") - - with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): - conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") - - with pytest.raises(OperationalError, match="Native close failed"): - conn.close() - - mock_conn.close.assert_called_once_with(False) - assert conn._conn is None - assert conn.closed - - def test_truncate_error_message_successful_cases(): """Test truncate_error_message with valid Microsoft messages for comparison.""" diff --git a/tests/test_009_pooling.py b/tests/test_009_pooling.py index c8cafed8f..e33f71030 100644 --- a/tests/test_009_pooling.py +++ b/tests/test_009_pooling.py @@ -127,187 +127,6 @@ def test_connection_pooling_reuse_spid(conn_str): assert spid1 == spid2, "Connections not reused - different SPIDs" -def test_pooled_close_paths_leave_no_open_transaction(conn_str): - """Every close path must leave the physical connection transaction-clean.""" - _run_in_subprocess( - """ - import os - import sys - - import mssql_python - - conn_str = os.environ["DB_CONNECTION_STRING"] - mssql_python.pooling(enabled=True, max_size=2, idle_timeout=30) - observer = mssql_python.connect(conn_str, autocommit=True) - try: - observer_cursor = observer.cursor() - - def open_transaction_count(session_id): - try: - observer_cursor.execute( - "SELECT open_transaction_count " - "FROM sys.dm_exec_sessions WHERE session_id = ?", - [session_id], - ) - except Exception as exc: - if "permission" in str(exc).lower(): - print( - "Test login cannot inspect another SQL Server session", - file=sys.stderr, - ) - sys.exit(77) - raise - return observer_cursor.fetchone() - - scenarios = ( - ("direct commit", False, "SELECT 1", None, "commit"), - ("prepared commit", False, "SELECT CAST(? AS INT)", [1], "commit"), - ("explicit rollback", False, "SELECT 1", None, "rollback"), - ("implicit close rollback", False, "SELECT 1", None, None), - ("autocommit close", True, "SELECT 1", None, None), - ( - "explicit transaction in autocommit", - True, - "BEGIN TRANSACTION; SELECT 1", - None, - None, - ), - ) - expected_spid = None - for name, autocommit, sql, params, action in scenarios: - subject = mssql_python.connect(conn_str, autocommit=autocommit) - try: - assert subject.autocommit is autocommit - cursor = subject.cursor() - cursor.execute("SELECT @@SPID") - subject_spid = cursor.fetchone()[0] - if expected_spid is None: - expected_spid = subject_spid - else: - assert subject_spid == expected_spid, ( - f"{name}: expected pooled SPID {expected_spid}, got {subject_spid}" - ) - - if open_transaction_count(subject_spid) is None: - print( - "Test login cannot inspect another SQL Server session", - file=sys.stderr, - ) - sys.exit(77) - - if params is None: - cursor.execute(sql) - else: - cursor.execute(sql, params) - cursor.fetchone() - if action == "commit": - subject.commit() - elif action == "rollback": - subject.rollback() - cursor.close() - finally: - subject.close() - - row = open_transaction_count(subject_spid) - assert row is not None, f"{name}: parked SQL Server session was not visible" - assert row[0] == 0, ( - f"{name}: pooled SPID {subject_spid} retained " - f"open_transaction_count={row[0]}" - ) - - observer_cursor.close() - finally: - observer.close() - mssql_python.pooling(enabled=False) - """, - conn_str, - ) - - -def test_autocommit_explicit_transaction_is_rolled_back_on_pool_checkin(conn_str): - """Autocommit normalization must not commit an explicit SQL transaction.""" - _run_in_subprocess( - """ - import os - - import mssql_python - - conn_str = os.environ["DB_CONNECTION_STRING"] - table = "pytest_pool_explicit_autocommit_transaction" - mssql_python.pooling(enabled=True, max_size=2, idle_timeout=30) - observer = mssql_python.connect(conn_str, autocommit=True) - try: - observer_cursor = observer.cursor() - observer_cursor.execute(f"DROP TABLE IF EXISTS {table}") - observer_cursor.execute(f"CREATE TABLE {table} (id INT PRIMARY KEY)") - - subject = mssql_python.connect(conn_str, autocommit=True) - subject_cursor = subject.cursor() - subject_cursor.execute("SELECT @@SPID") - subject_spid = subject_cursor.fetchone()[0] - try: - observer_cursor.execute( - "SELECT open_transaction_count " - "FROM sys.dm_exec_sessions WHERE session_id = ?", - [subject_spid], - ) - except Exception as exc: - if "permission" in str(exc).lower(): - import sys - - print( - "Test login cannot inspect another SQL Server session", - file=sys.stderr, - ) - sys.exit(77) - raise - row = observer_cursor.fetchone() - if row is None: - import sys - - print( - "Test login cannot inspect another SQL Server session", - file=sys.stderr, - ) - sys.exit(77) - - subject_cursor.execute(f"BEGIN TRANSACTION; INSERT INTO {table} VALUES (1)") - subject_cursor.close() - subject.close() - - observer_cursor.execute( - "SELECT open_transaction_count " - "FROM sys.dm_exec_sessions WHERE session_id = ?", - [subject_spid], - ) - row = observer_cursor.fetchone() - assert row is not None, "Previously visible pooled session disappeared on close" - assert row[0] == 0 - - observer_cursor.execute(f"SELECT COUNT(*) FROM {table}") - assert observer_cursor.fetchone()[0] == 0 - - reused = mssql_python.connect(conn_str, autocommit=True) - try: - reused_cursor = reused.cursor() - reused_cursor.execute("SELECT @@SPID, @@TRANCOUNT") - reused_spid, transaction_count = reused_cursor.fetchone() - assert reused_spid == subject_spid - assert transaction_count == 0 - reused_cursor.close() - finally: - reused.close() - - observer_cursor.execute(f"DROP TABLE {table}") - observer_cursor.close() - finally: - observer.close() - mssql_python.pooling(enabled=False) - """, - conn_str, - ) - - def test_connection_pooling_isolation_level_reset(conn_str): """Test that pooling correctly resets session state for isolation level. @@ -889,9 +708,9 @@ def session_identity(conn): spid, login_time = cur.fetchone() return (spid, login_time) - # Step 1: two distinct, autocommit connections. Autocommit keeps this - # test focused on detecting dead connections during checkout; failed - # manual-commit sanitation is covered separately below. + # Step 1: two distinct, autocommit connections. Autocommit avoids + # the implicit rollback in Connection.close(), which would + # otherwise fail on the killed session and leak its pool slot. victim = connect(conn_str) admin = connect(conn_str) victim.autocommit = True @@ -912,7 +731,7 @@ def session_identity(conn): admin.cursor().execute(f"KILL {victim_spid}") except Exception as e: msg = str(e) - if "does not have permission to use the kill statement" in msg.lower(): + if "permission" in msg.lower() or "KILL" in msg: import sys as _sys print( f"Skipping: KILL not permitted for this login: {msg}", @@ -932,12 +751,8 @@ def session_identity(conn): # login_time, so the identity check below catches the only # failure mode that matters. - # Step 3: close both. Sanitation of the killed connection should fail, - # discard it, and may surface that connection error to the caller. - try: - victim.close() - except Exception: - pass + # Step 3: return both to the pool. + victim.close() admin.close() # Step 4: re-acquire from the pool. Each must be working; the @@ -963,633 +778,6 @@ def session_identity(conn): ) -def test_failed_pool_sanitation_releases_capacity(conn_str): - """A connection discarded after failed sanitation must not consume a pool slot.""" - _run_in_subprocess( - """ - import os - import sys - import time - - from mssql_python import connect, pooling - from mssql_python.connection_string_builder import _ConnectionStringBuilder - from mssql_python.connection_string_parser import _ConnectionStringParser - - conn_str = os.environ["DB_CONNECTION_STRING"] - parsed = _ConnectionStringParser(validate_keywords=True)._parse(conn_str) - normalized = {} - for key, value in parsed.items(): - canonical = _ConnectionStringParser.normalize_key(key) - if canonical not in normalized: - normalized[canonical] = value - normalized["ConnectRetryCount"] = "0" - conn_str = _ConnectionStringBuilder(normalized).build() - pooling(max_size=2, idle_timeout=30) - victim = connect(conn_str) - admin = connect(conn_str, autocommit=True) - - victim_cursor = victim.cursor() - victim_cursor.execute("SELECT @@SPID") - victim_spid = victim_cursor.fetchone()[0] - victim_cursor.close() - - try: - admin.cursor().execute(f"KILL {victim_spid}") - except Exception as exc: - message = str(exc) - if "does not have permission to use the kill statement" in message.lower(): - print( - f"Skipping: KILL not permitted for this login: {message}", - file=sys.stderr, - ) - victim.close() - admin.close() - sys.exit(77) - raise - - deadline = time.monotonic() + 10 - while True: - try: - victim.cursor().execute("SELECT 1").fetchone() - except Exception: - break - if time.monotonic() >= deadline: - raise AssertionError("KILL did not terminate the victim connection") - time.sleep(0.05) - - try: - victim.close() - except Exception: - pass - else: - raise AssertionError("Expected pooled sanitation to fail after KILL") - - admin.close() - - first = connect(conn_str) - second = connect(conn_str) - try: - assert first.cursor().execute("SELECT 1").fetchone()[0] == 1 - assert second.cursor().execute("SELECT 1").fetchone()[0] == 1 - finally: - first.close() - second.close() - pooling(enabled=False) - """, - conn_str, - ) - - -def test_old_pool_generation_cannot_enter_replacement_pool(conn_str): - """A stale checked-out connection must not alter its replacement pool.""" - _run_in_subprocess( - """ - import os - - from mssql_python import connect, pooling - - conn_str = os.environ["DB_CONNECTION_STRING"] - pooling(max_size=2, idle_timeout=30) - old = connect(conn_str, autocommit=True) - old_cursor = old.cursor() - old_cursor.execute("SELECT @@SPID") - old_spid = old_cursor.fetchone()[0] - old_cursor.close() - - pooling(enabled=False) - pooling(enabled=True, max_size=2, idle_timeout=30) - first = connect(conn_str, autocommit=True) - second = connect(conn_str, autocommit=True) - try: - first_spid = first.cursor().execute("SELECT @@SPID").fetchone()[0] - second_spid = second.cursor().execute("SELECT @@SPID").fetchone()[0] - assert first_spid != second_spid - assert old_spid not in (first_spid, second_spid) - - old.close() - - try: - third = connect(conn_str, autocommit=True) - except Exception as exc: - assert "pool" in str(exc).lower() - else: - third.close() - raise AssertionError( - "Stale connection entered or decremented the replacement pool" - ) - finally: - old.close() - first.close() - second.close() - pooling(enabled=False) - """, - conn_str, - ) - - -def test_unclosed_native_handle_destructor_releases_pool_capacity(conn_str): - """Native destructor fallback must discard its checked-out pool slot.""" - _run_in_subprocess( - """ - import gc - import os - - import mssql_python - from mssql_python import connect, pooling - - conn_str = os.environ["DB_CONNECTION_STRING"] - pooling(max_size=1, idle_timeout=30) - wrapper = connect(conn_str, autocommit=True) - native = wrapper._conn - wrapper._conn = None - wrapper._closed = True - mssql_python._active_connections.discard(wrapper) - del wrapper - del native - gc.collect() - - replacement = connect(conn_str, autocommit=True) - try: - assert replacement.cursor().execute("SELECT 1").fetchone()[0] == 1 - finally: - replacement.close() - pooling(enabled=False) - """, - conn_str, - ) - - -@pytest.mark.parametrize("use_pool", [False, True]) -@pytest.mark.parametrize("autocommit", [False, True]) -def test_native_destructor_rolls_back_pending_dml(conn_str, use_pool, autocommit): - """Native destruction must release transactions, locks, and the server session.""" - _run_in_subprocess( - f"use_pool = {use_pool!r}\nautocommit = {autocommit!r}\n" + textwrap.dedent(""" - import gc - import os - import sys - import time - import uuid - - from mssql_python import connect, ddbc_bindings as ddbc, pooling - - conn_str = os.environ["DB_CONNECTION_STRING"] - pool_key = "pytest_native_cleanup_" + uuid.uuid4().hex - table = pool_key - pooling(max_size=1, idle_timeout=30) - observer = connect(conn_str, autocommit=True) - native = ddbc.Connection(conn_str, use_pool, {}, pool_key, None) - statement = native.alloc_statement_handle() - try: - assert ddbc.DDBCSQLExecDirect(statement, "SELECT @@SPID") in (0, 1) - row = [] - assert ddbc.DDBCSQLFetchOne(statement, row) in (0, 1) - session_id = row[0] - statement.free() - - cursor = observer.cursor() - try: - cursor.execute( - "SELECT session_id FROM sys.dm_exec_sessions WHERE session_id = ?", - [session_id], - ) - except Exception as exc: - if "permission" in str(exc).lower(): - print("Observer cannot inspect the native session", file=sys.stderr) - sys.exit(77) - raise - if cursor.fetchone() is None: - print("Observer cannot inspect the native session", file=sys.stderr) - sys.exit(77) - - cursor.execute("SET LOCK_TIMEOUT 1000") - cursor.execute(f"CREATE TABLE {table} (id INT)") - try: - native.set_autocommit(autocommit) - statement = native.alloc_statement_handle() - sql = f"INSERT INTO {table} VALUES (1)" - if autocommit: - sql = "BEGIN TRANSACTION; " + sql - assert ddbc.DDBCSQLExecDirect(statement, sql) in (0, 1) - statement.free() - statement = None - native = None - gc.collect() - - cursor.execute(f"SELECT COUNT(*) FROM {table} WITH (READCOMMITTEDLOCK)") - assert cursor.fetchone()[0] == 0, "Destructor committed abandoned work" - - deadline = time.monotonic() + 5 - while True: - cursor.execute( - "SELECT session_id FROM sys.dm_exec_sessions WHERE session_id = ?", - [session_id], - ) - if cursor.fetchone() is None: - break - assert time.monotonic() < deadline, "Native session survived destruction" - time.sleep(0.05) - - replacement = ddbc.Connection(conn_str, use_pool, {}, pool_key, None) - replacement_statement = replacement.alloc_statement_handle() - try: - assert ddbc.DDBCSQLExecDirect(replacement_statement, "SELECT 1") in (0, 1) - row = [] - assert ddbc.DDBCSQLFetchOne(replacement_statement, row) in (0, 1) - assert row == [1] - finally: - replacement_statement.free() - replacement.close() - finally: - cursor.execute(f"DROP TABLE {table}") - cursor.close() - finally: - if statement is not None: - statement.free() - if native is not None: - native.rollback() - native.close() - observer.close() - pooling(enabled=False) - """), - conn_str, - ) - - -@pytest.mark.parametrize("explicit_close", [False, True]) -def test_native_disconnect_with_concurrent_child_gc(conn_str, explicit_close): - """Child wrappers collected during disconnect must not double-free statements.""" - _run_in_subprocess( - f"explicit_close = {explicit_close!r}\n" + textwrap.dedent(""" - import gc - import os - import threading - - from mssql_python import ddbc_bindings as ddbc - - class StatementCycle: - def __init__(self, statement): - self.statement = statement - self.cycle = self - - barrier = threading.Barrier(2, timeout=10) - errors = [] - iterations = 50 - - def collect_children(): - try: - for _ in range(iterations): - barrier.wait() - gc.collect() - barrier.wait() - except Exception as exc: - errors.append(exc) - barrier.abort() - - gc.disable() - collector = threading.Thread(target=collect_children, daemon=True) - collector.start() - try: - for _ in range(iterations): - native = ddbc.Connection(os.environ["DB_CONNECTION_STRING"], False) - native.set_autocommit(True) - statement = native.alloc_statement_handle() - assert ddbc.DDBCSQLExecDirect(statement, "SELECT 1") in (0, 1) - cycle = StatementCycle(statement) - del statement, cycle - barrier.wait() - if explicit_close: - native.close() - native = None - barrier.wait() - finally: - collector.join(timeout=10) - if collector.is_alive(): - barrier.abort() - collector.join(timeout=10) - gc.enable() - assert not collector.is_alive(), "GC worker did not exit" - assert not errors, errors - gc.collect() - """), - conn_str, - ) - - -@pytest.mark.parametrize("explicit_close", [False, True]) -def test_cursor_cyclic_finalizer_with_concurrent_native_disconnect(conn_str, explicit_close): - """Exercise real Cursor.close/free after cyclic GC removes its WeakSet entry. - - The Python finalizer/WeakSet ordering is coordinated; overlap inside the - native cleanup calls is stress coverage, not a deterministic race trigger. - """ - _run_in_subprocess( - f"explicit_close = {explicit_close!r}\n" + textwrap.dedent(""" - import gc - import os - import threading - import weakref - - import mssql_python - from mssql_python import connect, ddbc_bindings as ddbc, pooling - - iterations = 50 - collect_barrier = threading.Barrier(2, timeout=10) - cleanup_barrier = threading.Barrier(2, timeout=10) - free_entered = threading.Event() - errors = [] - - class FinalizerStatement: - # Only coordinate entry: Cursor.__del__/close and native free - # still run their real implementations, with a real SQL handle. - def __init__(self, statement): - self.statement = statement - self.calls = 0 - self.completed = False - - def free(self): - self.calls += 1 - free_entered.set() - try: - cleanup_barrier.wait() - assert self.statement.free() is None - self.completed = True - except Exception as exc: - errors.append(f"Cursor finalizer: {exc!r}") - raise - - def collect_children(): - try: - for _ in range(iterations): - collect_barrier.wait() - gc.collect() - collect_barrier.wait() - except Exception as exc: - errors.append(f"GC worker: {exc!r}") - collect_barrier.abort() - cleanup_barrier.abort() - free_entered.set() - - pooling(enabled=False) - gc.disable() - collector = threading.Thread(target=collect_children, daemon=True) - collector.start() - connection = None - native = None - try: - for _ in range(iterations): - free_entered.clear() - connection = connect(os.environ["DB_CONNECTION_STRING"], autocommit=True) - cursor = connection.cursor() - assert cursor.execute("SELECT 1").fetchall()[0][0] == 1 - finalizer_statement = FinalizerStatement(cursor.hstmt) - cursor.hstmt = finalizer_statement - cursor.cycle = cursor - cursor_ref = weakref.ref(cursor) - del cursor - - collect_barrier.wait() - assert free_entered.wait(10), "Cursor finalizer did not enter free" - assert not errors, errors - assert cursor_ref() is None, "GC did not clear the cursor weakref" - assert not connection._cursors, "Connection.close would still see the cursor" - - if not explicit_close: - # The cursor retains its Python connection. Detach only - # the native owner to exercise its destructor fallback. - native = connection._conn - connection._conn = None - connection._closed = True - mssql_python._active_connections.discard(connection) - - cleanup_barrier.wait() - if explicit_close: - connection.close() - else: - native = None - collect_barrier.wait() - - assert finalizer_statement.calls == 1 - assert finalizer_statement.completed, errors - assert not errors, errors - assert finalizer_statement.statement.free() is None - assert ddbc.DDBCSQLFreeHandle(3, finalizer_statement.statement) == -2 - assert connection.closed - connection = None - collector.join(timeout=10) - assert not collector.is_alive(), "GC worker did not exit" - finally: - collect_barrier.abort() - cleanup_barrier.abort() - collector.join(timeout=10) - if connection is not None: - connection.close() - native = None - gc.enable() - assert not collector.is_alive(), "GC worker did not exit" - assert not errors, errors - gc.collect() - """), - conn_str, - ) - - -def test_failed_native_disconnect_preserves_child_statement(conn_str): - """SQLSTATE 25000 must not irreversibly invalidate a live child handle.""" - _run_in_subprocess( - """ - import os - import uuid - - from mssql_python import connect, ddbc_bindings as ddbc, pooling - - pooling(enabled=False) - conn_str = os.environ["DB_CONNECTION_STRING"] - table = "pytest_disconnect_failure_" + uuid.uuid4().hex - observer = connect(conn_str, autocommit=True) - observer_cursor = observer.cursor() - native = None - statement = None - created = False - try: - observer_cursor.execute("SET LOCK_TIMEOUT 1000") - observer_cursor.execute(f"CREATE TABLE {table} (id INT)") - created = True - native = ddbc.Connection(conn_str, False) - native.set_autocommit(False) - statement = native.alloc_statement_handle() - assert ddbc.DDBCSQLExecDirect(statement, f"INSERT INTO {table} VALUES (1)") in (0, 1) - - try: - native.close() - except RuntimeError as exc: - assert "25000" in str(exc), f"Unexpected disconnect failure: {exc}" - else: - raise AssertionError("Native disconnect accepted an uncommitted INSERT") - - extra_statement = native.alloc_statement_handle() - try: - assert ddbc.DDBCSQLExecDirect(extra_statement, "SELECT 42") in (0, 1) - extra_row = [] - assert ddbc.DDBCSQLFetchOne(extra_statement, extra_row) in (0, 1) - assert extra_row == [42] - finally: - extra_statement.free() - - assert ddbc.DDBCSQLExecDirect( - statement, f"SELECT COUNT(*), @@TRANCOUNT FROM {table}" - ) in (0, 1) - row = [] - assert ddbc.DDBCSQLFetchOne(statement, row) in (0, 1) - assert row[0] == 1 and row[1] > 0, row - statement._close_cursor() - native.rollback() - - assert ddbc.DDBCSQLExecDirect(statement, f"SELECT COUNT(*) FROM {table}") in (0, 1) - row = [] - assert ddbc.DDBCSQLFetchOne(statement, row) in (0, 1) - assert row == [0], "Failed disconnect committed the pending INSERT" - statement._close_cursor() - native.rollback() - native.close() - native = None - - # Disconnect already freed the ODBC statement. The raw entry point - # must consume the wrapper's implicit-free state, not the stale pointer. - assert ddbc.DDBCSQLFreeHandle(3, statement) in (0, 1) - assert ddbc.DDBCSQLFreeHandle(3, statement) == -2 - assert statement.free() is None - assert statement.free() is None - observer_cursor.execute(f"SELECT COUNT(*) FROM {table} WITH (READCOMMITTEDLOCK)") - assert observer_cursor.fetchone()[0] == 0 - finally: - try: - if native is not None: - try: - native.rollback() - finally: - native.close() - if statement is not None: - statement.free() - finally: - try: - if created: - observer_cursor.execute(f"DROP TABLE {table}") - finally: - observer_cursor.close() - observer.close() - """, - conn_str, - ) - - -@pytest.mark.parametrize("free_api", ["method", "raw"]) -def test_native_statement_free_entrypoints_are_idempotent(conn_str, free_api): - """Raw SQLRETURN and public None-returning free share one ownership state.""" - _run_in_subprocess( - f"free_api = {free_api!r}\n" + textwrap.dedent(""" - import os - - from mssql_python import ddbc_bindings as ddbc - - native = ddbc.Connection(os.environ["DB_CONNECTION_STRING"], False) - native.set_autocommit(True) - statement = native.alloc_statement_handle() - sibling = native.alloc_statement_handle() - try: - assert ddbc.DDBCSQLExecDirect(statement, "SELECT 1") in (0, 1) - if free_api == "raw": - assert ddbc.DDBCSQLFreeHandle(3, statement) in (0, 1) - else: - assert statement.free() is None - assert ddbc.DDBCSQLFreeHandle(3, statement) == -2 - assert statement.free() is None - assert statement.free() is None - - assert ddbc.DDBCSQLExecDirect(sibling, "SELECT 42") in (0, 1) - row = [] - assert ddbc.DDBCSQLFetchOne(sibling, row) in (0, 1) - assert row == [42] - native.close() - native = None - assert ddbc.DDBCSQLFreeHandle(3, sibling) in (0, 1) - assert ddbc.DDBCSQLFreeHandle(3, sibling) == -2 - assert sibling.free() is None - finally: - statement.free() - sibling.free() - if native is not None: - native.close() - """), - conn_str, - ) - - -def test_native_statement_allocation_racing_disconnect(conn_str): - """Allocation either registers before disconnect or rejects its closed state.""" - _run_in_subprocess( - """ - import os - import threading - - from mssql_python import ddbc_bindings as ddbc - - barrier = threading.Barrier(2, timeout=10) - errors = [] - statements = [] - iterations = 100 - native = None - - def allocate(): - try: - for _ in range(iterations): - barrier.wait() - try: - statements.append(native.alloc_statement_handle()) - except RuntimeError as exc: - assert str(exc) in ( - "Connection object is not initialized", - "Connection handle not allocated", - ), str(exc) - barrier.wait() - except Exception as exc: - errors.append(repr(exc)) - barrier.abort() - - worker = threading.Thread(target=allocate, daemon=True) - worker.start() - try: - for _ in range(iterations): - native = ddbc.Connection(os.environ["DB_CONNECTION_STRING"], False) - native.set_autocommit(True) - barrier.wait() - native.close() - barrier.wait() - assert not errors, errors - for statement in statements: - assert ddbc.DDBCSQLFreeHandle(3, statement) in (0, 1) - assert statement.free() is None - statements.clear() - try: - native.alloc_statement_handle() - except RuntimeError as exc: - assert "Connection object is not initialized" in str(exc) - else: - raise AssertionError("Allocation succeeded after native close") - finally: - worker.join(timeout=10) - if worker.is_alive(): - barrier.abort() - worker.join(timeout=10) - for statement in statements: - statement.free() - assert not worker.is_alive(), "Allocation worker did not exit" - assert not errors, errors - """, - conn_str, - ) - - def test_pool_recovery_after_failed_connection(conn_str): """Test that the pool recovers after a failed connection attempt.""" pooling(max_size=1, idle_timeout=30) From 489ba0ddb55294e3756afe5d8fcf6ecf30693939 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Fri, 25 Sep 2026 20:11:45 +0100 Subject: [PATCH 02/11] PERF: Preserve pooled transaction safety with a proven-clean fast path Replace the PR 777 revert with conservative clean-state tracking and consolidated native sanitation. Used or uncertain connections still roll back before parking; only previously sanitized unused connections can skip cleanup. Preserve scalar login timeouts without trusting arbitrary attributes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 13 + mssql_python/connection.py | 23 +- mssql_python/pybind/CMakeLists.txt | 18 + mssql_python/pybind/connection/connection.cpp | 354 +++++-- mssql_python/pybind/connection/connection.h | 26 +- .../pybind/connection/connection_pool.cpp | 44 +- .../pybind/connection/connection_pool.h | 14 +- mssql_python/pybind/ddbc_bindings.cpp | 206 ++-- mssql_python/pybind/ddbc_bindings.h | 14 +- tests/native/pool_sanitation.cpp | 614 ++++++++++++ tests/test_006_exceptions.py | 123 +++ tests/test_009_pooling.py | 886 +++++++++++++++++- 12 files changed, 2108 insertions(+), 227 deletions(-) create mode 100644 tests/native/pool_sanitation.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d50047c4..e374d8f23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), does not change the default provider or ship any Rust driver binaries. ### Changed +- Pooled check-in skips transaction sanitation only after a prior successful + native rollback/autocommit restore and no subsequent statement allocation or + uncertain operation. Used connections still roll back explicit transactions + before parking; sanitation uses one native attribute probe and one GIL release. + Raw-handle exposure and arbitrary connection attributes disable the fast path; + successfully applied scalar login timeouts (including `connect(timeout=30)`) + do not permanently disable it. - Fetches reuse owned native metadata for stable columns within a result set; `fetchmany()` avoids the Python metadata-dictionary roundtrip. Re-execution, result transitions and statement/connection cleanup invalidate this metadata. @@ -104,6 +111,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), before; users should call `cursor.setinputsizes()` to work around this. ### Fixed +- **GH-754:** Pooled connections are now rolled back and restored to autocommit + mode before being parked. This prevents an empty transaction from remaining + visible on an idle SQL Server session after `Connection.close()`. Abandoned + native connections also roll back pending work before disconnecting during + normal object destruction. Statement-handle allocation and cleanup are + synchronized with disconnect, including cleanup invoked by cursor finalizers. - Bounded text fetched as UTF-16 now preserves leading U+FEFF and U+FFFE as payload rather than treating them as byte-order markers. This corrects row-wise `fetchone()`, `fetchmany()`, and `fetchall()` results, including diff --git a/mssql_python/connection.py b/mssql_python/connection.py index 295ae6ed7..db6c812fb 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -2194,21 +2194,14 @@ def close(self) -> None: # Close the connection even if cursor cleanup had issues try: if self._conn: - if not self.autocommit: - # If autocommit is disabled, rollback any uncommitted changes - # This is important to ensure no partial transactions remain - # For autocommit True, this is not necessary as each statement is - # committed immediately - logger.debug("Rolling back uncommitted changes before closing connection.") - try: - self._conn.rollback() - except RuntimeError as e: - # Handle C++ layer RuntimeError with proper DB-API exception mapping - _raise_connection_error(e) - # TODO: Check potential race conditions in case of multithreaded scenarios - # Close the connection - self._conn.close() - self._conn = None + # Native close owns transaction cleanup and fail-closed discard. + # Avoid duplicate attribute probes and untrusted rollback hints. + try: + self._conn.close(rollback_before_disconnect=True) + except RuntimeError as e: + _raise_connection_error(e) + finally: + self._conn = None except Exception as e: logger.error(f"Error closing database connection: {e}") # Re-raise the connection close error as it's more critical diff --git a/mssql_python/pybind/CMakeLists.txt b/mssql_python/pybind/CMakeLists.txt index 77d599bd5..3be1cd0b0 100644 --- a/mssql_python/pybind/CMakeLists.txt +++ b/mssql_python/pybind/CMakeLists.txt @@ -393,3 +393,21 @@ if(APPLE) target_compile_definitions(ddbc_bindings PRIVATE MACOS_STRING_FIX) target_compile_options(ddbc_bindings PRIVATE -DAPPLE_SILICON) endif() + +# Exercise the real connection/pool/handle code against deterministic ODBC +# function pointers, without shipping test hooks in the extension. +option(BUILD_NATIVE_POOL_TESTS "Build database-free native pool sanitation tests" OFF) +if(BUILD_NATIVE_POOL_TESTS) + find_package(Python3 COMPONENTS Development REQUIRED) + get_target_property(pool_test_sources ddbc_bindings SOURCES) + add_executable(pool_sanitation_tests ${pool_test_sources} + ../../tests/native/pool_sanitation.cpp) + foreach(property INCLUDE_DIRECTORIES COMPILE_DEFINITIONS COMPILE_OPTIONS) + set_property(TARGET pool_sanitation_tests PROPERTY ${property} + $) + endforeach() + target_link_libraries(pool_sanitation_tests PRIVATE + Python3::Python simdutf::simdutf ${CMAKE_DL_LIBS}) + enable_testing() + add_test(NAME pool_sanitation COMMAND pool_sanitation_tests) +endif() diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 0fcedc8b9..653a15524 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -5,6 +5,8 @@ #include "connection/connection_pool.h" #include "utf_utils.h" #include +#include +#include #include #include #include @@ -18,6 +20,17 @@ #include "logger_bridge.hpp" #include "performance_counter.hpp" +static bool isPythonFinalizing() { + if (Py_IsInitialized() == 0) { + return true; + } +#if PY_VERSION_HEX >= 0x030D0000 + return Py_IsFinalizing() != 0; +#else + return _Py_IsFinalizing() != 0; +#endif +} + static SqlHandlePtr getEnvHandle() { static SqlHandlePtr envHandle = []() -> SqlHandlePtr { LOG("Allocating ODBC environment handle"); @@ -52,8 +65,8 @@ Connection::Connection(const std::u16string& conn_str, bool use_pool) allocateDbcHandle(); } -Connection::~Connection() { - disconnect(); // fallback if user forgets to disconnect +Connection::~Connection() noexcept { + disconnectNoThrow(); } // Allocates connection handle @@ -101,7 +114,7 @@ void Connection::connect(const py::dict& attrs_before) { updateLastUsed(); } -void Connection::disconnect() { +void Connection::disconnect(bool rollbackBeforeDisconnect) { PERF_TIMER("Connection::disconnect"); clearResultMetadata(); // Determine GIL state once, up front. disconnect() runs both from @@ -114,47 +127,75 @@ void Connection::disconnect() { // Py_IsInitialized() is checked first: after Py_Finalize() the interpreter is // gone and PyGILState_Check() is unreliable, so treat "not initialized" as // "no GIL" and skip all Python calls. (#671 follow-up) - bool hasGil = Py_IsInitialized() != 0 && PyGILState_Check() != 0; + bool hasGil = !isPythonFinalizing() && PyGILState_Check() != 0; if (_dbcHandle) { if (hasGil) { LOG("Disconnecting from database"); } - // 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 - // so they don't try to free the handles again during their destruction. - - // THREAD-SAFETY: Lock mutex to safely access _childStatementHandles - // This protects against concurrent allocStatementHandle() calls or GC finalizers + std::vector childHandles; size_t originalSize = 0, afterCompactSize = 0, badHandleCount = 0; - { - std::lock_guard lock(_childHandlesMutex); - - // First compact: remove expired weak_ptrs (they're already destroyed) - originalSize = _childStatementHandles.size(); - _childStatementHandles.erase( - std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(), - [](const std::weak_ptr& wp) { return wp.expired(); }), - _childStatementHandles.end()); - afterCompactSize = _childStatementHandles.size(); - - for (auto& weakHandle : _childStatementHandles) { - if (auto handle = weakHandle.lock()) { - // SAFETY ASSERTION: Only STMT handles should be in this vector - // This is guaranteed by allocStatementHandle() which only creates STMT handles - // If this assertion fails, it indicates a serious bug in handle tracking - if (handle->type() != SQL_HANDLE_STMT) { - ++badHandleCount; - continue; // Skip marking to prevent leak + auto disconnectNative = [&]() { + // Serialize explicit child free() calls as well as destruction. + // This lock must be released before reacquiring the GIL or logging. + std::lock_guard cleanupLock(_cleanupState->mutex); + { + std::lock_guard lock(_childHandlesMutex); + originalSize = _childStatementHandles.size(); + _childStatementHandles.erase( + std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(), + [](const std::weak_ptr& wp) { return wp.expired(); }), + _childStatementHandles.end()); + afterCompactSize = _childStatementHandles.size(); + childHandles.reserve(afterCompactSize); + for (auto& weakHandle : _childStatementHandles) { + if (auto handle = weakHandle.lock()) { + if (handle->type() != SQL_HANDLE_STMT) { + ++badHandleCount; + continue; + } + childHandles.push_back(std::move(handle)); } + } + } + if (rollbackBeforeDisconnect) { + // Explicit SQL transactions need manual mode for SQLEndTran. + // Never turn autocommit on here: that could commit abandoned work. + SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, + reinterpret_cast(SQL_AUTOCOMMIT_OFF), 0); + SQLEndTran_ptr(SQL_HANDLE_DBC, _dbcHandle->get(), SQL_ROLLBACK); + } + SQLRETURN result = SQLDisconnect_ptr(_dbcHandle->get()); + if (SQL_SUCCEEDED(result)) { + // Also cover children whose weak_ptr expired as their destructor + // began waiting for this gate: they cannot appear in the snapshot. + _cleanupState->disconnected = true; + std::lock_guard lock(_childHandlesMutex); + for (const auto& handle : childHandles) { handle->markImplicitlyFreed(); } + _childStatementHandles.clear(); + _allocationsSinceCompaction = 0; } - _childStatementHandles.clear(); - _allocationsSinceCompaction = 0; - } // Release lock before potentially slow SQLDisconnect call + return result; + }; + SQLRETURN ret; + if (hasGil) { + py::gil_scoped_release release; + ret = disconnectNative(); + } else { + ret = disconnectNative(); + } + if (!SQL_SUCCEEDED(ret)) { + if (hasGil) { + checkError(ret); + } else { + std::fputs("mssql-python: native disconnect failed\n", stderr); + } + // Keep ownership and child-handle tracking intact for a cleanup retry. + return; + } // Log after releasing _childHandlesMutex (#671): LOG()/LOG_ERROR() acquire // the GIL and must not run while a native mutex is held. Also gated on // hasGil so the GIL-less destructor / shutdown path never tries to log. @@ -168,26 +209,6 @@ void Connection::disconnect() { } } - 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()); - } - // 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)) { - // Intentionally no LOG() here: LOG() acquires the GIL internally - // via py::gil_scoped_acquire, which is unsafe during interpreter - // shutdown or stack unwinding (can deadlock or call std::terminate). - } // triggers SQLFreeHandle via destructor, if last owner _dbcHandle.reset(); } else if (hasGil) { @@ -195,6 +216,39 @@ void Connection::disconnect() { } } +void Connection::disconnectNoThrow() noexcept { + try { + if (isPythonFinalizing()) { + abandonDuringFinalization(); + return; + } + if (!_dbcHandle) { + return; + } + // disconnect() already supports GIL-less cleanup. Drop the GIL once so + // neither its diagnostics nor handle destruction can enter Python. + if (PyGILState_Check()) { + py::gil_scoped_release release; + disconnect(true); + } else { + disconnect(true); + } + } catch (...) { + std::fputs("mssql-python: unexpected failure during native connection cleanup\n", stderr); + } +} + +void Connection::abandonDuringFinalization() noexcept { + { + std::lock_guard lock(_childHandlesMutex); + _childStatementHandles.clear(); + _allocationsSinceCompaction = 0; + } + // SqlHandle::free() already suppresses SQLFreeHandle during finalization. + // Clearing the shared pointer leaves process teardown to the operating system. + _dbcHandle.reset(); +} + // TODO(microsoft): Add an exception class in C++ for error handling, // DB spec compliant void Connection::checkError(SQLRETURN ret) const { @@ -233,6 +287,7 @@ void Connection::clearResultMetadata() { void Connection::commit() { PERF_TIMER("Connection::commit"); + _poolClean = false; if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } @@ -250,6 +305,7 @@ void Connection::commit() { void Connection::rollback() { PERF_TIMER("Connection::rollback"); + _poolClean = false; if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } @@ -267,6 +323,9 @@ void Connection::rollback() { void Connection::setAutocommit(bool enable) { PERF_TIMER("Connection::setAutocommit"); + if (!enable) { + _poolClean = false; + } if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } @@ -283,7 +342,10 @@ void Connection::setAutocommit(bool enable) { ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, reinterpret_cast(static_cast(value)), 0); } - checkError(ret); + if (!SQL_SUCCEEDED(ret)) { + _poolClean = false; + checkError(ret); + } if (value == SQL_AUTOCOMMIT_ON) { LOG("Autocommit enabled"); } else { @@ -301,28 +363,43 @@ bool Connection::getAutocommit() const { SQLINTEGER string_length; SQLRETURN ret = SQLGetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, &value, sizeof(value), &string_length); - checkError(ret); + if (!SQL_SUCCEEDED(ret)) { + _poolClean = false; + checkError(ret); + } return value == SQL_AUTOCOMMIT_ON; } SqlHandlePtr Connection::allocStatementHandle() { PERF_TIMER("Connection::allocStatementHandle"); - if (!_dbcHandle) { - ThrowStdException("Connection handle not allocated"); - } - updateLastUsed(); + // Invalidate before allocation (including failures), not by parsing SQL. + // Every execution/catalog/fetch path, including direct native calls, needs + // a statement handle. Retained handles also prevent re-establishing proof. + _poolClean = false; LOG("Allocating statement handle"); - SQLHANDLE stmt = nullptr; - SQLRETURN ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &stmt); - checkError(ret); - auto stmtHandle = std::make_shared(static_cast(SQL_HANDLE_STMT), stmt); - - // THREAD-SAFETY: Lock mutex before modifying _childStatementHandles - // This protects against concurrent disconnect() or allocStatementHandle() calls, - // or GC finalizers running from different threads + // Keep the wrapper outside the lock scope: unwinding a failed registration + // frees the statement through the same cleanup gate. + SqlHandlePtr stmtHandle; bool compacted = false; size_t compactBefore = 0, compactAfter = 0; { + py::gil_scoped_release release; + std::lock_guard cleanupLock(_cleanupState->mutex); + if (_cleanupState->disconnected || !_dbcHandle) { + ThrowStdException("Connection handle not allocated"); + } + updateLastUsed(); + SQLHANDLE stmt = nullptr; + SQLRETURN ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &stmt); + if (!SQL_SUCCEEDED(ret)) { + // Snapshot diagnostics before disconnect can overwrite/free the DBC. + ErrorInfo err = SQLReadError(SQL_HANDLE_DBC, _dbcHandle->get(), ret); + ThrowStdException(err.sqlState.length() == 5 + ? "SQLSTATE:" + err.sqlState + ":" + err.ddbcErrorMsg + : err.ddbcErrorMsg); + } + stmtHandle = std::make_shared(static_cast(SQL_HANDLE_STMT), + stmt, _cleanupState); std::lock_guard lock(_childHandlesMutex); // Track this child handle so we can mark it as implicitly freed when connection closes @@ -356,6 +433,15 @@ SqlHandlePtr Connection::allocStatementHandle() { } SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) { + _poolClean = false; + // A scalar login timeout only bounds connection establishment; it cannot + // execute user work or retain a deferred buffer. Keep all other attributes + // conservative, and never clear an earlier permanent invalidation. + const bool scalarLoginTimeout = + attribute == SQL_ATTR_LOGIN_TIMEOUT && PyLong_CheckExact(value.ptr()); + if (!scalarLoginTimeout) { + _poolProofDisabled = true; + } clearResultMetadata(); LOG("Setting SQL attribute=%d", attribute); // SQLPOINTER ptr = nullptr; @@ -382,7 +468,21 @@ SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) { if (py::isinstance(value)) { // Get the integer value - int64_t longValue = value.cast(); + int64_t longValue; + try { + longValue = value.cast(); + } catch (const py::cast_error&) { + _poolProofDisabled = true; + throw; + } catch (const py::error_already_set&) { + _poolProofDisabled = true; + throw; + } + if (scalarLoginTimeout && + (longValue < 0 || + static_cast(longValue) > std::numeric_limits::max())) { + _poolProofDisabled = true; + } SQLRETURN ret; { @@ -395,6 +495,7 @@ SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) { } if (!SQL_SUCCEEDED(ret)) { + _poolProofDisabled = true; LOG("Failed to set integer attribute=%d, ret=%d", attribute, ret); } else { LOG("Set integer attribute=%d successfully", attribute); @@ -566,6 +667,7 @@ bool Connection::reset() { (SQLPOINTER)SQL_RESET_CONNECTION_YES, SQL_IS_INTEGER); } if (!SQL_SUCCEEDED(ret)) { + _poolClean = false; LOG("Failed to reset connection (ret=%d). Marking as dead.", ret); return false; } @@ -580,6 +682,7 @@ bool Connection::reset() { (SQLPOINTER)SQL_TXN_READ_COMMITTED, SQL_IS_INTEGER); } if (!SQL_SUCCEEDED(ret)) { + _poolClean = false; LOG("Failed to reset transaction isolation level (ret=%d). Marking as dead.", ret); return false; } @@ -588,6 +691,62 @@ bool Connection::reset() { return true; } +void Connection::prepareForPool() { + PERF_TIMER("Connection::prepareForPool"); + if (!_dbcHandle) { + ThrowStdException("Connection handle not allocated"); + } + if (_poolClean && !_poolProofDisabled) { + PERF_TIMER("Connection::prepareForPool::clean"); + return; + } + + _poolClean = false; + clearResultMetadata(); + SQLRETURN ret; + { + // One GIL release and metadata invalidation for the whole sequence. + // Do not trust Python mode/rollback hints: native callers and set_attr + // can change the state, and autocommit permits explicit BEGIN TRAN. + py::gil_scoped_release release; + PERF_TIMER("Connection::prepareForPool::sanitize"); + SQLINTEGER mode = SQL_AUTOCOMMIT_OFF; + SQLINTEGER length = 0; + { + PERF_TIMER("Connection::prepareForPool::get_autocommit"); + ret = SQLGetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, + &mode, sizeof(mode), &length); + } + if (SQL_SUCCEEDED(ret) && mode == SQL_AUTOCOMMIT_ON) { + PERF_TIMER("Connection::prepareForPool::autocommit_off"); + ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, + reinterpret_cast(SQL_AUTOCOMMIT_OFF), 0); + } + if (SQL_SUCCEEDED(ret)) { + PERF_TIMER("Connection::prepareForPool::rollback"); + ret = SQLEndTran_ptr(SQL_HANDLE_DBC, _dbcHandle->get(), SQL_ROLLBACK); + } + // Never enable autocommit after a failed rollback: it could commit + // abandoned work. Manual mode can leave even an empty transaction open. + if (SQL_SUCCEEDED(ret)) { + PERF_TIMER("Connection::prepareForPool::autocommit_on"); + ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, + reinterpret_cast(SQL_AUTOCOMMIT_ON), 0); + } + } + checkError(ret); + _autocommit = true; + updateLastUsed(); + // A native statement alias can execute again without another allocation, + // even in a later lease. Only expired wrappers permit the fast path. + if (!_poolProofDisabled) { + std::lock_guard lock(_childHandlesMutex); + _poolClean = std::all_of( + _childStatementHandles.begin(), _childStatementHandles.end(), + [](const std::weak_ptr& handle) { return handle.expired(); }); + } +} + void Connection::updateLastUsed() { _lastUsed = std::chrono::steady_clock::now(); } @@ -655,7 +814,8 @@ ConnectionHandle::ConnectionHandle(const std::u16string& connStr, bool usePool, PERF_TIMER("ConnectionHandle::ConnectionHandle"); if (_usePool) { _conn = ConnectionPoolManager::getInstance().acquireConnection(_connStr, attrsBefore, - _poolKey, tokenFactory); + _poolKey, tokenFactory, + &_originPool); // acquireConnection returns nullptr when pooling was disabled out from // under us (a disable_pooling() won the race). Fall back to a non-pooled // connection and flip _usePool so close() disconnects it directly rather @@ -683,19 +843,54 @@ ConnectionHandle::ConnectionHandle(const std::u16string& connStr, bool usePool, ConnectionHandle::~ConnectionHandle() { if (_conn) { - close(); + if (isPythonFinalizing()) { + _conn->abandonDuringFinalization(); + _conn = nullptr; + return; + } + try { + // Discard ends abandoned work without returning this connection to + // the pool or entering Python from a native destructor. + ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn); + } catch (...) { + std::fputs("mssql-python: failed to release native connection pool capacity\n", stderr); + _conn->disconnectNoThrow(); + } } } -void ConnectionHandle::close() { +void ConnectionHandle::close(bool rollbackBeforeDisconnect) { PERF_TIMER("ConnectionHandle::close"); if (!_conn) { ThrowStdException("Connection object is not initialized"); } + try { + if (_usePool) { + _conn->prepareForPool(); + } else { + // Preserve unpooled close semantics without doing pool sanitation. + if (rollbackBeforeDisconnect && !_conn->getAutocommit()) { + _conn->rollback(); + } + _conn->disconnect(); + } + } catch (...) { + // A low-level unpooled close still allows the caller to recover from + // SQLDisconnect failure using the existing connection and children. + if (!_usePool && !rollbackBeforeDisconnect) { + throw; + } + // Never retain a connection whose transaction state could not be + // sanitized. Release capacity and preserve the original cleanup error. + try { + ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn); + } catch (...) { + } + _conn = nullptr; + throw; + } if (_usePool) { - ConnectionPoolManager::getInstance().returnConnection(_poolKey, _conn); - } else { - _conn->disconnect(); + ConnectionPoolManager::getInstance().returnConnection(_poolKey, _originPool, _conn); } _conn = nullptr; } @@ -733,13 +928,20 @@ bool ConnectionHandle::getAutocommit() const { SqlHandlePtr ConnectionHandle::allocStatementHandle() { PERF_TIMER("ConnectionHandle::allocStatementHandle"); - if (!_conn) { + // close() can detach _conn while allocation waits without the GIL. + auto conn = _conn; + if (!conn) { ThrowStdException("Connection object is not initialized"); } - return _conn->allocStatementHandle(); + return conn->allocStatementHandle(); } py::object Connection::getInfo(SQLUSMALLINT infoType) const { + _poolClean = false; + if (infoType == SQL_DRIVER_HDBC || infoType == SQL_DRIVER_HENV || + infoType == SQL_DRIVER_HSTMT || infoType == SQL_DRIVER_HLIB) { + _poolProofDisabled = true; + } if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } diff --git a/mssql_python/pybind/connection/connection.h b/mssql_python/pybind/connection/connection.h index c9df35206..a8aca3765 100644 --- a/mssql_python/pybind/connection/connection.h +++ b/mssql_python/pybind/connection/connection.h @@ -32,13 +32,19 @@ class Connection { public: Connection(const std::u16string& connStr, bool fromPool); - ~Connection(); + ~Connection() noexcept; // Establish the connection using the stored connection string. void connect(const py::dict& attrs_before = py::dict()); // Disconnect and free the connection handle. - void disconnect(); + void disconnect(bool rollbackBeforeDisconnect = false); + + // Roll back and disconnect without Python callbacks or escaping exceptions. + void disconnectNoThrow() noexcept; + + // Relinquish native handles without ODBC calls during interpreter finalization. + void abandonDuringFinalization() noexcept; // Commit the current transaction. void commit(); @@ -53,6 +59,7 @@ class Connection { bool getAutocommit() const; bool isAlive() const; bool reset(); + void prepareForPool(); void updateLastUsed(); std::chrono::steady_clock::time_point lastUsed() const; @@ -101,6 +108,11 @@ class Connection { std::u16string _connStr; bool _fromPool = false; bool _autocommit = true; + // Only successful rollback + AUTOCOMMIT_ON establishes this proof. A new + // login or deferred SQL_ATTR_RESET_CONNECTION alone does not establish it. + mutable bool _poolClean = false; + // Escaped raw handles and arbitrary/deferred attributes cannot be tracked. + mutable bool _poolProofDisabled = false; SqlHandlePtr _dbcHandle; std::chrono::steady_clock::time_point _lastUsed; // POSIX-epoch expiry (seconds) of the access token this connection last @@ -130,8 +142,13 @@ class Connection { // Prevents data races between allocStatementHandle() and disconnect(), // or concurrent GC finalizers running from different threads mutable std::mutex _childHandlesMutex; + // Child wrappers retain this gate even after the Connection is destroyed. + const std::shared_ptr _cleanupState = + std::make_shared(); }; +class ConnectionPool; + class ConnectionHandle { public: ConnectionHandle(const std::u16string& connStr, bool usePool, @@ -140,7 +157,7 @@ class ConnectionHandle { const py::object& tokenFactory = py::object()); ~ConnectionHandle(); - void close(); + void close(bool rollbackBeforeDisconnect = false); void commit(); void rollback(); void setAutocommit(bool enabled); @@ -160,4 +177,7 @@ class ConnectionHandle { // Entra access-token auth so distinct identities never share a pool. // Empty is never stored; the ctor falls back to _connStr. std::u16string _poolKey; + // Identifies the exact pool generation that issued _conn. A weak reference + // prevents a checked-out connection from keeping a disabled pool alive. + std::weak_ptr _originPool; }; diff --git a/mssql_python/pybind/connection/connection_pool.cpp b/mssql_python/pybind/connection/connection_pool.cpp index 831a01db2..3ea3ccc5f 100644 --- a/mssql_python/pybind/connection/connection_pool.cpp +++ b/mssql_python/pybind/connection/connection_pool.cpp @@ -329,6 +329,15 @@ void ConnectionPool::release(std::shared_ptr conn) { } } +void ConnectionPool::discard(std::shared_ptr conn) { + { + std::lock_guard lock(_mutex); + if (_current_size > 0) + --_current_size; + } + conn->disconnectNoThrow(); +} + bool ConnectionPool::canEvict() { std::lock_guard lock(_mutex); // Never evict while any connection is checked out or in-flight. Reserved @@ -388,7 +397,9 @@ ConnectionPoolManager& ConnectionPoolManager::getInstance() { std::shared_ptr ConnectionPoolManager::acquireConnection(const std::u16string& connStr, const py::dict& attrs_before, const std::u16string& pool_key, - const py::object& token_factory) { + const py::object& token_factory, + std::weak_ptr* + originating_pool) { PERF_TIMER("ConnectionPoolManager::acquireConnection"); // Key the pool by pool_key when provided (identity-aware), // else fall back to the connection string (legacy behavior). @@ -450,6 +461,9 @@ std::shared_ptr ConnectionPoolManager::acquireConnection(const std:: created = true; } pool = pool_ref; + if (originating_pool) { + *originating_pool = pool; + } } // Log after releasing _manager_mutex (#671): LOG() acquires the GIL, and // holding a native mutex across a GIL acquisition deadlocks a thread that @@ -474,18 +488,18 @@ std::shared_ptr ConnectionPoolManager::acquireConnection(const std:: return pool->acquire(connStr, attrs_before, token_factory); } -void ConnectionPoolManager::returnConnection(const std::u16string& pool_key, - const std::shared_ptr conn) { - std::shared_ptr pool; +void ConnectionPoolManager::returnConnection( + const std::u16string& pool_key, const std::weak_ptr& originating_pool, + const std::shared_ptr conn) { + std::shared_ptr pool = originating_pool.lock(); + bool registered = false; { std::lock_guard lock(_manager_mutex); auto it = _pools.find(pool_key); - if (it != _pools.end()) { - pool = it->second; - } + registered = pool && it != _pools.end() && it->second == pool; } // Call release() outside _manager_mutex to avoid deadlock. - if (pool) { + if (registered) { pool->release(conn); } else { // No pool is registered under this key (e.g. the pool was lazily @@ -506,6 +520,20 @@ void ConnectionPoolManager::returnConnection(const std::u16string& pool_key, } } +void ConnectionPoolManager::discardConnection( + const std::weak_ptr& originating_pool, + const std::shared_ptr conn) { + if (!conn) { + return; + } + std::shared_ptr pool = originating_pool.lock(); + if (pool) { + pool->discard(conn); + } else { + conn->disconnectNoThrow(); + } +} + void ConnectionPoolManager::configure(int max_size, int idle_timeout_secs) { std::lock_guard lock(_manager_mutex); _default_max_size = max_size; diff --git a/mssql_python/pybind/connection/connection_pool.h b/mssql_python/pybind/connection/connection_pool.h index edc87c865..6f67eab56 100644 --- a/mssql_python/pybind/connection/connection_pool.h +++ b/mssql_python/pybind/connection/connection_pool.h @@ -34,6 +34,9 @@ class ConnectionPool { // Returns a connection to the pool for reuse void release(std::shared_ptr conn); + // Permanently removes a checked-out connection and releases its capacity. + void discard(std::shared_ptr conn); + // Closes all connections in the pool, releasing resources void close(); @@ -71,7 +74,8 @@ class ConnectionPoolManager { std::shared_ptr acquireConnection( const std::u16string& conn_str, const py::dict& attrs_before = py::dict(), const std::u16string& pool_key = std::u16string(), - const py::object& token_factory = py::object()); + const py::object& token_factory = py::object(), + std::weak_ptr* originating_pool = nullptr); // Arms (true) or disarms (false) new-pool creation. Disarming, done under // _manager_mutex, guarantees that any acquireConnection() serialized after @@ -80,7 +84,13 @@ class ConnectionPoolManager { // Returns a connection to its original pool, identified by pool_key // (the same key passed to acquireConnection). - void returnConnection(const std::u16string& pool_key, std::shared_ptr conn); + void returnConnection(const std::u16string& pool_key, + const std::weak_ptr& originating_pool, + std::shared_ptr conn); + + // Discards a connection from the exact pool generation that issued it. + void discardConnection(const std::weak_ptr& originating_pool, + std::shared_ptr conn); // Closes all pools and their connections void closePools(); diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 36d06122e..4a8184bb9 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -1588,7 +1588,9 @@ void DriverLoader::loadDriver() { } // SqlHandle definition -SqlHandle::SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle) : _type(type), _handle(rawHandle) {} +SqlHandle::SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle, + std::shared_ptr cleanupState) + : _type(type), _handle(rawHandle), _cleanupState(std::move(cleanupState)) {} SqlHandle::~SqlHandle() { if (_handle) { @@ -1596,6 +1598,13 @@ SqlHandle::~SqlHandle() { } } +std::unique_lock SqlHandle::lockForCleanup() const { + if (_cleanupState) { + return std::unique_lock(_cleanupState->mutex); + } + return {}; +} + SQLHANDLE SqlHandle::get() const { return _handle; } @@ -1629,84 +1638,73 @@ void SqlHandle::markImplicitlyFreed() { * If you need destruction logs, use explicit close() methods instead. */ void SqlHandle::free() { + freeHandle(); +} + +SQLRETURN SqlHandle::freeHandle() { PERF_TIMER("SqlHandle::free"); resultMetadata.clear(); - if (_handle && SQLFreeHandle_ptr) { - // GH-610: Clear describe cache to prevent memory leak. - describeCache.clear(); - - // Check if Python is shutting down using centralized helper function - bool pythonShuttingDown = is_python_finalizing(); - - // RESOURCE LEAK MITIGATION: - // When handles are skipped during shutdown, they are not freed, which could - // cause resource leaks. However, this is mitigated by: - // 1. Python-side atexit cleanup (in __init__.py) that explicitly closes all - // connections before shutdown, ensuring handles are freed in correct order - // 2. OS-level cleanup at process termination recovers any remaining resources - // 3. This tradeoff prioritizes crash prevention over resource cleanup, which - // is appropriate since we're already in shutdown sequence - bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC; + bool pythonShuttingDown = is_python_finalizing(); + bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC; #ifdef _WIN32 - // The static ENV is destroyed during DLL_PROCESS_DETACH, after Python - // finalization. Calling ODBC then can access already-torn-down SSPI state. - skipDuringShutdown = skipDuringShutdown || _type == SQL_HANDLE_ENV; + // The static ENV is destroyed during DLL_PROCESS_DETACH, after Python + // finalization. Calling ODBC then can access already-torn-down SSPI state. + skipDuringShutdown = skipDuringShutdown || _type == SQL_HANDLE_ENV; #endif - if (pythonShuttingDown && skipDuringShutdown) { - _handle = nullptr; // Mark as freed to prevent double-free attempts - return; - } + if (pythonShuttingDown && skipDuringShutdown) { + // Do not wait for another thread's ODBC cleanup during finalization. + // Process teardown owns any resources not released by atexit cleanup. + _handle = nullptr; + return SQL_SUCCESS; + } - // CRITICAL FIX: Check if handle was already implicitly freed by parent handle - // When Connection::disconnect() frees the DBC handle, the ODBC driver automatically - // frees all child STMT handles. We track this state to avoid double-free attempts. - // This approach avoids calling ODBC functions on potentially-freed handles, which - // would cause use-after-free errors. - if (_implicitly_freed) { - _handle = nullptr; // Just clear the pointer, don't call ODBC functions - return; + auto freeNative = [this]() -> SQLRETURN { + auto cleanupLock = lockForCleanup(); + if (!_handle || !SQLFreeHandle_ptr) { + return SQL_INVALID_HANDLE; } - - // Handle is valid and not implicitly freed, proceed with normal freeing. - // Release the GIL during the blocking ODBC call (SQLFreeHandle on a STMT - // with an open server-side cursor, or on a DBC, performs network I/O). - // This is critical when the connection is reached through an in-process - // Python TCP forwarder (e.g. paramiko + sshtunnel) - the forwarder - // thread needs the GIL to push bytes, so holding it here deadlocks - // (issue #565). Only release the GIL if it is actually held AND the - // interpreter is not finalizing - gil_scoped_release is unsafe during - // shutdown even if PyGILState_Check() reports the GIL as held. - if (!pythonShuttingDown && PyGILState_Check()) { - py::gil_scoped_release release; - SQLFreeHandle_ptr(_type, _handle); - } else { - SQLFreeHandle_ptr(_type, _handle); + describeCache.clear(); + if (_implicitly_freed || (_cleanupState && _cleanupState->disconnected)) { + _handle = nullptr; + return SQL_SUCCESS; } - _handle = nullptr; + SQLRETURN ret = SQLFreeHandle_ptr(_type, _handle); + if (SQL_SUCCEEDED(ret)) { + _handle = nullptr; + } + return ret; + }; + // The same gate is held through SQLDisconnect and child invalidation. + // Release the GIL before waiting, and unlock before reacquiring it. + if (!pythonShuttingDown && PyGILState_Check()) { + py::gil_scoped_release release; + return freeNative(); } + return freeNative(); } void SqlHandle::close_cursor() { resultMetadata.clear(); - if (_type != SQL_HANDLE_STMT || !_handle) { - return; - } - if (_implicitly_freed) { + if (is_python_finalizing()) { return; } - if (!SQLFreeStmt_ptr) { - ThrowStdException("SQLFreeStmt function not loaded"); - } - // Release the GIL during the blocking SQLFreeStmt(SQL_CLOSE) network - // round-trip; see issue #565 (in-process forwarder deadlock). - // Skip GIL release when the GIL isn't held or the interpreter is - // finalizing - gil_scoped_release is unsafe in shutdown. + auto closeNative = [this]() -> SQLRETURN { + auto cleanupLock = lockForCleanup(); + if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed || + (_cleanupState && _cleanupState->disconnected)) { + return SQL_SUCCESS; + } + if (!SQLFreeStmt_ptr) { + ThrowStdException("SQLFreeStmt function not loaded"); + } + return SQLFreeStmt_ptr(_handle, SQL_CLOSE); + }; SQLRETURN ret; - if (!is_python_finalizing() && PyGILState_Check()) { + if (PyGILState_Check()) { py::gil_scoped_release release; - ret = SQLFreeStmt_ptr(_handle, SQL_CLOSE); + ret = closeNative(); } else { - ret = SQLFreeStmt_ptr(_handle, SQL_CLOSE); + ret = closeNative(); } if (ret != SQL_SUCCESS && ret != SQL_SUCCESS_WITH_INFO) { ThrowStdException("SQLFreeStmt(SQL_CLOSE) failed"); @@ -1715,44 +1713,25 @@ void SqlHandle::close_cursor() { void SqlHandle::cancel() { resultMetadata.clear(); - // SQLCancel is intentionally lenient: it is a no-op on non-STMT handles, - // already-freed handles, or if the driver does not expose it. This lets - // _ArrowReader.close() call it unconditionally without coordinating with - // the fetch thread. The GIL is released so a blocked fetch thread can - // observe the cancel and return. - // - // Cross-thread invariant (why no mutex is needed): - // The only cross-thread pattern this driver blesses is exactly the one - // ODBC blesses: cancel() may be called from a thread *other than* the - // fetch thread to unblock an in-flight SQLFetch/SQLExecute on the same - // HSTMT. Per the ODBC spec, SQLCancel (with the SQLGetDiagRec/Field - // family) is the only entry point safe to call across threads on the - // same statement handle. All other operations on a Cursor/SqlHandle - // are single-owner: per DB API 2.0 and the Cursor thread-safety note - // in cursor.py, callers must not share a Cursor for its lifecycle - // operations (execute/fetch/close/free) across threads. Under that - // contract, free() / close_cursor() / SQLFreeHandle can never be in - // flight on this handle concurrently with cancel(), so the read of - // _handle above and the SQLCancel_ptr(h) call below cannot race a - // free() that clears _handle. - // - // A std::mutex here would only close the cancel()-vs-free() window; - // it would NOT close the (equally real) free()-vs-fetch window - // without also locking every fetch — which would serialize network - // I/O and defeat the whole point of cross-thread cancel. The right - // place to defend against a misuse (Cursor shared across threads for - // close vs. reader-cancel) is at the Python Cursor layer, not here. - if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed) { - return; - } - if (!SQLCancel_ptr) { + if (is_python_finalizing()) { return; } - SQLHANDLE h = _handle; + // Fetch/execute do not take this cleanup gate, so cross-thread cancellation + // can still interrupt them. Reader finalizers must not cancel a freed handle. + auto cancelNative = [this]() -> SQLRETURN { + auto cleanupLock = lockForCleanup(); + if (_type != SQL_HANDLE_STMT || !_handle || _implicitly_freed || !SQLCancel_ptr || + (_cleanupState && _cleanupState->disconnected)) { + return SQL_SUCCESS; + } + return SQLCancel_ptr(_handle); + }; SQLRETURN ret; - { + if (PyGILState_Check()) { py::gil_scoped_release release; - ret = SQLCancel_ptr(h); + ret = cancelNative(); + } else { + ret = cancelNative(); } // SQLCancel may return SQL_SUCCESS_WITH_INFO when there was nothing to // cancel; that is fine. We only throw on hard failure. @@ -1934,19 +1913,22 @@ SQLRETURN SQLColumns_wrap(SqlHandlePtr StatementHandle, const py::object& catalo ErrorInfo SQLCheckError_Wrap(SQLSMALLINT handleType, SqlHandlePtr handle, SQLRETURN retcode) { PERF_TIMER("SQLCheckError_Wrap"); LOG("SQLCheckError: Checking ODBC errors - handleType=%d, retcode=%d", handleType, retcode); + if (retcode != SQL_INVALID_HANDLE && !SQL_SUCCEEDED(retcode) && !SQLGetDiagRec_ptr) { + LOG("SQLCheckError: SQLGetDiagRec function pointer not initialized, loading driver"); + DriverLoader::getInstance().loadDriver(); + } + return SQLReadError(handleType, handle ? handle->get() : nullptr, retcode); +} + +ErrorInfo SQLReadError(SQLSMALLINT handleType, SQLHANDLE rawHandle, SQLRETURN retcode) { ErrorInfo errorInfo; - if (retcode == SQL_INVALID_HANDLE) { - LOG("SQLCheckError: SQL_INVALID_HANDLE detected - handle is invalid"); + if (retcode == SQL_INVALID_HANDLE || !rawHandle) { errorInfo.ddbcErrorMsg = "Invalid handle!"; return errorInfo; } - assert(handle != 0); - SQLHANDLE rawHandle = handle->get(); if (!SQL_SUCCEEDED(retcode)) { if (!SQLGetDiagRec_ptr) { - LOG("SQLCheckError: SQLGetDiagRec function pointer not " - "initialized, loading driver"); - DriverLoader::getInstance().loadDriver(); // Load the driver + ThrowStdException("SQLGetDiagRec function pointer not initialized"); } SQLWCHAR sqlState[6], message[SQL_MAX_MESSAGE_LENGTH_SQLSERVER]; @@ -6360,25 +6342,16 @@ SQLRETURN SQLFreeHandle_wrap(SQLSMALLINT HandleType, SqlHandlePtr Handle) { LOG("SQLFreeHandle_wrap: Free SQL handle type=%d", HandleType); // Guard against a null/None handle being passed from Python - dereferencing // Handle->get() on a null shared_ptr would segfault. - if (!Handle || !Handle->get()) { + if (!Handle || HandleType != Handle->type()) { return SQL_INVALID_HANDLE; } - if (!SQLAllocHandle_ptr) { + if (!SQLFreeHandle_ptr) { LOG("SQLFreeHandle_wrap: Function pointer not initialized. Loading the " "driver."); DriverLoader::getInstance().loadDriver(); // Load the driver } - // Release the GIL during the blocking SQLFreeHandle network round-trip - // (see issue #565 - in-process Python TCP forwarder deadlock). - // Skip GIL release in shutdown paths where it would crash. - SQLRETURN ret; - if (!is_python_finalizing() && PyGILState_Check()) { - py::gil_scoped_release release; - ret = SQLFreeHandle_ptr(HandleType, Handle->get()); - } else { - ret = SQLFreeHandle_ptr(HandleType, Handle->get()); - } + SQLRETURN ret = Handle->freeHandle(); if (!SQL_SUCCEEDED(ret)) { LOG("SQLFreeHandle_wrap: SQLFreeHandle failed with error code - %d", ret); return ret; @@ -6498,7 +6471,8 @@ PYBIND11_MODULE(ddbc_bindings, m) { const py::object&>(), py::arg("conn_str"), py::arg("use_pool"), py::arg("attrs_before") = py::dict(), py::arg("pool_key") = std::u16string(), py::arg("token_factory") = py::none()) - .def("close", &ConnectionHandle::close, "Close the connection") + .def("close", &ConnectionHandle::close, + py::arg("rollback_before_disconnect") = false, "Close the connection") .def("commit", &ConnectionHandle::commit, "Commit the current transaction") .def("rollback", &ConnectionHandle::rollback, "Rollback the current transaction") .def("set_autocommit", &ConnectionHandle::setAutocommit) diff --git a/mssql_python/pybind/ddbc_bindings.h b/mssql_python/pybind/ddbc_bindings.h index ebec0c79d..32d9f8067 100644 --- a/mssql_python/pybind/ddbc_bindings.h +++ b/mssql_python/pybind/ddbc_bindings.h @@ -285,13 +285,20 @@ struct DescribedParamInfo { SQLSMALLINT decimalDigits; }; +struct ConnectionCleanupState { + std::mutex mutex; + bool disconnected = false; // Protected by mutex, shared with every child. +}; + class SqlHandle { public: - SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle); + SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle, + std::shared_ptr cleanupState = nullptr); ~SqlHandle(); SQLHANDLE get() const; SQLSMALLINT type() const; void free(); + SQLRETURN freeHandle(); void close_cursor(); // Cancel an in-progress statement (SQLCancel). Safe to call from a // thread other than the one running the fetch — this is the *only* @@ -326,9 +333,12 @@ class SqlHandle { ResultMetadataCache resultMetadata; private: + // The caller must release the GIL before waiting for native cleanup. + std::unique_lock lockForCleanup() const; SQLSMALLINT _type; SQLHANDLE _handle; bool _implicitly_freed = false; // Tracks if handle was freed by parent + std::shared_ptr _cleanupState; }; using SqlHandlePtr = std::shared_ptr; @@ -339,6 +349,8 @@ struct ErrorInfo { std::string ddbcErrorMsg; }; ErrorInfo SQLCheckError_Wrap(SQLSMALLINT handleType, SqlHandlePtr handle, SQLRETURN retcode); +// Driver must be initialized; reads diagnostics without Python logging/callbacks. +ErrorInfo SQLReadError(SQLSMALLINT handleType, SQLHANDLE handle, SQLRETURN retcode); // Thread-safe decimal separator accessor class class ThreadSafeDecimalSeparator { diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp new file mode 100644 index 000000000..13c521070 --- /dev/null +++ b/tests/native/pool_sanitation.cpp @@ -0,0 +1,614 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "connection/connection.h" +#include "connection/connection_pool.h" +#include "logger_bridge.hpp" +#include +#include +#include +#include +#include +#include + +namespace { +struct Handle { + SQLSMALLINT type; + SQLHANDLE parent; + bool autocommit = true; + bool transaction = false; + bool pendingWork = false; + SQLULEN loginTimeout = 0; +}; + +std::unordered_map> handles; +std::vector calls; +std::string failNext; +int commits = 0; +int logins = 0; +std::vector loginTimeouts; + +void require(bool condition, const std::string& message) { + if (!condition) { + throw std::runtime_error(message); + } +} + +SQLRETURN record(const std::string& operation) { + calls.push_back(operation); + if (operation == failNext) { + failNext.clear(); + return SQL_ERROR; + } + return SQL_SUCCESS; +} + +SQLRETURN SQL_API allocate(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* out) { + auto ret = record(type == SQL_HANDLE_STMT ? "allocate_statement" : "allocate"); + if (!SQL_SUCCEEDED(ret)) { + return ret; + } + auto handle = std::make_unique(Handle{type, parent}); + *out = handle.get(); + handles.emplace(*out, std::move(handle)); + return SQL_SUCCESS; +} + +SQLRETURN SQL_API freeHandle(SQLSMALLINT, SQLHANDLE handle) { + auto ret = record("free"); + if (SQL_SUCCEEDED(ret)) { + handles.erase(handle); + } + return ret; +} + +SQLRETURN SQL_API setEnv(SQLHENV, SQLINTEGER, SQLPOINTER, SQLINTEGER) { + return SQL_SUCCESS; +} + +SQLRETURN SQL_API login(SQLHDBC dbc, SQLHWND, SQLWCHAR*, SQLSMALLINT, + SQLWCHAR*, SQLSMALLINT, SQLSMALLINT*, SQLUSMALLINT) { + ++logins; + loginTimeouts.push_back(handles.at(dbc)->loginTimeout); + return record("login"); +} + +SQLRETURN SQL_API setAttr(SQLHDBC dbc, SQLINTEGER attribute, SQLPOINTER value, SQLINTEGER length) { + std::string operation = "attribute"; + if (attribute == SQL_ATTR_AUTOCOMMIT) { + operation = value ? "on" : "off"; + } else if (attribute == SQL_ATTR_RESET_CONNECTION) { + operation = "reset"; + } else if (attribute == SQL_ATTR_TXN_ISOLATION) { + operation = "isolation"; + } + auto ret = record(operation); + if (!SQL_SUCCEEDED(ret)) { + return ret; + } + auto& handle = *handles.at(dbc); + if (attribute == SQL_ATTR_AUTOCOMMIT) { + if (value) { + if (handle.pendingWork) { + ++commits; + } + handle.transaction = handle.pendingWork = false; + } + handle.autocommit = value != nullptr; + } else if (attribute == SQL_ATTR_LOGIN_TIMEOUT && length == SQL_IS_INTEGER) { + handle.loginTimeout = reinterpret_cast(value); + } + // A reset is deferred until execution. It must not establish clean proof. + return SQL_SUCCESS; +} + +SQLRETURN SQL_API getAttr(SQLHDBC dbc, SQLINTEGER attribute, SQLPOINTER value, + SQLINTEGER, SQLINTEGER*) { + auto ret = record(attribute == SQL_ATTR_AUTOCOMMIT ? "get" : "alive"); + if (SQL_SUCCEEDED(ret)) { + *static_cast(value) = attribute == SQL_ATTR_AUTOCOMMIT + ? (handles.at(dbc)->autocommit ? SQL_AUTOCOMMIT_ON : SQL_AUTOCOMMIT_OFF) + : SQL_CD_FALSE; + } + return ret; +} + +SQLRETURN SQL_API endTran(SQLSMALLINT, SQLHANDLE dbc, SQLSMALLINT completion) { + auto ret = record(completion == SQL_ROLLBACK ? "rollback" : "commit"); + if (SQL_SUCCEEDED(ret) && !handles.at(dbc)->autocommit) { + auto& handle = *handles.at(dbc); + if (completion == SQL_COMMIT && handle.pendingWork) { + ++commits; + } + handle.pendingWork = false; + // Model the empty manual-mode transaction that motivated PR #777. + handle.transaction = true; + } + return ret; +} + +SQLRETURN SQL_API disconnect(SQLHDBC dbc) { + record("disconnect"); + if (handles.at(dbc)->pendingWork) { + return SQL_ERROR; + } + handles.at(dbc)->transaction = handles.at(dbc)->pendingWork = false; + for (auto it = handles.begin(); it != handles.end();) { + if (it->second->parent == dbc) { + it = handles.erase(it); + } else { + ++it; + } + } + return SQL_SUCCESS; +} + +SQLRETURN SQL_API getInfo(SQLHDBC dbc, SQLUSMALLINT, SQLPOINTER value, + SQLSMALLINT, SQLSMALLINT* length) { + *length = sizeof(dbc); + if (value) { + std::memcpy(value, &dbc, sizeof(dbc)); + } + return SQL_SUCCESS; +} + +SQLRETURN SQL_API diagnostic(SQLSMALLINT, SQLHANDLE, SQLSMALLINT recordNumber, + SQLWCHAR* state, SQLINTEGER* native, SQLWCHAR* message, + SQLSMALLINT, SQLSMALLINT* length) { + if (recordNumber != 1) { + return SQL_NO_DATA; + } + const SQLWCHAR sqlstate[] = {'H', 'Y', '0', '0', '0', 0}; + const SQLWCHAR text[] = {'f', 'a', 'i', 'l', 'e', 'd', 0}; + std::copy(std::begin(sqlstate), std::end(sqlstate), state); + std::copy(std::begin(text), std::end(text), message); + *native = 0; + *length = 6; + return SQL_SUCCESS; +} + +void expectCalls(std::initializer_list expected) { + std::vector wanted(expected.begin(), expected.end()); + if (calls != wanted) { + std::string actual; + for (const auto& call : calls) { + actual += call + " "; + } + throw std::runtime_error("Unexpected ODBC calls: " + actual); + } +} + +void expectFailure(const std::function& action) { + try { + action(); + } catch (const std::runtime_error&) { + return; + } + throw std::runtime_error("Expected native failure"); +} + +std::unique_ptr acquire(bool pooled = true, + const py::dict& attrs = py::dict()) { + return std::make_unique(u"native pool test", pooled, attrs); +} + +void checkParked() { + require(commits == 0, "Cleanup committed pending work"); + for (const auto& item : handles) { + if (item.second->type == SQL_HANDLE_DBC) { + require(item.second->autocommit && !item.second->transaction, + "Pooled DBC retained a transaction or manual mode"); + } + } +} + +void warm() { + auto connection = acquire(); + connection->close(); + checkParked(); +} + +void startWork(const SqlHandlePtr& statement) { + auto dbc = handles.at(statement->get())->parent; + handles.at(dbc)->transaction = handles.at(dbc)->pendingWork = true; +} + +void resetScenario() { + failNext.clear(); + auto& manager = ConnectionPoolManager::getInstance(); + manager.closePools(); + manager.configure(1, 30); + manager.setAccepting(true); + calls.clear(); + commits = logins = 0; + loginTimeouts.clear(); +} +} // namespace + +int main() { + py::scoped_interpreter interpreter; + mssql_python::logging::LoggerBridge::updateLevel(1000); + SQLAllocHandle_ptr = allocate; + SQLFreeHandle_ptr = freeHandle; + SQLSetEnvAttr_ptr = setEnv; + SQLDriverConnect_ptr = login; + SQLSetConnectAttr_ptr = setAttr; + SQLGetConnectAttr_ptr = getAttr; + SQLEndTran_ptr = endTran; + SQLDisconnect_ptr = disconnect; + SQLGetInfo_ptr = getInfo; + SQLGetDiagRec_ptr = diagnostic; + + int passed = 0; + auto run = [&](const char* name, const std::function& test) { + resetScenario(); + test(); + ++passed; + std::cout << "PASS " << name << '\n'; + }; + try { + run("new login is not clean proof; repeated empty leases skip all sanitation", [] { + auto connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + for (int i = 0; i < 100; ++i) { + connection = acquire(); + connection->setAutocommit(true); + calls.clear(); + connection->close(true); + expectCalls({}); + checkParked(); + } + require(logins == 1, "Empty leases did not reuse the physical connection"); + }); + run("timeout=30 is applied before login and preserves repeated empty-lease fast path", [] { + py::dict attrs; + attrs[py::int_(SQL_ATTR_LOGIN_TIMEOUT)] = py::int_(30); + auto connection = acquire(true, attrs); + connection->setAutocommit(true); + require(loginTimeouts == std::vector{30}, + "Login timeout was not applied as 30 before SQLDriverConnect"); + require(std::count(calls.begin(), calls.end(), "attribute") == 1, + "Login timeout was not applied exactly once"); + calls.clear(); + connection->close(true); + expectCalls({"get", "off", "rollback", "on"}); + for (int i = 0; i < 100; ++i) { + connection = acquire(true, attrs); + connection->setAutocommit(true); + calls.clear(); + connection->close(true); + expectCalls({}); + checkParked(); + } + require(logins == 1 && loginTimeouts == std::vector{30}, + "Timed leases did not reuse the physical connection with timeout 30"); + }); + run("valid scalar timeout cannot make dirty work clean", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + startWork(statement); + statement.reset(); + connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, py::int_(30)); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + checkParked(); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({}); + }); + for (bool timeoutFirst : {true, false}) { + run("scalar timeout never clears an unknown attribute's permanent invalidation", [timeoutFirst] { + py::dict attrs; + auto setTimeout = [&] { + attrs[py::int_(SQL_ATTR_LOGIN_TIMEOUT)] = py::int_(30); + }; + if (timeoutFirst) { + setTimeout(); + } + attrs[py::int_(SQL_ATTR_AUTOCOMMIT)] = py::int_(SQL_AUTOCOMMIT_ON); + if (!timeoutFirst) { + setTimeout(); + } + auto connection = acquire(true, attrs); + require(loginTimeouts == std::vector{30}, + "Timeout was lost when combined with another attribute"); + connection->close(); + connection = acquire(); + connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, py::int_(30)); + connection->close(); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + for (const py::object& value : std::vector{ + py::str("30"), py::bytes("30"), py::bool_(true), + py::int_(-1), py::int_(uint64_t{1} << 32)}) { + run("nonscalar and out-of-range login timeouts permanently disable proof", [&value] { + warm(); + auto connection = acquire(); + connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, value); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + for (const py::object& value : std::vector{ + py::float_(30.0), py::eval("1 << 100")}) { + run("unsupported or overflowing timeout conversion fails closed", [&value] { + warm(); + auto connection = acquire(); + expectFailure([&] { connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, value); }); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + run("failed login-timeout application releases capacity without login", [] { + py::dict attrs; + attrs[py::int_(SQL_ATTR_LOGIN_TIMEOUT)] = py::int_(30); + failNext = "attribute"; + expectFailure([&] { acquire(true, attrs); }); + require(logins == 0, "Connected despite failed login-timeout application"); + auto connection = acquire(true, attrs); + require(loginTimeouts == std::vector{30}, + "Failed timeout application did not release pool capacity"); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + run("manual mode rolls back once and parks in autocommit", [] { + warm(); + auto connection = acquire(); + connection->setAutocommit(false); + calls.clear(); + connection->close(); + expectCalls({"get", "rollback", "on"}); + checkParked(); + }); + run("statement allocation invalidates even without execution", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + statement.reset(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({}); + }); + run("failed statement allocation invalidates clean proof", [] { + warm(); + auto connection = acquire(); + failNext = "allocate_statement"; + expectFailure([&] { connection->allocStatementHandle(); }); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + run("explicit transaction and retained statement alias across leases", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + startWork(statement); + auto generation = statement->resultMetadata.snapshot().generation; + statement->resultMetadata.publish(generation, std::make_shared()); + connection->close(); + auto metadata = statement->resultMetadata.snapshot(); + require(!metadata.metadata && metadata.generation == generation + 1, + "Pool sanitation must invalidate result metadata exactly once"); + checkParked(); + connection = acquire(); + startWork(statement); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + checkParked(); + statement.reset(); + connection = acquire(); + connection->close(); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({}); + }); + run("failed cursor free cannot enable the next lease's fast path", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + startWork(statement); + failNext = "free"; + require(statement->freeHandle() == SQL_ERROR, "Expected failed free"); + connection->close(); + connection = acquire(); + startWork(statement); + connection->close(); + checkParked(); + }); + for (const auto* operation : {"get", "off", "rollback", "on"}) { + run(operation, [operation] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + startWork(statement); + statement.reset(); + calls.clear(); + failNext = operation; + expectFailure([&] { connection->close(); }); + require(std::find(calls.begin(), calls.end(), "disconnect") != calls.end(), + "Failed sanitation did not disconnect"); + if (std::string(operation) != "on") { + require(std::find(calls.begin(), calls.end(), "on") == calls.end(), + "Enabled autocommit after failed sanitation"); + } + require(commits == 0, "Failure cleanup committed work"); + connection = acquire(); + require(logins == 2, "Discard did not release capacity / replace DBC"); + connection->close(); + checkParked(); + }); + } + for (auto info : {SQL_DRIVER_HDBC, SQL_DRIVER_HENV, SQL_DRIVER_HSTMT, SQL_DRIVER_HLIB}) { + run("raw handle exposure permanently disables proof", [info] { + warm(); + auto connection = acquire(); + connection->getInfo(static_cast(info)); + connection->close(); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + run("ordinary getinfo invalidates the current lease only", [] { + warm(); + auto connection = acquire(); + connection->getInfo(SQL_DBMS_NAME); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({}); + }); + run("arbitrary attrs_before disable proof", [] { + py::dict attrs; + attrs[py::int_(SQL_ATTR_AUTOCOMMIT)] = py::int_(SQL_AUTOCOMMIT_OFF); + auto connection = acquire(true, attrs); + connection->close(); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + for (bool fail : {false, true}) { + run("generic set_attr including failure disables proof", [fail] { + warm(); + auto connection = acquire(); + if (fail) { + failNext = "attribute"; + expectFailure([&] { connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, py::int_(30)); }); + } else { + connection->setAttr(SQL_ATTR_AUTOCOMMIT, py::int_(SQL_AUTOCOMMIT_OFF)); + } + calls.clear(); + connection->close(); + if (fail) { + expectCalls({"get", "off", "rollback", "on"}); + } else { + expectCalls({"get", "rollback", "on"}); + } + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + for (const auto* operation : {"commit", "rollback", "on", "off", "get"}) { + run("failed native operation invalidates clean proof", [operation] { + warm(); + auto connection = acquire(); + failNext = operation; + expectFailure([&] { + if (std::string(operation) == "commit") { + connection->commit(); + } else if (std::string(operation) == "rollback") { + connection->rollback(); + } else if (std::string(operation) == "get") { + connection->getAutocommit(); + } else { + connection->setAutocommit(std::string(operation) == "on"); + } + }); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + for (const auto* operation : {"reset", "isolation"}) { + run("failed deferred reset replaces the physical connection", [operation] { + warm(); + failNext = operation; + auto connection = acquire(); + require(logins == 2, "Failed reset was reused"); + calls.clear(); + connection->close(); + expectCalls({"get", "off", "rollback", "on"}); + }); + } + run("abandonment rolls back and releases capacity", [] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + startWork(statement); + connection.reset(); + require(commits == 0, "Abandoned connection committed work"); + connection = acquire(); + require(logins == 2, "Abandonment did not release capacity"); + connection->close(); + checkParked(); + }); + for (bool autocommit : {true, false}) { + run("unpooled close does not run pooled sanitation", [autocommit] { + auto connection = acquire(false); + connection->setAutocommit(autocommit); + calls.clear(); + connection->close(true); + if (autocommit) { + expectCalls({"get", "disconnect", "free"}); + } else { + expectCalls({"get", "rollback", "disconnect", "free"}); + } + }); + } + for (const auto* operation : {"get", "rollback"}) { + run("unpooled failure discards without pool sanitation", [operation] { + auto connection = acquire(false); + connection->setAutocommit(false); + failNext = operation; + expectFailure([&] { connection->close(true); }); + require(std::find(calls.begin(), calls.end(), "disconnect") != calls.end(), + "Unpooled failure did not disconnect"); + require(commits == 0, "Unpooled error cleanup committed work"); + }); + } + run("raw unpooled disconnect failure preserves connection and children", [] { + auto connection = acquire(false); + connection->setAutocommit(false); + auto statement = connection->allocStatementHandle(); + startWork(statement); + calls.clear(); + expectFailure([&] { connection->close(); }); + expectCalls({"disconnect"}); + require(handles.count(statement->get()) == 1, "Live child was freed"); + auto sibling = connection->allocStatementHandle(); + connection->rollback(); + connection->close(); + require(statement->isImplicitlyFreed() && sibling->isImplicitlyFreed(), + "Successful disconnect did not invalidate children"); + }); + ConnectionPoolManager::getInstance().closePools(); + std::cout << passed << " native pool tests passed\n"; + return 0; + } catch (const std::exception& error) { + std::cerr << "FAIL after " << passed << " tests: " << error.what() << '\n'; + failNext.clear(); + ConnectionPoolManager::getInstance().closePools(); + return 1; + } +} diff --git a/tests/test_006_exceptions.py b/tests/test_006_exceptions.py index b0d117761..318e3b0e6 100644 --- a/tests/test_006_exceptions.py +++ b/tests/test_006_exceptions.py @@ -276,6 +276,129 @@ def test_connect_runtime_error_mapped_to_correct_dbapi_exception(): assert not isinstance(exc_info.value, RuntimeError) +def test_close_cleans_up_after_rollback_failure(): + """A rollback error must not defer native close to object destruction.""" + from unittest.mock import MagicMock, patch + + mock_conn = MagicMock() + mock_conn.get_autocommit.return_value = False + mock_conn.close.side_effect = RuntimeError("SQLSTATE:08S01:Communication link failure") + + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): + conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") + + with pytest.raises(OperationalError, match="Communication link failure"): + conn.close() + + mock_conn.rollback.assert_not_called() + mock_conn.close.assert_called_once_with(rollback_before_disconnect=True) + assert conn._conn is None + assert conn.closed + + +def test_close_cleans_up_after_autocommit_read_failure(): + """An autocommit read error must not bypass native close and handle release.""" + from unittest.mock import MagicMock, patch + + mock_conn = MagicMock() + mock_conn.close.side_effect = RuntimeError("SQLSTATE:08S01:Communication link failure") + + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): + conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") + + with pytest.raises(OperationalError, match="Communication link failure"): + conn.close() + + mock_conn.rollback.assert_not_called() + mock_conn.get_autocommit.assert_not_called() + mock_conn.close.assert_called_once_with(rollback_before_disconnect=True) + assert conn._conn is None + assert conn.closed + + +def test_close_delegates_manual_transaction_cleanup_to_native(): + """Only native close probes the mode and rolls back, without Python hints.""" + from unittest.mock import MagicMock, patch + + mock_conn = MagicMock() + mock_conn.get_autocommit.return_value = False + + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): + conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") + + conn.close() + + mock_conn.get_autocommit.assert_not_called() + mock_conn.rollback.assert_not_called() + mock_conn.close.assert_called_once_with(rollback_before_disconnect=True) + + +def test_autocommit_close_delegates_transaction_cleanup_to_native(): + """Autocommit may still contain an explicit SQL transaction.""" + from unittest.mock import MagicMock, patch + + mock_conn = MagicMock() + mock_conn.get_autocommit.return_value = True + + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): + conn = connect( + "Server=testserver;Database=mydb;Trusted_Connection=yes;", + autocommit=True, + ) + + conn.close() + + mock_conn.rollback.assert_not_called() + mock_conn.get_autocommit.assert_not_called() + mock_conn.close.assert_called_once_with(rollback_before_disconnect=True) + + +@pytest.mark.parametrize("preclose_failure", ["autocommit", "rollback"]) +def test_native_close_error_does_not_need_python_preclose_calls(preclose_failure): + """Native cleanup is authoritative even if Python probes would have failed.""" + from unittest.mock import MagicMock, patch + + mock_conn = MagicMock() + if preclose_failure == "autocommit": + mock_conn.get_autocommit.side_effect = RuntimeError("SQLSTATE:08S01:Autocommit read failed") + else: + mock_conn.get_autocommit.return_value = False + mock_conn.rollback.side_effect = RuntimeError("SQLSTATE:08S01:Rollback failed") + mock_conn.close.side_effect = RuntimeError("SQLSTATE:08003:Native close failed") + + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=mock_conn): + conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") + + with pytest.raises(OperationalError, match="Native close failed"): + conn.close() + + mock_conn.close.assert_called_once_with(rollback_before_disconnect=True) + mock_conn.get_autocommit.assert_not_called() + mock_conn.rollback.assert_not_called() + assert conn._conn is None + assert conn.closed + + +def test_cursor_cleanup_failure_still_calls_native_close(): + from unittest.mock import MagicMock, patch + + native = MagicMock() + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=native): + conn = connect("Server=testserver;Database=mydb;Trusted_Connection=yes;") + broken = MagicMock(closed=False) + broken.close.side_effect = RuntimeError("cursor cleanup failed") + healthy = MagicMock(closed=False) + conn._cursors = {broken, healthy} + + conn.close() + + broken.close.assert_called_once_with() + healthy.close.assert_called_once_with() + native.close.assert_called_once_with(rollback_before_disconnect=True) + assert conn.closed + assert conn._conn is None + + def test_truncate_error_message_successful_cases(): """Test truncate_error_message with valid Microsoft messages for comparison.""" diff --git a/tests/test_009_pooling.py b/tests/test_009_pooling.py index e33f71030..08f1d651e 100644 --- a/tests/test_009_pooling.py +++ b/tests/test_009_pooling.py @@ -83,6 +83,35 @@ def reset_pooling_state(): # ============================================================================= +def test_pooled_positive_login_timeout_reaches_native_attrs(): + """Exercise the Python boundary paired with native timeout call-count tests.""" + from unittest.mock import MagicMock, patch + from mssql_python.constants import ConstantsDDBC + + pooling(enabled=True, max_size=2, idle_timeout=30) + native = MagicMock() + with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=native) as create: + for _ in range(3): + connection = connect( + "Server=testserver;Database=mydb;Trusted_Connection=yes;", + timeout=30, + autocommit=True, + ) + assert create.call_args.args[1] is True + assert create.call_args.args[2] == { + ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value: 30 + } + assert connection.timeout == 0 # Query timeout is independent. + connection.close() + assert create.call_count == 3 + assert native.set_autocommit.call_count == 3 + native.set_autocommit.assert_called_with(True) + assert native.close.call_count == 3 + native.close.assert_called_with(rollback_before_disconnect=True) + native.get_autocommit.assert_not_called() + native.rollback.assert_not_called() + + def test_connection_pooling_basic(conn_str): """Test basic connection pooling functionality with multiple connections.""" # Enable pooling with small pool size @@ -127,6 +156,220 @@ def test_connection_pooling_reuse_spid(conn_str): assert spid1 == spid2, "Connections not reused - different SPIDs" +def test_pooled_close_paths_leave_no_open_transaction(conn_str): + """Every close path must leave the physical connection transaction-clean.""" + _run_in_subprocess( + """ + import os + import sys + + import mssql_python + + conn_str = os.environ["DB_CONNECTION_STRING"] + mssql_python.pooling(enabled=True, max_size=2, idle_timeout=30) + observer = mssql_python.connect(conn_str, autocommit=True) + try: + observer_cursor = observer.cursor() + + def open_transaction_count(session_id): + try: + observer_cursor.execute( + "SELECT open_transaction_count " + "FROM sys.dm_exec_sessions WHERE session_id = ?", + [session_id], + ) + except Exception as exc: + if "permission" in str(exc).lower(): + print( + "Test login cannot inspect another SQL Server session", + file=sys.stderr, + ) + sys.exit(77) + raise + return observer_cursor.fetchone() + + scenarios = ( + ("direct commit", False, "SELECT 1", None, "commit"), + ("prepared commit", False, "SELECT CAST(? AS INT)", [1], "commit"), + ("explicit rollback", False, "SELECT 1", None, "rollback"), + ("implicit close rollback", False, "SELECT 1", None, None), + ("autocommit close", True, "SELECT 1", None, None), + ( + "explicit transaction in autocommit", + True, + "BEGIN TRANSACTION; SELECT 1", + None, + None, + ), + ) + expected_spid = None + for name, autocommit, sql, params, action in scenarios: + subject = mssql_python.connect(conn_str, autocommit=autocommit) + try: + assert subject.autocommit is autocommit + cursor = subject.cursor() + cursor.execute("SELECT @@SPID") + subject_spid = cursor.fetchone()[0] + if expected_spid is None: + expected_spid = subject_spid + else: + assert subject_spid == expected_spid, ( + f"{name}: expected pooled SPID {expected_spid}, got {subject_spid}" + ) + + if open_transaction_count(subject_spid) is None: + print( + "Test login cannot inspect another SQL Server session", + file=sys.stderr, + ) + sys.exit(77) + + if params is None: + cursor.execute(sql) + else: + cursor.execute(sql, params) + cursor.fetchone() + if action == "commit": + subject.commit() + elif action == "rollback": + subject.rollback() + cursor.close() + finally: + subject.close() + + row = open_transaction_count(subject_spid) + assert row is not None, f"{name}: parked SQL Server session was not visible" + assert row[0] == 0, ( + f"{name}: pooled SPID {subject_spid} retained " + f"open_transaction_count={row[0]}" + ) + + observer_cursor.close() + finally: + observer.close() + mssql_python.pooling(enabled=False) + """, + conn_str, + ) + + +@pytest.mark.parametrize("execution", ["execute", "executemany", "failed_execute"]) +def test_autocommit_explicit_transaction_is_rolled_back_on_pool_checkin(conn_str, execution): + """Autocommit normalization must not commit an explicit SQL transaction.""" + _run_in_subprocess( + """ + import os + + import mssql_python + + conn_str = os.environ["DB_CONNECTION_STRING"] + execution = EXECUTION_MODE + table = f"pytest_pool_explicit_autocommit_transaction_{execution}" + mssql_python.pooling(enabled=True, max_size=2, idle_timeout=30) + observer = mssql_python.connect(conn_str, autocommit=True) + try: + observer_cursor = observer.cursor() + observer_cursor.execute(f"DROP TABLE IF EXISTS {table}") + observer_cursor.execute(f"CREATE TABLE {table} (id INT PRIMARY KEY)") + + subject = mssql_python.connect(conn_str, autocommit=True) + subject_cursor = subject.cursor() + subject_cursor.execute("SELECT @@SPID") + subject_spid = subject_cursor.fetchone()[0] + try: + observer_cursor.execute( + "SELECT open_transaction_count " + "FROM sys.dm_exec_sessions WHERE session_id = ?", + [subject_spid], + ) + except Exception as exc: + if "permission" in str(exc).lower(): + import sys + + print( + "Test login cannot inspect another SQL Server session", + file=sys.stderr, + ) + sys.exit(77) + raise + row = observer_cursor.fetchone() + if row is None: + import sys + + print( + "Test login cannot inspect another SQL Server session", + file=sys.stderr, + ) + sys.exit(77) + + subject_cursor.close() + subject.close() + # Exercise proven-clean, no-statement leases before reintroducing + # explicit work. Inspect the parked session without borrowing it. + for _ in range(3): + empty = mssql_python.connect(conn_str, autocommit=True) + empty.close() + observer_cursor.execute( + "SELECT open_transaction_count " + "FROM sys.dm_exec_sessions WHERE session_id = ?", + [subject_spid], + ) + row = observer_cursor.fetchone() + assert row is not None and row[0] == 0 + + subject = mssql_python.connect(conn_str, autocommit=True) + subject_cursor = subject.cursor() + subject_cursor.execute("BEGIN TRANSACTION") + if execution == "executemany": + subject_cursor.executemany( + f"INSERT INTO {table} VALUES (?)", [(1,), (2,)] + ) + elif execution == "failed_execute": + subject_cursor.execute(f"INSERT INTO {table} VALUES (1)") + try: + subject_cursor.execute(f"INSERT INTO {table} VALUES (1)") + except mssql_python.IntegrityError: + pass + else: + raise AssertionError("Expected duplicate-key execution failure") + else: + subject_cursor.execute(f"INSERT INTO {table} VALUES (1)") + subject_cursor.close() + subject.close() + + observer_cursor.execute( + "SELECT open_transaction_count " + "FROM sys.dm_exec_sessions WHERE session_id = ?", + [subject_spid], + ) + row = observer_cursor.fetchone() + assert row is not None, "Previously visible pooled session disappeared on close" + assert row[0] == 0 + + observer_cursor.execute(f"SELECT COUNT(*) FROM {table}") + assert observer_cursor.fetchone()[0] == 0 + + reused = mssql_python.connect(conn_str, autocommit=True) + try: + reused_cursor = reused.cursor() + reused_cursor.execute("SELECT @@SPID, @@TRANCOUNT") + reused_spid, transaction_count = reused_cursor.fetchone() + assert reused_spid == subject_spid + assert transaction_count == 0 + reused_cursor.close() + finally: + reused.close() + + observer_cursor.execute(f"DROP TABLE {table}") + observer_cursor.close() + finally: + observer.close() + mssql_python.pooling(enabled=False) + """.replace("EXECUTION_MODE", repr(execution)), + conn_str, + ) + + def test_connection_pooling_isolation_level_reset(conn_str): """Test that pooling correctly resets session state for isolation level. @@ -708,9 +951,9 @@ def session_identity(conn): spid, login_time = cur.fetchone() return (spid, login_time) - # Step 1: two distinct, autocommit connections. Autocommit avoids - # the implicit rollback in Connection.close(), which would - # otherwise fail on the killed session and leak its pool slot. + # Step 1: two distinct, autocommit connections. Autocommit keeps this + # test focused on detecting dead connections during checkout; failed + # manual-commit sanitation is covered separately below. victim = connect(conn_str) admin = connect(conn_str) victim.autocommit = True @@ -731,7 +974,7 @@ def session_identity(conn): admin.cursor().execute(f"KILL {victim_spid}") except Exception as e: msg = str(e) - if "permission" in msg.lower() or "KILL" in msg: + if "does not have permission to use the kill statement" in msg.lower(): import sys as _sys print( f"Skipping: KILL not permitted for this login: {msg}", @@ -751,8 +994,12 @@ def session_identity(conn): # login_time, so the identity check below catches the only # failure mode that matters. - # Step 3: return both to the pool. - victim.close() + # Step 3: close both. Sanitation of the killed connection should fail, + # discard it, and may surface that connection error to the caller. + try: + victim.close() + except Exception: + pass admin.close() # Step 4: re-acquire from the pool. Each must be working; the @@ -778,6 +1025,633 @@ def session_identity(conn): ) +def test_failed_pool_sanitation_releases_capacity(conn_str): + """A connection discarded after failed sanitation must not consume a pool slot.""" + _run_in_subprocess( + """ + import os + import sys + import time + + from mssql_python import connect, pooling + from mssql_python.connection_string_builder import _ConnectionStringBuilder + from mssql_python.connection_string_parser import _ConnectionStringParser + + conn_str = os.environ["DB_CONNECTION_STRING"] + parsed = _ConnectionStringParser(validate_keywords=True)._parse(conn_str) + normalized = {} + for key, value in parsed.items(): + canonical = _ConnectionStringParser.normalize_key(key) + if canonical not in normalized: + normalized[canonical] = value + normalized["ConnectRetryCount"] = "0" + conn_str = _ConnectionStringBuilder(normalized).build() + pooling(max_size=2, idle_timeout=30) + victim = connect(conn_str) + admin = connect(conn_str, autocommit=True) + + victim_cursor = victim.cursor() + victim_cursor.execute("SELECT @@SPID") + victim_spid = victim_cursor.fetchone()[0] + victim_cursor.close() + + try: + admin.cursor().execute(f"KILL {victim_spid}") + except Exception as exc: + message = str(exc) + if "does not have permission to use the kill statement" in message.lower(): + print( + f"Skipping: KILL not permitted for this login: {message}", + file=sys.stderr, + ) + victim.close() + admin.close() + sys.exit(77) + raise + + deadline = time.monotonic() + 10 + while True: + try: + victim.cursor().execute("SELECT 1").fetchone() + except Exception: + break + if time.monotonic() >= deadline: + raise AssertionError("KILL did not terminate the victim connection") + time.sleep(0.05) + + try: + victim.close() + except Exception: + pass + else: + raise AssertionError("Expected pooled sanitation to fail after KILL") + + admin.close() + + first = connect(conn_str) + second = connect(conn_str) + try: + assert first.cursor().execute("SELECT 1").fetchone()[0] == 1 + assert second.cursor().execute("SELECT 1").fetchone()[0] == 1 + finally: + first.close() + second.close() + pooling(enabled=False) + """, + conn_str, + ) + + +def test_old_pool_generation_cannot_enter_replacement_pool(conn_str): + """A stale checked-out connection must not alter its replacement pool.""" + _run_in_subprocess( + """ + import os + + from mssql_python import connect, pooling + + conn_str = os.environ["DB_CONNECTION_STRING"] + pooling(max_size=2, idle_timeout=30) + old = connect(conn_str, autocommit=True) + old_cursor = old.cursor() + old_cursor.execute("SELECT @@SPID") + old_spid = old_cursor.fetchone()[0] + old_cursor.close() + + pooling(enabled=False) + pooling(enabled=True, max_size=2, idle_timeout=30) + first = connect(conn_str, autocommit=True) + second = connect(conn_str, autocommit=True) + try: + first_spid = first.cursor().execute("SELECT @@SPID").fetchone()[0] + second_spid = second.cursor().execute("SELECT @@SPID").fetchone()[0] + assert first_spid != second_spid + assert old_spid not in (first_spid, second_spid) + + old.close() + + try: + third = connect(conn_str, autocommit=True) + except Exception as exc: + assert "pool" in str(exc).lower() + else: + third.close() + raise AssertionError( + "Stale connection entered or decremented the replacement pool" + ) + finally: + old.close() + first.close() + second.close() + pooling(enabled=False) + """, + conn_str, + ) + + +def test_unclosed_native_handle_destructor_releases_pool_capacity(conn_str): + """Native destructor fallback must discard its checked-out pool slot.""" + _run_in_subprocess( + """ + import gc + import os + + import mssql_python + from mssql_python import connect, pooling + + conn_str = os.environ["DB_CONNECTION_STRING"] + pooling(max_size=1, idle_timeout=30) + wrapper = connect(conn_str, autocommit=True) + native = wrapper._conn + wrapper._conn = None + wrapper._closed = True + mssql_python._active_connections.discard(wrapper) + del wrapper + del native + gc.collect() + + replacement = connect(conn_str, autocommit=True) + try: + assert replacement.cursor().execute("SELECT 1").fetchone()[0] == 1 + finally: + replacement.close() + pooling(enabled=False) + """, + conn_str, + ) + + +@pytest.mark.parametrize("use_pool", [False, True]) +@pytest.mark.parametrize("autocommit", [False, True]) +def test_native_destructor_rolls_back_pending_dml(conn_str, use_pool, autocommit): + """Native destruction must release transactions, locks, and the server session.""" + _run_in_subprocess( + f"use_pool = {use_pool!r}\nautocommit = {autocommit!r}\n" + textwrap.dedent(""" + import gc + import os + import sys + import time + import uuid + + from mssql_python import connect, ddbc_bindings as ddbc, pooling + + conn_str = os.environ["DB_CONNECTION_STRING"] + pool_key = "pytest_native_cleanup_" + uuid.uuid4().hex + table = pool_key + pooling(max_size=1, idle_timeout=30) + observer = connect(conn_str, autocommit=True) + native = ddbc.Connection(conn_str, use_pool, {}, pool_key, None) + statement = native.alloc_statement_handle() + try: + assert ddbc.DDBCSQLExecDirect(statement, "SELECT @@SPID") in (0, 1) + row = [] + assert ddbc.DDBCSQLFetchOne(statement, row) in (0, 1) + session_id = row[0] + statement.free() + + cursor = observer.cursor() + try: + cursor.execute( + "SELECT session_id FROM sys.dm_exec_sessions WHERE session_id = ?", + [session_id], + ) + except Exception as exc: + if "permission" in str(exc).lower(): + print("Observer cannot inspect the native session", file=sys.stderr) + sys.exit(77) + raise + if cursor.fetchone() is None: + print("Observer cannot inspect the native session", file=sys.stderr) + sys.exit(77) + + cursor.execute("SET LOCK_TIMEOUT 1000") + cursor.execute(f"CREATE TABLE {table} (id INT)") + try: + native.set_autocommit(autocommit) + statement = native.alloc_statement_handle() + sql = f"INSERT INTO {table} VALUES (1)" + if autocommit: + sql = "BEGIN TRANSACTION; " + sql + assert ddbc.DDBCSQLExecDirect(statement, sql) in (0, 1) + statement.free() + statement = None + native = None + gc.collect() + + cursor.execute(f"SELECT COUNT(*) FROM {table} WITH (READCOMMITTEDLOCK)") + assert cursor.fetchone()[0] == 0, "Destructor committed abandoned work" + + deadline = time.monotonic() + 5 + while True: + cursor.execute( + "SELECT session_id FROM sys.dm_exec_sessions WHERE session_id = ?", + [session_id], + ) + if cursor.fetchone() is None: + break + assert time.monotonic() < deadline, "Native session survived destruction" + time.sleep(0.05) + + replacement = ddbc.Connection(conn_str, use_pool, {}, pool_key, None) + replacement_statement = replacement.alloc_statement_handle() + try: + assert ddbc.DDBCSQLExecDirect(replacement_statement, "SELECT 1") in (0, 1) + row = [] + assert ddbc.DDBCSQLFetchOne(replacement_statement, row) in (0, 1) + assert row == [1] + finally: + replacement_statement.free() + replacement.close() + finally: + cursor.execute(f"DROP TABLE {table}") + cursor.close() + finally: + if statement is not None: + statement.free() + if native is not None: + native.rollback() + native.close() + observer.close() + pooling(enabled=False) + """), + conn_str, + ) + + +@pytest.mark.parametrize("explicit_close", [False, True]) +def test_native_disconnect_with_concurrent_child_gc(conn_str, explicit_close): + """Child wrappers collected during disconnect must not double-free statements.""" + _run_in_subprocess( + f"explicit_close = {explicit_close!r}\n" + textwrap.dedent(""" + import gc + import os + import threading + + from mssql_python import ddbc_bindings as ddbc + + class StatementCycle: + def __init__(self, statement): + self.statement = statement + self.cycle = self + + barrier = threading.Barrier(2, timeout=10) + errors = [] + iterations = 50 + + def collect_children(): + try: + for _ in range(iterations): + barrier.wait() + gc.collect() + barrier.wait() + except Exception as exc: + errors.append(exc) + barrier.abort() + + gc.disable() + collector = threading.Thread(target=collect_children, daemon=True) + collector.start() + try: + for _ in range(iterations): + native = ddbc.Connection(os.environ["DB_CONNECTION_STRING"], False) + native.set_autocommit(True) + statement = native.alloc_statement_handle() + assert ddbc.DDBCSQLExecDirect(statement, "SELECT 1") in (0, 1) + cycle = StatementCycle(statement) + del statement, cycle + barrier.wait() + if explicit_close: + native.close() + native = None + barrier.wait() + finally: + collector.join(timeout=10) + if collector.is_alive(): + barrier.abort() + collector.join(timeout=10) + gc.enable() + assert not collector.is_alive(), "GC worker did not exit" + assert not errors, errors + gc.collect() + """), + conn_str, + ) + + +@pytest.mark.parametrize("explicit_close", [False, True]) +def test_cursor_cyclic_finalizer_with_concurrent_native_disconnect(conn_str, explicit_close): + """Exercise real Cursor.close/free after cyclic GC removes its WeakSet entry. + + The Python finalizer/WeakSet ordering is coordinated; overlap inside the + native cleanup calls is stress coverage, not a deterministic race trigger. + """ + _run_in_subprocess( + f"explicit_close = {explicit_close!r}\n" + textwrap.dedent(""" + import gc + import os + import threading + import weakref + + import mssql_python + from mssql_python import connect, ddbc_bindings as ddbc, pooling + + iterations = 50 + collect_barrier = threading.Barrier(2, timeout=10) + cleanup_barrier = threading.Barrier(2, timeout=10) + free_entered = threading.Event() + errors = [] + + class FinalizerStatement: + # Only coordinate entry: Cursor.__del__/close and native free + # still run their real implementations, with a real SQL handle. + def __init__(self, statement): + self.statement = statement + self.calls = 0 + self.completed = False + + def free(self): + self.calls += 1 + free_entered.set() + try: + cleanup_barrier.wait() + assert self.statement.free() is None + self.completed = True + except Exception as exc: + errors.append(f"Cursor finalizer: {exc!r}") + raise + + def collect_children(): + try: + for _ in range(iterations): + collect_barrier.wait() + gc.collect() + collect_barrier.wait() + except Exception as exc: + errors.append(f"GC worker: {exc!r}") + collect_barrier.abort() + cleanup_barrier.abort() + free_entered.set() + + pooling(enabled=False) + gc.disable() + collector = threading.Thread(target=collect_children, daemon=True) + collector.start() + connection = None + native = None + try: + for _ in range(iterations): + free_entered.clear() + connection = connect(os.environ["DB_CONNECTION_STRING"], autocommit=True) + cursor = connection.cursor() + assert cursor.execute("SELECT 1").fetchall()[0][0] == 1 + finalizer_statement = FinalizerStatement(cursor.hstmt) + cursor.hstmt = finalizer_statement + cursor.cycle = cursor + cursor_ref = weakref.ref(cursor) + del cursor + + collect_barrier.wait() + assert free_entered.wait(10), "Cursor finalizer did not enter free" + assert not errors, errors + assert cursor_ref() is None, "GC did not clear the cursor weakref" + assert not connection._cursors, "Connection.close would still see the cursor" + + if not explicit_close: + # The cursor retains its Python connection. Detach only + # the native owner to exercise its destructor fallback. + native = connection._conn + connection._conn = None + connection._closed = True + mssql_python._active_connections.discard(connection) + + cleanup_barrier.wait() + if explicit_close: + connection.close() + else: + native = None + collect_barrier.wait() + + assert finalizer_statement.calls == 1 + assert finalizer_statement.completed, errors + assert not errors, errors + assert finalizer_statement.statement.free() is None + assert ddbc.DDBCSQLFreeHandle(3, finalizer_statement.statement) == -2 + assert connection.closed + connection = None + collector.join(timeout=10) + assert not collector.is_alive(), "GC worker did not exit" + finally: + collect_barrier.abort() + cleanup_barrier.abort() + collector.join(timeout=10) + if connection is not None: + connection.close() + native = None + gc.enable() + assert not collector.is_alive(), "GC worker did not exit" + assert not errors, errors + gc.collect() + """), + conn_str, + ) + + +def test_failed_native_disconnect_preserves_child_statement(conn_str): + """SQLSTATE 25000 must not irreversibly invalidate a live child handle.""" + _run_in_subprocess( + """ + import os + import uuid + + from mssql_python import connect, ddbc_bindings as ddbc, pooling + + pooling(enabled=False) + conn_str = os.environ["DB_CONNECTION_STRING"] + table = "pytest_disconnect_failure_" + uuid.uuid4().hex + observer = connect(conn_str, autocommit=True) + observer_cursor = observer.cursor() + native = None + statement = None + created = False + try: + observer_cursor.execute("SET LOCK_TIMEOUT 1000") + observer_cursor.execute(f"CREATE TABLE {table} (id INT)") + created = True + native = ddbc.Connection(conn_str, False) + native.set_autocommit(False) + statement = native.alloc_statement_handle() + assert ddbc.DDBCSQLExecDirect(statement, f"INSERT INTO {table} VALUES (1)") in (0, 1) + + try: + native.close() + except RuntimeError as exc: + assert "25000" in str(exc), f"Unexpected disconnect failure: {exc}" + else: + raise AssertionError("Native disconnect accepted an uncommitted INSERT") + + extra_statement = native.alloc_statement_handle() + try: + assert ddbc.DDBCSQLExecDirect(extra_statement, "SELECT 42") in (0, 1) + extra_row = [] + assert ddbc.DDBCSQLFetchOne(extra_statement, extra_row) in (0, 1) + assert extra_row == [42] + finally: + extra_statement.free() + + assert ddbc.DDBCSQLExecDirect( + statement, f"SELECT COUNT(*), @@TRANCOUNT FROM {table}" + ) in (0, 1) + row = [] + assert ddbc.DDBCSQLFetchOne(statement, row) in (0, 1) + assert row[0] == 1 and row[1] > 0, row + statement._close_cursor() + native.rollback() + + assert ddbc.DDBCSQLExecDirect(statement, f"SELECT COUNT(*) FROM {table}") in (0, 1) + row = [] + assert ddbc.DDBCSQLFetchOne(statement, row) in (0, 1) + assert row == [0], "Failed disconnect committed the pending INSERT" + statement._close_cursor() + native.rollback() + native.close() + native = None + + # Disconnect already freed the ODBC statement. The raw entry point + # must consume the wrapper's implicit-free state, not the stale pointer. + assert ddbc.DDBCSQLFreeHandle(3, statement) in (0, 1) + assert ddbc.DDBCSQLFreeHandle(3, statement) == -2 + assert statement.free() is None + assert statement.free() is None + observer_cursor.execute(f"SELECT COUNT(*) FROM {table} WITH (READCOMMITTEDLOCK)") + assert observer_cursor.fetchone()[0] == 0 + finally: + try: + if native is not None: + try: + native.rollback() + finally: + native.close() + if statement is not None: + statement.free() + finally: + try: + if created: + observer_cursor.execute(f"DROP TABLE {table}") + finally: + observer_cursor.close() + observer.close() + """, + conn_str, + ) + + +@pytest.mark.parametrize("free_api", ["method", "raw"]) +def test_native_statement_free_entrypoints_are_idempotent(conn_str, free_api): + """Raw SQLRETURN and public None-returning free share one ownership state.""" + _run_in_subprocess( + f"free_api = {free_api!r}\n" + textwrap.dedent(""" + import os + + from mssql_python import ddbc_bindings as ddbc + + native = ddbc.Connection(os.environ["DB_CONNECTION_STRING"], False) + native.set_autocommit(True) + statement = native.alloc_statement_handle() + sibling = native.alloc_statement_handle() + try: + assert ddbc.DDBCSQLExecDirect(statement, "SELECT 1") in (0, 1) + if free_api == "raw": + assert ddbc.DDBCSQLFreeHandle(3, statement) in (0, 1) + else: + assert statement.free() is None + assert ddbc.DDBCSQLFreeHandle(3, statement) == -2 + assert statement.free() is None + assert statement.free() is None + + assert ddbc.DDBCSQLExecDirect(sibling, "SELECT 42") in (0, 1) + row = [] + assert ddbc.DDBCSQLFetchOne(sibling, row) in (0, 1) + assert row == [42] + native.close() + native = None + assert ddbc.DDBCSQLFreeHandle(3, sibling) in (0, 1) + assert ddbc.DDBCSQLFreeHandle(3, sibling) == -2 + assert sibling.free() is None + finally: + statement.free() + sibling.free() + if native is not None: + native.close() + """), + conn_str, + ) + + +def test_native_statement_allocation_racing_disconnect(conn_str): + """Allocation either registers before disconnect or rejects its closed state.""" + _run_in_subprocess( + """ + import os + import threading + + from mssql_python import ddbc_bindings as ddbc + + barrier = threading.Barrier(2, timeout=10) + errors = [] + statements = [] + iterations = 100 + native = None + + def allocate(): + try: + for _ in range(iterations): + barrier.wait() + try: + statements.append(native.alloc_statement_handle()) + except RuntimeError as exc: + assert str(exc) in ( + "Connection object is not initialized", + "Connection handle not allocated", + ), str(exc) + barrier.wait() + except Exception as exc: + errors.append(repr(exc)) + barrier.abort() + + worker = threading.Thread(target=allocate, daemon=True) + worker.start() + try: + for _ in range(iterations): + native = ddbc.Connection(os.environ["DB_CONNECTION_STRING"], False) + native.set_autocommit(True) + barrier.wait() + native.close() + barrier.wait() + assert not errors, errors + for statement in statements: + assert ddbc.DDBCSQLFreeHandle(3, statement) in (0, 1) + assert statement.free() is None + statements.clear() + try: + native.alloc_statement_handle() + except RuntimeError as exc: + assert "Connection object is not initialized" in str(exc) + else: + raise AssertionError("Allocation succeeded after native close") + finally: + worker.join(timeout=10) + if worker.is_alive(): + barrier.abort() + worker.join(timeout=10) + for statement in statements: + statement.free() + assert not worker.is_alive(), "Allocation worker did not exit" + assert not errors, errors + """, + conn_str, + ) + + def test_pool_recovery_after_failed_connection(conn_str): """Test that the pool recovers after a failed connection attempt.""" pooling(max_size=1, idle_timeout=30) From cf988f4dcaab9eb0253eb5b8ac82303a6d6d465e Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Fri, 25 Sep 2026 20:15:50 +0100 Subject: [PATCH 03/11] STYLE: Format pooling timeout regression assertion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_009_pooling.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_009_pooling.py b/tests/test_009_pooling.py index 08f1d651e..4ee97cd35 100644 --- a/tests/test_009_pooling.py +++ b/tests/test_009_pooling.py @@ -98,9 +98,7 @@ def test_pooled_positive_login_timeout_reaches_native_attrs(): autocommit=True, ) assert create.call_args.args[1] is True - assert create.call_args.args[2] == { - ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value: 30 - } + assert create.call_args.args[2] == {ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value: 30} assert connection.timeout == 0 # Query timeout is independent. connection.close() assert create.call_count == 3 From e7444e70f56e31d9e3c9c37365fc97901ec7ee85 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Sat, 26 Sep 2026 10:39:39 +0100 Subject: [PATCH 04/11] PERF: Reduce pooled connection cleanup overhead Replace redundant autocommit transitions with a reset-triggered rollback batch and preserve proof so checkout can skip duplicate session reset work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 73 +++++++++++++--- mssql_python/pybind/connection/connection.h | 3 + tests/native/pool_sanitation.cpp | 85 +++++++++++++------ tests/test_009_pooling.py | 7 ++ 4 files changed, 131 insertions(+), 37 deletions(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 653a15524..bde49044b 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -288,6 +288,7 @@ void Connection::clearResultMetadata() { void Connection::commit() { PERF_TIMER("Connection::commit"); _poolClean = false; + _poolSessionReset = false; if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } @@ -306,6 +307,7 @@ void Connection::commit() { void Connection::rollback() { PERF_TIMER("Connection::rollback"); _poolClean = false; + _poolSessionReset = false; if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } @@ -325,6 +327,7 @@ void Connection::setAutocommit(bool enable) { PERF_TIMER("Connection::setAutocommit"); if (!enable) { _poolClean = false; + _poolSessionReset = false; } if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); @@ -376,6 +379,7 @@ SqlHandlePtr Connection::allocStatementHandle() { // Every execution/catalog/fetch path, including direct native calls, needs // a statement handle. Retained handles also prevent re-establishing proof. _poolClean = false; + _poolSessionReset = false; LOG("Allocating statement handle"); // Keep the wrapper outside the lock scope: unwinding a failed registration // frees the statement through the same cleanup gate. @@ -434,6 +438,7 @@ SqlHandlePtr Connection::allocStatementHandle() { SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) { _poolClean = false; + _poolSessionReset = false; // A scalar login timeout only bounds connection establishment; it cannot // execute user work or retain a deferred buffer. Keep all other attributes // conservative, and never clear an earlier permanent invalidation. @@ -644,6 +649,10 @@ bool Connection::reset() { if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } + if (_poolClean && _poolSessionReset && !_poolProofDisabled) { + updateLastUsed(); + return true; + } clearResultMetadata(); LOG("Resetting connection via SQL_ATTR_RESET_CONNECTION"); // NOTE: SQL_ATTR_RESET_CONNECTION is a pool-checkin reset: it asks the @@ -702,8 +711,11 @@ void Connection::prepareForPool() { } _poolClean = false; + _poolSessionReset = false; clearResultMetadata(); SQLRETURN ret; + std::string statementError; + bool sessionReset = false; { // One GIL release and metadata invalidation for the whole sequence. // Do not trust Python mode/rollback hints: native callers and set_attr @@ -718,24 +730,58 @@ void Connection::prepareForPool() { &mode, sizeof(mode), &length); } if (SQL_SUCCEEDED(ret) && mode == SQL_AUTOCOMMIT_ON) { - PERF_TIMER("Connection::prepareForPool::autocommit_off"); - ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, - reinterpret_cast(SQL_AUTOCOMMIT_OFF), 0); - } - if (SQL_SUCCEEDED(ret)) { - PERF_TIMER("Connection::prepareForPool::rollback"); + // SQLEndTran is a no-op in autocommit mode, but SQL Server still + // permits an explicit BEGIN TRANSACTION. Schedule the driver's + // session reset first so states such as SET NOEXEC ON cannot suppress + // the rollback batch that triggers it. + PERF_TIMER("Connection::prepareForPool::rollback_autocommit"); + ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_RESET_CONNECTION, + reinterpret_cast(SQL_RESET_CONNECTION_YES), + SQL_IS_INTEGER); + if (SQL_SUCCEEDED(ret)) { + SQLHANDLE statement = nullptr; + ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &statement); + if (SQL_SUCCEEDED(ret)) { + const std::u16string rollbackQuery = + u"IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION; " + u"SET TRANSACTION ISOLATION LEVEL READ COMMITTED"; + ret = SQLExecDirect_ptr(statement, + reinterpretU16stringAsSqlWChar(rollbackQuery), SQL_NTS); + if (!SQL_SUCCEEDED(ret)) { + ErrorInfo error = SQLReadError(SQL_HANDLE_STMT, statement, ret); + statementError = error.sqlState.length() == 5 + ? "SQLSTATE:" + error.sqlState + ":" + error.ddbcErrorMsg + : error.ddbcErrorMsg; + } + SQLRETURN freeRet = SQLFreeHandle_ptr(SQL_HANDLE_STMT, statement); + if (SQL_SUCCEEDED(ret) && !SQL_SUCCEEDED(freeRet)) { + ErrorInfo error = SQLReadError(SQL_HANDLE_STMT, statement, freeRet); + statementError = error.sqlState.length() == 5 + ? "SQLSTATE:" + error.sqlState + ":" + error.ddbcErrorMsg + : error.ddbcErrorMsg; + ret = freeRet; + } + sessionReset = SQL_SUCCEEDED(ret); + } + } + } else if (SQL_SUCCEEDED(ret)) { + PERF_TIMER("Connection::prepareForPool::rollback_manual"); ret = SQLEndTran_ptr(SQL_HANDLE_DBC, _dbcHandle->get(), SQL_ROLLBACK); + // Never enable autocommit after a failed rollback: it could commit + // abandoned work. Manual mode can leave even an empty transaction open. + if (SQL_SUCCEEDED(ret)) { + PERF_TIMER("Connection::prepareForPool::autocommit_on"); + ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, + reinterpret_cast(SQL_AUTOCOMMIT_ON), 0); + } } - // Never enable autocommit after a failed rollback: it could commit - // abandoned work. Manual mode can leave even an empty transaction open. - if (SQL_SUCCEEDED(ret)) { - PERF_TIMER("Connection::prepareForPool::autocommit_on"); - ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, - reinterpret_cast(SQL_AUTOCOMMIT_ON), 0); - } + } + if (!statementError.empty()) { + ThrowStdException(statementError); } checkError(ret); _autocommit = true; + _poolSessionReset = sessionReset; updateLastUsed(); // A native statement alias can execute again without another allocation, // even in a later lease. Only expired wrappers permit the fast path. @@ -938,6 +984,7 @@ SqlHandlePtr ConnectionHandle::allocStatementHandle() { py::object Connection::getInfo(SQLUSMALLINT infoType) const { _poolClean = false; + _poolSessionReset = false; if (infoType == SQL_DRIVER_HDBC || infoType == SQL_DRIVER_HENV || infoType == SQL_DRIVER_HSTMT || infoType == SQL_DRIVER_HLIB) { _poolProofDisabled = true; diff --git a/mssql_python/pybind/connection/connection.h b/mssql_python/pybind/connection/connection.h index a8aca3765..c9dc6fab5 100644 --- a/mssql_python/pybind/connection/connection.h +++ b/mssql_python/pybind/connection/connection.h @@ -111,6 +111,9 @@ class Connection { // Only successful rollback + AUTOCOMMIT_ON establishes this proof. A new // login or deferred SQL_ATTR_RESET_CONNECTION alone does not establish it. mutable bool _poolClean = false; + // True only after check-in has triggered the driver reset and restored the + // default isolation level. This permits checkout to skip a duplicate reset. + mutable bool _poolSessionReset = false; // Escaped raw handles and arbitrary/deferred attributes cannot be tracked. mutable bool _poolProofDisabled = false; SqlHandlePtr _dbcHandle; diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp index 13c521070..6062ad021 100644 --- a/tests/native/pool_sanitation.cpp +++ b/tests/native/pool_sanitation.cpp @@ -18,6 +18,7 @@ struct Handle { bool autocommit = true; bool transaction = false; bool pendingWork = false; + bool resetPending = false; SQLULEN loginTimeout = 0; }; @@ -95,6 +96,8 @@ SQLRETURN SQL_API setAttr(SQLHDBC dbc, SQLINTEGER attribute, SQLPOINTER value, S handle.transaction = handle.pendingWork = false; } handle.autocommit = value != nullptr; + } else if (attribute == SQL_ATTR_RESET_CONNECTION) { + handle.resetPending = true; } else if (attribute == SQL_ATTR_LOGIN_TIMEOUT && length == SQL_IS_INTEGER) { handle.loginTimeout = reinterpret_cast(value); } @@ -127,6 +130,19 @@ SQLRETURN SQL_API endTran(SQLSMALLINT, SQLHANDLE dbc, SQLSMALLINT completion) { return ret; } +SQLRETURN SQL_API executeDirect(SQLHSTMT statement, SQLWCHAR*, SQLINTEGER) { + auto& handle = *handles.at(handles.at(statement)->parent); + if (handle.resetPending) { + handle.transaction = handle.pendingWork = false; + handle.resetPending = false; + } + auto ret = record("rollback_batch"); + if (SQL_SUCCEEDED(ret)) { + handle.transaction = handle.pendingWork = false; + } + return ret; +} + SQLRETURN SQL_API disconnect(SQLHDBC dbc) { record("disconnect"); if (handles.at(dbc)->pendingWork) { @@ -234,6 +250,7 @@ int main() { SQLDriverConnect_ptr = login; SQLSetConnectAttr_ptr = setAttr; SQLGetConnectAttr_ptr = getAttr; + SQLExecDirect_ptr = executeDirect; SQLEndTran_ptr = endTran; SQLDisconnect_ptr = disconnect; SQLGetInfo_ptr = getInfo; @@ -251,9 +268,11 @@ int main() { auto connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); for (int i = 0; i < 100; ++i) { + calls.clear(); connection = acquire(); + expectCalls({"alive"}); connection->setAutocommit(true); calls.clear(); connection->close(true); @@ -273,7 +292,7 @@ int main() { "Login timeout was not applied exactly once"); calls.clear(); connection->close(true); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); for (int i = 0; i < 100; ++i) { connection = acquire(true, attrs); connection->setAutocommit(true); @@ -294,7 +313,7 @@ int main() { connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, py::int_(30)); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); checkParked(); connection = acquire(); calls.clear(); @@ -324,7 +343,7 @@ int main() { connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } for (const py::object& value : std::vector{ @@ -336,11 +355,11 @@ int main() { connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, value); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } for (const py::object& value : std::vector{ @@ -351,11 +370,11 @@ int main() { expectFailure([&] { connection->setAttr(SQL_ATTR_LOGIN_TIMEOUT, value); }); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } run("failed login-timeout application releases capacity without login", [] { @@ -369,7 +388,7 @@ int main() { "Failed timeout application did not release pool capacity"); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); run("manual mode rolls back once and parks in autocommit", [] { warm(); @@ -387,7 +406,7 @@ int main() { statement.reset(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); connection = acquire(); calls.clear(); connection->close(); @@ -400,7 +419,7 @@ int main() { expectFailure([&] { connection->allocStatementHandle(); }); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); run("explicit transaction and retained statement alias across leases", [] { warm(); @@ -418,7 +437,7 @@ int main() { startWork(statement); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); checkParked(); statement.reset(); connection = acquire(); @@ -441,7 +460,7 @@ int main() { connection->close(); checkParked(); }); - for (const auto* operation : {"get", "off", "rollback", "on"}) { + for (const auto* operation : {"get", "reset", "allocate_statement", "rollback_batch", "free"}) { run(operation, [operation] { auto connection = acquire(); auto statement = connection->allocStatementHandle(); @@ -452,9 +471,25 @@ int main() { expectFailure([&] { connection->close(); }); require(std::find(calls.begin(), calls.end(), "disconnect") != calls.end(), "Failed sanitation did not disconnect"); - if (std::string(operation) != "on") { + require(commits == 0, "Failure cleanup committed work"); + connection = acquire(); + require(logins == 2, "Discard did not release capacity / replace DBC"); + connection->close(); + checkParked(); + }); + } + for (const auto* operation : {"rollback", "on"}) { + run("manual-mode sanitation failure", [operation] { + auto connection = acquire(); + connection->setAutocommit(false); + calls.clear(); + failNext = operation; + expectFailure([&] { connection->close(); }); + require(std::find(calls.begin(), calls.end(), "disconnect") != calls.end(), + "Failed manual-mode sanitation did not disconnect"); + if (std::string(operation) == "rollback") { require(std::find(calls.begin(), calls.end(), "on") == calls.end(), - "Enabled autocommit after failed sanitation"); + "Enabled autocommit after failed rollback"); } require(commits == 0, "Failure cleanup committed work"); connection = acquire(); @@ -472,7 +507,7 @@ int main() { connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } run("ordinary getinfo invalidates the current lease only", [] { @@ -481,7 +516,7 @@ int main() { connection->getInfo(SQL_DBMS_NAME); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); connection = acquire(); calls.clear(); connection->close(); @@ -495,7 +530,7 @@ int main() { connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); for (bool fail : {false, true}) { run("generic set_attr including failure disables proof", [fail] { @@ -510,14 +545,14 @@ int main() { calls.clear(); connection->close(); if (fail) { - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); } else { expectCalls({"get", "rollback", "on"}); } connection = acquire(); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } for (const auto* operation : {"commit", "rollback", "on", "off", "get"}) { @@ -538,18 +573,20 @@ int main() { }); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } for (const auto* operation : {"reset", "isolation"}) { run("failed deferred reset replaces the physical connection", [operation] { - warm(); - failNext = operation; auto connection = acquire(); + connection->setAutocommit(false); + connection->close(); + failNext = operation; + connection = acquire(); require(logins == 2, "Failed reset was reused"); calls.clear(); connection->close(); - expectCalls({"get", "off", "rollback", "on"}); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); } run("abandonment rolls back and releases capacity", [] { diff --git a/tests/test_009_pooling.py b/tests/test_009_pooling.py index 4ee97cd35..1bcdde646 100644 --- a/tests/test_009_pooling.py +++ b/tests/test_009_pooling.py @@ -199,6 +199,13 @@ def open_transaction_count(session_id): None, None, ), + ( + "explicit transaction with noexec", + True, + "BEGIN TRANSACTION; SELECT 1; SET NOEXEC ON", + None, + None, + ), ) expected_spid = None for name, autocommit, sql, params, action in scenarios: From b75971582bb0d9a95f8cbd8326eec7ed677465c5 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Wed, 7 Oct 2026 07:40:23 +0100 Subject: [PATCH 05/11] PERF: Avoid redundant pooled setters and empty diagnostic reads Preserve native sanitation proof and original diagnostic enumeration on uncertain header results. Add deterministic ODBC-call and diagnostic-preservation regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 6 + mssql_python/pybind/ddbc_bindings.cpp | 14 +- tests/native/pool_sanitation.cpp | 311 ++++++++++++++++++ 3 files changed, 330 insertions(+), 1 deletion(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index bde49044b..9a158c9a2 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -333,6 +333,12 @@ void Connection::setAutocommit(bool enable) { ThrowStdException("Connection handle not allocated"); } clearResultMetadata(); + // Only successful native sanitation with no retained statement/raw handle + // aliases proves ON. A new login or _autocommit alone is not proof. + if (enable && _dbcHandle->get() && _poolClean && !_poolProofDisabled) { + _autocommit = true; + return; + } SQLINTEGER value = enable ? SQL_AUTOCOMMIT_ON : SQL_AUTOCOMMIT_OFF; LOG("Setting autocommit=%d", enable); SQLRETURN ret; diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 4a8184bb9..fdd3010b0 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -2025,10 +2025,22 @@ py::list SQLGetAllDiagRecords(SqlHandlePtr handle) { return records; } +static bool FetchDiagnosticsAreEmpty(SQLHSTMT hStmt) { + if (!SQLGetDiagField_ptr) + return false; + // Diagnostic header reads preserve the originating records. An unsupported + // or uncertain count must retain the original record enumeration. + SQLINTEGER count = -1; + SQLRETURN ret = SQLGetDiagField_ptr(SQL_HANDLE_STMT, hStmt, 0, SQL_DIAG_NUMBER, + &count, 0, nullptr); + return ret == SQL_SUCCESS && count == 0; +} + // Called only with the GIL held, immediately after the originating ODBC call. static void CaptureFetchDiagnostics(SQLHSTMT hStmt, SQLRETURN ret, py::handle messages, bool internalTruncation = false) { - if ((ret == SQL_SUCCESS_WITH_INFO || ret == SQL_NO_DATA) && messages && !messages.is_none()) + if ((ret == SQL_SUCCESS_WITH_INFO || ret == SQL_NO_DATA) && messages && !messages.is_none() && + !FetchDiagnosticsAreEmpty(hStmt)) AppendDiagRecords(hStmt, SQL_HANDLE_STMT, messages, internalTruncation); } diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp index 6062ad021..19cab6c43 100644 --- a/tests/native/pool_sanitation.cpp +++ b/tests/native/pool_sanitation.cpp @@ -11,6 +11,12 @@ #include #include +SQLSMALLINT SQLNumResultCols_wrap(SqlHandlePtr statementHandle, py::handle messages); +SQLRETURN FetchOne_wrap(SqlHandlePtr statementHandle, py::list& row, + const std::string& charEncoding, const std::string& wcharEncoding, + int charCtype, py::handle messages); +py::list SQLGetAllDiagRecords(SqlHandlePtr handle); + namespace { struct Handle { SQLSMALLINT type; @@ -194,6 +200,119 @@ void expectCalls(std::initializer_list expected) { } } +SQLRETURN SQL_API freeStatement(SQLHSTMT, SQLUSMALLINT option) { + require(option == SQL_UNBIND, "Unexpected statement cleanup"); + return record("statement_unbind"); +} + +struct DiagnosticRecord { + std::u16string state; + std::u16string message; + SQLRETURN result = SQL_SUCCESS; +}; +std::vector fetchRecords; +SQLRETURN fetchResult = SQL_SUCCESS_WITH_INFO; +SQLRETURN numberResult = SQL_SUCCESS; +SQLINTEGER diagnosticCount = 0; +bool writeDiagnosticCount = true; +SQLRETURN stateResult = SQL_SUCCESS; + +SQLRETURN SQL_API fetchDiagnostic(SQLSMALLINT type, SQLHANDLE, SQLSMALLINT number, + SQLWCHAR* state, SQLINTEGER* native, SQLWCHAR* message, + SQLSMALLINT capacity, SQLSMALLINT* length) { + require(type == SQL_HANDLE_STMT && number > 0, "Invalid record lookup"); + record("diag_record"); + if (static_cast(number) > fetchRecords.size()) return SQL_NO_DATA; + const auto& item = fetchRecords[number - 1]; + require(item.state.size() == 5, "Invalid test SQLSTATE"); + std::copy(item.state.begin(), item.state.end(), state); + state[5] = 0; + *native = 42; + const auto size = std::min(item.message.size(), static_cast(capacity - 1)); + std::copy_n(item.message.begin(), size, message); + message[size] = 0; + *length = static_cast(item.message.size()); + return item.result; +} + +SQLRETURN SQL_API fetchDiagnosticField(SQLSMALLINT type, SQLHANDLE, SQLSMALLINT number, + SQLSMALLINT identifier, SQLPOINTER value, + SQLSMALLINT capacity, SQLSMALLINT* length) { + require(type == SQL_HANDLE_STMT, "Wrong diagnostic handle type"); + if (identifier == SQL_DIAG_NUMBER) { + record("diag_number"); + require(number == 0 && capacity == 0 && length == nullptr, + "SQL_DIAG_NUMBER must read the numeric header field"); + if (writeDiagnosticCount) *static_cast(value) = diagnosticCount; + return numberResult; + } + record("diag_state"); + require(identifier == SQL_DIAG_SQLSTATE && number > 0, "Unexpected diagnostic field"); + if (stateResult != SQL_SUCCESS) return stateResult; + if (static_cast(number) > fetchRecords.size()) return SQL_NO_DATA; + const auto& state = fetchRecords[number - 1].state; + require(capacity >= static_cast(6 * sizeof(SQLWCHAR)), "SQLSTATE buffer too small"); + std::copy(state.begin(), state.end(), static_cast(value)); + static_cast(value)[5] = 0; + return SQL_SUCCESS; +} + +SQLRETURN SQL_API fetchColumnCount(SQLHSTMT, SQLSMALLINT* count) { + record("column_count"); + *count = 0; + return fetchResult; +} + +SQLRETURN SQL_API fetchNullLob(SQLHANDLE, SQLUSMALLINT, SQLSMALLINT, SQLPOINTER, SQLLEN, + SQLLEN* length) { + record("get_lob"); + *length = SQL_NULL_DATA; + return fetchResult; +} + +SQLRETURN SQL_API fetchNoData(SQLHANDLE) { + record("fetch_no_data"); + return SQL_NO_DATA; +} + +struct FetchDiagnosticScope { + SQLGetDiagRecFunc oldRecord = SQLGetDiagRec_ptr; + SQLGetDiagFieldFunc oldField = SQLGetDiagField_ptr; + SQLNumResultColsFunc oldCount = SQLNumResultCols_ptr; + SQLGetDataFunc oldData = SQLGetData_ptr; + SQLFetchFunc oldFetch = SQLFetch_ptr; + SQLFreeStmtFunc oldFreeStmt = SQLFreeStmt_ptr; + + FetchDiagnosticScope() { + fetchRecords.clear(); + fetchResult = SQL_SUCCESS_WITH_INFO; + numberResult = stateResult = SQL_SUCCESS; + diagnosticCount = 0; + writeDiagnosticCount = true; + SQLGetDiagRec_ptr = fetchDiagnostic; + SQLGetDiagField_ptr = fetchDiagnosticField; + SQLNumResultCols_ptr = fetchColumnCount; + SQLGetData_ptr = fetchNullLob; + SQLFetch_ptr = fetchNoData; + SQLFreeStmt_ptr = freeStatement; + } + ~FetchDiagnosticScope() { + SQLGetDiagRec_ptr = oldRecord; + SQLGetDiagField_ptr = oldField; + SQLNumResultCols_ptr = oldCount; + SQLGetData_ptr = oldData; + SQLFetch_ptr = oldFetch; + SQLFreeStmt_ptr = oldFreeStmt; + } +}; + +void expectMessage(const py::list& messages, size_t index, const std::string& state, + const std::string& text) { + auto message = messages[index].cast(); + require(message[0].cast() == "[" + state + "] (42)" && + message[1].cast() == text, "Diagnostic message changed"); +} + void expectFailure(const std::function& action) { try { action(); @@ -264,6 +383,132 @@ int main() { std::cout << "PASS " << name << '\n'; }; try { + for (SQLRETURN origin : {SQLRETURN(SQL_SUCCESS_WITH_INFO), SQLRETURN(SQL_NO_DATA)}) { + run("successful zero-record header avoids enumeration without clearing messages", [origin] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + FetchDiagnosticScope diagnostics; + fetchResult = origin; + py::list messages; + messages.append(py::make_tuple("existing", "preserved")); + calls.clear(); + require(SQLNumResultCols_wrap(statement, messages) == 0, "Column count changed"); + expectCalls({"column_count", "diag_number"}); + require(messages.size() == 1, "Zero-record gate cleared existing messages"); + statement.reset(); + connection->close(); + }); + } + for (int scenario = 0; scenario < 9; ++scenario) { + run("missing unsupported info unknown and nonzero headers retain enumeration", [scenario] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + FetchDiagnosticScope diagnostics; + fetchRecords = {{u"01000", u"PRINT message"}, + {u"01004", u"visible truncation", SQL_SUCCESS_WITH_INFO}}; + switch (scenario) { + case 0: SQLGetDiagField_ptr = nullptr; break; + case 1: numberResult = SQL_ERROR; break; + case 2: numberResult = SQL_SUCCESS_WITH_INFO; break; + case 3: diagnosticCount = -1; break; + case 4: writeDiagnosticCount = false; break; + case 5: diagnosticCount = 2; break; + case 6: numberResult = SQL_NO_DATA; break; + case 7: numberResult = SQL_INVALID_HANDLE; break; + case 8: diagnosticCount = 1; break; // Never cap enumeration at the header. + } + py::list messages; + calls.clear(); + SQLNumResultCols_wrap(statement, messages); + if (scenario == 0) { + expectCalls({"column_count", "diag_record", "diag_record", "diag_record"}); + } else { + expectCalls({"column_count", "diag_number", "diag_record", "diag_record", + "diag_record"}); + } + require(messages.size() == 2, "Fallback lost warning or PRINT records"); + expectMessage(messages, 0, "01000", "PRINT message"); + expectMessage(messages, 1, "01004", "visible truncation"); + statement.reset(); + connection->close(); + }); + } + for (bool empty : {false, true}) { + run("actual SQLFetch NO_DATA preserves return and any messages", [empty] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + FetchDiagnosticScope diagnostics; + if (!empty) { + diagnosticCount = 1; + fetchRecords = {{u"01000", u"final PRINT"}}; + } + py::list rows, messages; + calls.clear(); + auto ret = FetchOne_wrap(statement, rows, "utf-16le", "utf-16le", + SQL_C_WCHAR, messages); + require(ret == SQL_NO_DATA && rows.empty(), "NO_DATA result changed"); + if (empty) { + expectCalls({"statement_unbind", "fetch_no_data", "diag_number"}); + require(messages.empty(), "Empty diagnostics created a message"); + } else { + expectCalls({"statement_unbind", "fetch_no_data", "diag_number", + "diag_record", "diag_record"}); + require(messages.size() == 1, "NO_DATA lost its PRINT record"); + expectMessage(messages, 0, "01000", "final PRINT"); + } + statement.reset(); + connection->close(); + }); + } + for (int scenario = 0; scenario < 4; ++scenario) { + run("LOB continuation filters only internal truncation and retains unrelated warnings", [scenario] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + FetchDiagnosticScope diagnostics; + diagnosticCount = 3; + fetchRecords = {{u"01004", u"internal truncation"}, + {u"01000", u"PRINT message"}, + {u"01S02", u"option changed", SQL_SUCCESS_WITH_INFO}}; + if (scenario == 1) SQLGetDiagField_ptr = nullptr; + if (scenario == 2) stateResult = SQL_ERROR; + if (scenario == 3) stateResult = SQL_SUCCESS_WITH_INFO; + py::list messages; + calls.clear(); + auto value = FetchLobColumnData(statement->get(), 1, SQL_C_WCHAR, + true, false, "utf-16le", messages); + require(value.is_none() && messages.size() == 2, + "LOB continuation lost warnings or exposed internal truncation"); + expectMessage(messages, 0, "01000", "PRINT message"); + expectMessage(messages, 1, "01S02", "option changed"); + require(std::count(calls.begin(), calls.end(), "diag_number") == + (scenario == 1 ? 0 : 1), + "LOB header probe count changed"); + statement.reset(); + connection->close(); + }); + } + run("no probe for success absent messages or explicit diagnostic enumeration", [] { + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + FetchDiagnosticScope diagnostics; + py::list messages; + fetchResult = SQL_SUCCESS; + calls.clear(); + SQLNumResultCols_wrap(statement, messages); + expectCalls({"column_count"}); + fetchResult = SQL_SUCCESS_WITH_INFO; + calls.clear(); + SQLNumResultCols_wrap(statement, {}); + SQLNumResultCols_wrap(statement, py::none()); + expectCalls({"column_count", "column_count"}); + fetchRecords = {{u"01000", u"explicit diagnostic"}}; + calls.clear(); + messages = SQLGetAllDiagRecords(statement); + expectCalls({"diag_record", "diag_record"}); + expectMessage(messages, 0, "01000", "explicit diagnostic"); + statement.reset(); + connection->close(); + }); run("new login is not clean proof; repeated empty leases skip all sanitation", [] { auto connection = acquire(); calls.clear(); @@ -273,7 +518,9 @@ int main() { calls.clear(); connection = acquire(); expectCalls({"alive"}); + calls.clear(); connection->setAutocommit(true); + expectCalls({}); calls.clear(); connection->close(true); expectCalls({}); @@ -281,6 +528,56 @@ int main() { } require(logins == 1, "Empty leases did not reuse the physical connection"); }); + run("new login and repeated setters do not establish clean proof", [] { + auto connection = acquire(); + calls.clear(); + connection->setAutocommit(true); + connection->setAutocommit(true); + expectCalls({"on", "on"}); + connection->close(); + connection = acquire(); + calls.clear(); + connection->setAutocommit(true); + expectCalls({}); + connection->close(); + }); + run("manual mode is never elided; only successful sanitation restores proof", [] { + warm(); + auto connection = acquire(); + calls.clear(); + connection->setAutocommit(false); + connection->setAutocommit(false); + connection->setAutocommit(true); + connection->setAutocommit(true); + expectCalls({"off", "off", "on", "on"}); + connection->close(); + connection = acquire(); + calls.clear(); + connection->setAutocommit(true); + expectCalls({}); + connection->close(); + }); + run("failed setter preserves metadata invalidation and sanitation requirement", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + auto generation = statement->resultMetadata.snapshot().generation; + statement->resultMetadata.publish(generation, std::make_shared()); + calls.clear(); + failNext = "on"; + expectFailure([&] { connection->setAutocommit(true); }); + expectCalls({"on"}); + auto snapshot = statement->resultMetadata.snapshot(); + require(!snapshot.metadata && snapshot.generation == generation + 1, + "Setter did not invalidate metadata before ODBC failure"); + statement.reset(); + calls.clear(); + connection->setAutocommit(true); + expectCalls({"on"}); + calls.clear(); + connection->close(); + expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); + }); run("timeout=30 is applied before login and preserves repeated empty-lease fast path", [] { py::dict attrs; attrs[py::int_(SQL_ATTR_LOGIN_TIMEOUT)] = py::int_(30); @@ -434,6 +731,9 @@ int main() { "Pool sanitation must invalidate result metadata exactly once"); checkParked(); connection = acquire(); + calls.clear(); + connection->setAutocommit(true); + expectCalls({"on"}); startWork(statement); calls.clear(); connection->close(); @@ -506,6 +806,9 @@ int main() { connection->close(); connection = acquire(); calls.clear(); + connection->setAutocommit(true); + expectCalls({"on"}); + calls.clear(); connection->close(); expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); @@ -529,6 +832,9 @@ int main() { connection->close(); connection = acquire(); calls.clear(); + connection->setAutocommit(true); + expectCalls({"on"}); + calls.clear(); connection->close(); expectCalls({"get", "reset", "allocate_statement", "rollback_batch", "free"}); }); @@ -559,6 +865,11 @@ int main() { run("failed native operation invalidates clean proof", [operation] { warm(); auto connection = acquire(); + if (std::string(operation) == "on") { + // A proven ON->ON is deliberately skipped; inject failure + // only after an operation invalidates that proof. + auto statement = connection->allocStatementHandle(); + } failNext = operation; expectFailure([&] { if (std::string(operation) == "commit") { From ac1761a26e5507df0b382583c3a779dcb87c2c3d Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Fri, 9 Oct 2026 11:37:19 +0100 Subject: [PATCH 06/11] TEST: Handle diagnostic count probes and gate native sanitation in CI Teach the mixed-diagnostic interceptor the SQL_DIAG_NUMBER header contract while retaining SQLSTATE and message assertions. Run the existing native sanitation CTest target as a blocking job in standard PR validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/pr-validation-pipeline.yml | 35 +++++++++++++++++ tests/test_fetch_settings_cache.py | 48 ++++++++++++++++++++---- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/eng/pipelines/pr-validation-pipeline.yml b/eng/pipelines/pr-validation-pipeline.yml index 99507c8e6..309c02cf9 100644 --- a/eng/pipelines/pr-validation-pipeline.yml +++ b/eng/pipelines/pr-validation-pipeline.yml @@ -11,6 +11,41 @@ variables: arm64QemuImage: 'tonistiigi/binfmt@sha256:400a4873b838d1b89194d982c45e5fb3cda4593fbfd7e08a02e76b03b21166f0' jobs: +- job: NativePoolSanitation + displayName: 'Native pool sanitation tests' + timeoutInMinutes: 20 + pool: + vmImage: 'ubuntu-latest' + + steps: + - checkout: self + persistCredentials: false + + - task: UsePythonVersion@0 + inputs: + versionSpec: '3.13' + addToPath: true + displayName: 'Use Python 3.13 for native tests' + + - bash: | + set -euo pipefail + sudo apt-get update + sudo apt-get install -y build-essential cmake unixodbc-dev + python -m pip install --no-deps --require-hashes -r eng/requirements-build-linux.txt + displayName: 'Install native test build dependencies' + + - bash: | + set -euo pipefail + cmake -S mssql_python/pybind -B mssql_python/pybind/build/native-tests \ + -DCMAKE_BUILD_TYPE=Release -DENABLE_PROFILING=OFF \ + -DBUILD_NATIVE_POOL_TESTS=ON -DPython3_EXECUTABLE="$(command -v python)" + cmake --build mssql_python/pybind/build/native-tests \ + --config Release --target pool_sanitation_tests --parallel 2 + ctest --test-dir mssql_python/pybind/build/native-tests \ + -C Release --output-on-failure --no-tests=error \ + --timeout 120 -R '^pool_sanitation$' + displayName: 'Build and run native pool sanitation tests' + - job: CodeQLAnalysis displayName: 'CodeQL Security Analysis' pool: diff --git a/tests/test_fetch_settings_cache.py b/tests/test_fetch_settings_cache.py index 78ad5229b..4cec693d3 100644 --- a/tests/test_fetch_settings_cache.py +++ b/tests/test_fetch_settings_cache.py @@ -943,8 +943,8 @@ def _check_native_mixed_fetch_diagnostics(mode, expected_native): ] all_records = [(f"[{state}] ({number})", message) for state, number, message in records] wanted = [all_records[index] for index in (0, 2, 4)] - rec_calls, field_calls, callback_errors = [], [], [] - observed_handles, delegated_records = [], [] + rec_calls, field_calls, header_calls, callback_errors = [], [], [], [] + observed_handles, delegated_records, delegated_headers = [], [], [] phase, target_handle = "observe", None def guarded(callback_type): @@ -994,13 +994,31 @@ def read_record(handle_type, handle, number, state, native_error, message, capac return success @guarded(field_type) - def read_state(handle_type, handle, number, identifier, output, capacity, length): + def read_field(handle_type, handle, number, identifier, output, capacity, length): if phase != "inject" or (handle_type, handle) != target_handle: - if original_read_state is None: + if original_read_field is None: return error - return original_read_state( + result = original_read_field( handle_type, handle, number, identifier, output, capacity, length ) + if identifier == 2 and output: # SQL_DIAG_NUMBER is a SQLINTEGER header field. + count = ctypes.cast(output, ctypes.POINTER(ctypes.c_int32))[0] + delegated_headers.append((handle_type, handle, number, result, count)) + return result + if identifier == 2: + header_calls.append(number) + if ( + not handle + or handle_type != ConstantsDDBC.SQL_HANDLE_STMT.value + or number != 0 + or not output + or capacity != 0 + or length + ): + callback_errors.append("invalid diagnostic-count header lookup") + return error + ctypes.cast(output, ctypes.POINTER(ctypes.c_int32))[0] = len(records) + return success field_calls.append(number) # SQL_DIAG_SQLSTATE uses bytes, including the sixth SQLWCHAR terminator. if ( @@ -1036,7 +1054,7 @@ def read_state(handle_type, handle, number, identifier, output, capacity, length original_rec, original_field = rec_pointer.value, field_pointer.value assert original_rec, "Driver diagnostic records must be available" original_read_record = rec_type(original_rec) - original_read_state = field_type(original_field) if original_field else None + original_read_field = field_type(original_field) if original_field else None try: rec_pointer.value = ctypes.cast(read_record, ctypes.c_void_p).value # Learn this cursor's raw handle while forwarding the diagnostic call unchanged. @@ -1047,7 +1065,7 @@ def read_state(handle_type, handle, number, identifier, output, capacity, length assert target_handle[0] == ConstantsDDBC.SQL_HANDLE_STMT.value and target_handle[1] phase = "inject" field_pointer.value = ( - None if mode == "missing" else ctypes.cast(read_state, ctypes.c_void_p).value + None if mode == "missing" else ctypes.cast(read_field, ctypes.c_void_p).value ) delegated_records.clear() assert mssql_python.ddbc_bindings.DDBCSQLGetAllDiagRecords(other_cursor.hstmt) == [] @@ -1057,6 +1075,7 @@ def read_state(handle_type, handle, number, identifier, output, capacity, length assert tuple(cursor.fetchone()) == (b"x" * 8193,) assert not callback_errors, callback_errors assert cursor.messages == wanted + assert header_calls == ([] if mode == "missing" else [0]) assert field_calls == ([] if mode == "missing" else [1, 2, 3, 4, 5, 6]) if mode == "available": assert rec_calls == [1, 3, 5] @@ -1065,15 +1084,28 @@ def read_state(handle_type, handle, number, identifier, output, capacity, length rec_calls.clear() field_calls.clear() + header_calls.clear() assert mssql_python.ddbc_bindings.DDBCSQLGetAllDiagRecords(cursor.hstmt) == all_records assert not callback_errors, callback_errors assert rec_calls == [1, 2, 3, 4, 5, 6] assert field_calls == [] + assert header_calls == [] delegated_records.clear() + delegated_headers.clear() phase = "delegate" assert cursor.fetchone() is None assert not callback_errors, callback_errors - assert delegated_records == [(*target_handle, 1)] + if mode == "missing": + assert delegated_headers == [] + assert delegated_records == [(*target_handle, 1)] + else: + assert len(delegated_headers) == 1 + handle_type, handle, number, result, count = delegated_headers[0] + assert (handle_type, handle, number) == (*target_handle, 0) + if result == success and count == 0: + assert delegated_records == [] + else: + assert delegated_records == [(*target_handle, 1)] assert cursor.messages == wanted finally: rec_pointer.value, field_pointer.value = original_rec, original_field From 27e2259d0b98da0d49cec3782681e173d4dcde31 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Mon, 12 Oct 2026 09:14:10 +0100 Subject: [PATCH 07/11] TEST: Bounds-check native getInfo fixture without memcpy Replace the test-only raw copy with a validated portable byte copy and cover short, null, and unaligned buffers. No production driver changes or alert suppressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/native/pool_sanitation.cpp | 55 +++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp index 19cab6c43..451421542 100644 --- a/tests/native/pool_sanitation.cpp +++ b/tests/native/pool_sanitation.cpp @@ -6,7 +6,7 @@ #include "logger_bridge.hpp" #include #include -#include +#include #include #include #include @@ -166,10 +166,17 @@ SQLRETURN SQL_API disconnect(SQLHDBC dbc) { } SQLRETURN SQL_API getInfo(SQLHDBC dbc, SQLUSMALLINT, SQLPOINTER value, - SQLSMALLINT, SQLSMALLINT* length) { - *length = sizeof(dbc); + SQLSMALLINT bufferLength, SQLSMALLINT* length) { + const auto required = static_cast(sizeof(dbc)); + if (length) { + *length = required; + } + if (bufferLength < 0 || (value && bufferLength < required)) { + return SQL_ERROR; + } if (value) { - std::memcpy(value, &dbc, sizeof(dbc)); + std::copy_n(reinterpret_cast(&dbc), sizeof(dbc), + static_cast(value)); } return SQL_SUCCESS; } @@ -383,6 +390,46 @@ int main() { std::cout << "PASS " << name << '\n'; }; try { + run("getInfo size queries allow null output and length pointers", [] { + Handle handle{SQL_HANDLE_DBC, nullptr}; + SQLSMALLINT length = 0; + require(getInfo(&handle, SQL_DRIVER_HDBC, nullptr, 0, &length) == SQL_SUCCESS, + "Size query failed"); + require(length == static_cast(sizeof(SQLHDBC)), + "Size query returned the wrong handle size"); + require(getInfo(&handle, SQL_DRIVER_HDBC, nullptr, 0, nullptr) == SQL_SUCCESS, + "Optional length pointer was dereferenced"); + }); + run("getInfo rejects short buffers without writing", [] { + Handle handle{SQL_HANDLE_DBC, nullptr}; + std::array buffer; + for (SQLSMALLINT capacity = -1; + capacity < static_cast(buffer.size()); ++capacity) { + buffer.fill(0xa5); + SQLSMALLINT length = 0; + require(getInfo(&handle, SQL_DRIVER_HDBC, buffer.data(), capacity, &length) == SQL_ERROR, + "Short buffer was accepted"); + require(length == static_cast(sizeof(SQLHDBC)), + "Short buffer did not report the required size"); + require(std::all_of(buffer.begin(), buffer.end(), + [](unsigned char byte) { return byte == 0xa5; }), + "Short buffer was modified"); + } + }); + run("getInfo copies handle bytes into an unaligned exact-size buffer", [] { + Handle handle{SQL_HANDLE_DBC, nullptr}; + SQLHDBC dbc = &handle; + alignas(SQLHDBC) std::array buffer; + buffer.fill(0xa5); + require(getInfo(dbc, SQL_DRIVER_HDBC, buffer.data() + 1, + static_cast(sizeof(dbc)), nullptr) == SQL_SUCCESS, + "Exact-size buffer was rejected"); + require(std::equal(buffer.begin() + 1, buffer.end() - 1, + reinterpret_cast(&dbc)), + "Copied handle representation differs"); + require(buffer.front() == 0xa5 && buffer.back() == 0xa5, + "Copy wrote outside the advertised buffer"); + }); for (SQLRETURN origin : {SQLRETURN(SQL_SUCCESS_WITH_INFO), SQLRETURN(SQL_NO_DATA)}) { run("successful zero-record header avoids enumeration without clearing messages", [origin] { auto connection = acquire(); From 52eb19342347b1d9ff1808cbdc45ef275922d834 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Mon, 12 Oct 2026 12:01:26 +0100 Subject: [PATCH 08/11] FIX: Discard pooled connections after failed child handle cleanup Preserve failed statement frees beyond wrapper destruction and reject affected connections on check-in and checkout. Count child lifetimes through cleanup completion before enabling the clean fast path. Cover orphan reclamation, retained aliases, cleanup races, and pool capacity recovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 31 +++- mssql_python/pybind/ddbc_bindings.cpp | 15 +- mssql_python/pybind/ddbc_bindings.h | 4 + tests/native/pool_sanitation.cpp | 170 +++++++++++++++++- 4 files changed, 209 insertions(+), 11 deletions(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 9a158c9a2..4499dfb8f 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -655,6 +655,12 @@ bool Connection::reset() { if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } + if (_cleanupState->childFreeFailed) { + _poolClean = false; + _poolSessionReset = false; + LOG("Cannot reuse connection after a statement handle failed to free"); + return false; + } if (_poolClean && _poolSessionReset && !_poolProofDisabled) { updateLastUsed(); return true; @@ -702,6 +708,13 @@ bool Connection::reset() { return false; } + // A retained child can finish cleanup while either attribute call releases the GIL. + if (_cleanupState->childFreeFailed) { + _poolClean = false; + _poolSessionReset = false; + LOG("Cannot reuse connection after a statement handle failed to free"); + return false; + } updateLastUsed(); return true; } @@ -711,6 +724,9 @@ void Connection::prepareForPool() { if (!_dbcHandle) { ThrowStdException("Connection handle not allocated"); } + if (_cleanupState->childFreeFailed) { + ThrowStdException("Cannot pool connection after a statement handle failed to free"); + } if (_poolClean && !_poolProofDisabled) { PERF_TIMER("Connection::prepareForPool::clean"); return; @@ -786,17 +802,18 @@ void Connection::prepareForPool() { ThrowStdException(statementError); } checkError(ret); + // A weak_ptr expires before its destructor finishes. Read the count before + // the failure latch so a completed destructor cannot hide a failed native free. + const bool noChildren = _cleanupState->childStatementCount == 0; + if (_cleanupState->childFreeFailed) { + ThrowStdException("Cannot pool connection after a statement handle failed to free"); + } _autocommit = true; _poolSessionReset = sessionReset; updateLastUsed(); // A native statement alias can execute again without another allocation, - // even in a later lease. Only expired wrappers permit the fast path. - if (!_poolProofDisabled) { - std::lock_guard lock(_childHandlesMutex); - _poolClean = std::all_of( - _childStatementHandles.begin(), _childStatementHandles.end(), - [](const std::weak_ptr& handle) { return handle.expired(); }); - } + // even in a later lease. All wrappers must finish cleanup before the fast path. + _poolClean = !_poolProofDisabled && noChildren; } void Connection::updateLastUsed() { diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index ca7ec016e..b536fc2d7 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -1590,12 +1590,19 @@ void DriverLoader::loadDriver() { // SqlHandle definition SqlHandle::SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle, std::shared_ptr cleanupState) - : _type(type), _handle(rawHandle), _cleanupState(std::move(cleanupState)) {} + : _type(type), _handle(rawHandle), _cleanupState(std::move(cleanupState)) { + if (_type == SQL_HANDLE_STMT && _cleanupState) { + ++_cleanupState->childStatementCount; + } +} SqlHandle::~SqlHandle() { if (_handle) { free(); } + if (_type == SQL_HANDLE_STMT && _cleanupState) { + --_cleanupState->childStatementCount; + } } std::unique_lock SqlHandle::lockForCleanup() const { @@ -1660,7 +1667,7 @@ SQLRETURN SqlHandle::freeHandle() { auto freeNative = [this]() -> SQLRETURN { auto cleanupLock = lockForCleanup(); - if (!_handle || !SQLFreeHandle_ptr) { + if (!_handle) { return SQL_INVALID_HANDLE; } describeCache.clear(); @@ -1668,9 +1675,11 @@ SQLRETURN SqlHandle::freeHandle() { _handle = nullptr; return SQL_SUCCESS; } - SQLRETURN ret = SQLFreeHandle_ptr(_type, _handle); + SQLRETURN ret = SQLFreeHandle_ptr ? SQLFreeHandle_ptr(_type, _handle) : SQL_INVALID_HANDLE; if (SQL_SUCCEEDED(ret)) { _handle = nullptr; + } else if (_type == SQL_HANDLE_STMT && _cleanupState) { + _cleanupState->childFreeFailed = true; } return ret; }; diff --git a/mssql_python/pybind/ddbc_bindings.h b/mssql_python/pybind/ddbc_bindings.h index 748ecc5ca..8fc5f9d27 100644 --- a/mssql_python/pybind/ddbc_bindings.h +++ b/mssql_python/pybind/ddbc_bindings.h @@ -13,6 +13,7 @@ #include // Add this line for datetime support #include +#include #include #include #include @@ -291,6 +292,9 @@ struct DescribedParamInfo { struct ConnectionCleanupState { std::mutex mutex; bool disconnected = false; // Protected by mutex, shared with every child. + // Unlike weak_ptr expiration, this count includes destructors still freeing handles. + std::atomic childStatementCount{0}; + std::atomic childFreeFailed{false}; // Sticky for this physical connection. }; class SqlHandle { diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp index 451421542..a866c1044 100644 --- a/tests/native/pool_sanitation.cpp +++ b/tests/native/pool_sanitation.cpp @@ -34,6 +34,17 @@ std::string failNext; int commits = 0; int logins = 0; std::vector loginTimeouts; +std::function duringFree; +std::function duringRollbackBatch; +std::function duringReset; + +struct CallbackScope { + std::function& hook; + CallbackScope(std::function& hook, std::function action) : hook(hook) { + hook = std::move(action); + } + ~CallbackScope() { hook = {}; } +}; void require(bool condition, const std::string& message) { if (!condition) { @@ -62,6 +73,7 @@ SQLRETURN SQL_API allocate(SQLSMALLINT type, SQLHANDLE parent, SQLHANDLE* out) { } SQLRETURN SQL_API freeHandle(SQLSMALLINT, SQLHANDLE handle) { + if (duringFree) duringFree(); auto ret = record("free"); if (SQL_SUCCEEDED(ret)) { handles.erase(handle); @@ -103,6 +115,7 @@ SQLRETURN SQL_API setAttr(SQLHDBC dbc, SQLINTEGER attribute, SQLPOINTER value, S } handle.autocommit = value != nullptr; } else if (attribute == SQL_ATTR_RESET_CONNECTION) { + if (duringReset) duringReset(); handle.resetPending = true; } else if (attribute == SQL_ATTR_LOGIN_TIMEOUT && length == SQL_IS_INTEGER) { handle.loginTimeout = reinterpret_cast(value); @@ -137,6 +150,7 @@ SQLRETURN SQL_API endTran(SQLSMALLINT, SQLHANDLE dbc, SQLSMALLINT completion) { } SQLRETURN SQL_API executeDirect(SQLHSTMT statement, SQLWCHAR*, SQLINTEGER) { + if (duringRollbackBatch) duringRollbackBatch(); auto& handle = *handles.at(handles.at(statement)->parent); if (handle.resetPending) { handle.transaction = handle.pendingWork = false; @@ -794,16 +808,170 @@ int main() { connection->close(); expectCalls({}); }); - run("failed cursor free cannot enable the next lease's fast path", [] { + for (bool fail : {false, true}) { + run("child lifetime count outlives weak pointer expiration", [fail] { + auto cleanup = std::make_shared(); + SQLHANDLE nativeStatement = nullptr; + require(SQL_SUCCEEDED(allocate(SQL_HANDLE_STMT, nullptr, &nativeStatement)), + "Test statement allocation failed"); + auto statement = std::make_shared( + static_cast(SQL_HANDLE_STMT), nativeStatement, cleanup); + std::weak_ptr alias = statement; + bool sawExpiredWithPendingCleanup = false; + { + CallbackScope callback(duringFree, [&] { + sawExpiredWithPendingCleanup = + alias.expired() && cleanup->childStatementCount == 1; + }); + if (fail) failNext = "free"; + statement.reset(); + } + require(sawExpiredWithPendingCleanup, + "Lifetime count did not cover the expired wrapper's destructor"); + require(cleanup->childStatementCount == 0, "Completed destructor retained its count"); + require(cleanup->childFreeFailed == fail, "Destructor lost the native free outcome"); + if (fail) { + require(SQL_SUCCEEDED(freeHandle(SQL_HANDLE_STMT, nativeStatement)), + "Test orphan cleanup failed"); + } + }); + } + run("failed cursor free discards the connection and invalidates surviving aliases", [] { warm(); auto connection = acquire(); auto statement = connection->allocStatementHandle(); startWork(statement); + const auto nativeStatement = statement->get(); + const auto dbc = handles.at(nativeStatement)->parent; failNext = "free"; require(statement->freeHandle() == SQL_ERROR, "Expected failed free"); + calls.clear(); + expectFailure([&] { connection->close(); }); + expectCalls({"off", "rollback", "disconnect", "free"}); + require(handles.count(nativeStatement) == 0 && handles.count(dbc) == 0, + "Discard did not reclaim the failed statement and its connection"); + require(statement->isImplicitlyFreed(), "Surviving alias was not invalidated"); + calls.clear(); + require(statement->freeHandle() == SQL_SUCCESS, "Implicitly freed alias failed cleanup"); + statement.reset(); + expectCalls({}); + require(commits == 0, "Discard committed pending work"); + connection = acquire(); + require(logins == 2, "Discard did not release capacity / replace DBC"); connection->close(); + checkParked(); + }); + for (bool autocommit : {true, false}) { + for (bool explicitFailure : {true, false}) { + run("expired alias after failed destruction cannot return an orphan to the pool", + [autocommit, explicitFailure] { + warm(); + auto connection = acquire(); + connection->setAutocommit(autocommit); + auto statement = connection->allocStatementHandle(); + startWork(statement); + const auto nativeStatement = statement->get(); + const auto dbc = handles.at(nativeStatement)->parent; + std::weak_ptr alias = statement; + if (explicitFailure) { + failNext = "free"; + require(statement->freeHandle() == SQL_ERROR, "Expected explicit free failure"); + } + failNext = "free"; + statement.reset(); + require(alias.expired(), "Wrapper did not expire"); + require(handles.count(nativeStatement) == 1, "Native free unexpectedly succeeded"); + calls.clear(); + expectFailure([&] { connection->close(); }); + expectCalls({"off", "rollback", "disconnect", "free"}); + require(handles.count(nativeStatement) == 0 && handles.count(dbc) == 0, + "Discard did not reclaim the orphan and its connection"); + require(commits == 0, "Orphan cleanup committed pending work"); + connection = acquire(); + require(logins == 2, "Orphan cleanup did not release the size-one pool capacity"); + connection->close(); + connection = acquire(); + calls.clear(); + connection->close(); + expectCalls({}); + require(logins == 2, "Replacement connection did not retain the clean fast path"); + checkParked(); + }); + } + } + run("successful retry cannot clear a previous child-free failure", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + startWork(statement); + failNext = "free"; + require(statement->freeHandle() == SQL_ERROR, "Expected explicit free failure"); + require(statement->freeHandle() == SQL_SUCCESS, "Expected successful retry"); + statement.reset(); + calls.clear(); + expectFailure([&] { connection->close(); }); + expectCalls({"off", "rollback", "disconnect", "free"}); connection = acquire(); + require(logins == 2, "Successful retry cleared the failure latch"); + connection->close(); + checkParked(); + }); + run("child-free failure while parked rejects the connection at checkout", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + const auto nativeStatement = statement->get(); + const auto dbc = handles.at(nativeStatement)->parent; + connection->close(); + failNext = "free"; + statement.reset(); + require(handles.count(nativeStatement) == 1, "Expected orphaned statement"); + connection = acquire(); + require(logins == 2, "Checkout reused a connection with failed child cleanup"); + require(handles.count(nativeStatement) == 0 && handles.count(dbc) == 0, + "Checkout did not reclaim the orphan and its connection"); + connection->close(); + checkParked(); + }); + run("child-free failure during sanitation still discards the connection", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); startWork(statement); + const auto nativeStatement = statement->get(); + const auto dbc = handles.at(nativeStatement)->parent; + { + CallbackScope callback(duringRollbackBatch, [&] { + failNext = "free"; + statement.reset(); + }); + expectFailure([&] { connection->close(); }); + } + require(handles.count(nativeStatement) == 0 && handles.count(dbc) == 0, + "Sanitation hid a failed destructor's orphan"); + require(commits == 0, "Sanitation failure committed pending work"); + connection = acquire(); + require(logins == 2, "Sanitation failure did not release capacity"); + connection->close(); + checkParked(); + }); + run("child-free failure during checkout reset rejects the candidate", [] { + warm(); + auto connection = acquire(); + auto statement = connection->allocStatementHandle(); + const auto nativeStatement = statement->get(); + const auto dbc = handles.at(nativeStatement)->parent; + connection->close(); + { + CallbackScope callback(duringReset, [&] { + failNext = "free"; + statement.reset(); + }); + connection = acquire(); + } + require(logins == 2, "Checkout reset hid a failed child free"); + require(handles.count(nativeStatement) == 0 && handles.count(dbc) == 0, + "Checkout reset did not reclaim the orphan and its connection"); connection->close(); checkParked(); }); From 613b1e842557d72aeb3a585513732315a0b017bd Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Mon, 2 Nov 2026 15:46:49 +0530 Subject: [PATCH 09/11] FIX: Preserve bounded pooled autocommit validation Apply the requested autocommit mode during every dirty pooled checkout so attribute failures stay bounded and trigger replacement recovery. Align the Python boundary regression with the merged native attrs contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 4 +--- tests/test_009_pooling.py | 11 +++++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 671e7e71f..10913a0f4 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -736,9 +736,7 @@ bool Connection::reset(unsigned int timeoutSeconds, bool autocommit) { return false; } } - if (!autocommit) { - setAutocommit(false); - } + setAutocommit(autocommit); { py::gil_scoped_release release; ret = SQLSetConnectAttr_ptr( diff --git a/tests/test_009_pooling.py b/tests/test_009_pooling.py index 68c6fda34..4330729ca 100644 --- a/tests/test_009_pooling.py +++ b/tests/test_009_pooling.py @@ -90,6 +90,7 @@ def test_pooled_positive_login_timeout_reaches_native_attrs(): pooling(enabled=True, max_size=2, idle_timeout=30) native = MagicMock() + native.get_autocommit.return_value = True with patch("mssql_python.connection.ddbc_bindings.Connection", return_value=native) as create: for _ in range(3): connection = connect( @@ -98,15 +99,17 @@ def test_pooled_positive_login_timeout_reaches_native_attrs(): autocommit=True, ) assert create.call_args.args[1] is True - assert create.call_args.args[2] == {ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value: 30} + assert create.call_args.args[2] == { + ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value: 30, + ConstantsDDBC.SQL_ATTR_AUTOCOMMIT.value: ConstantsDDBC.SQL_AUTOCOMMIT_ON.value, + } assert connection.timeout == 0 # Query timeout is independent. connection.close() assert create.call_count == 3 - assert native.set_autocommit.call_count == 3 - native.set_autocommit.assert_called_with(True) + assert native.get_autocommit.call_count == 3 + native.set_autocommit.assert_not_called() assert native.close.call_count == 3 native.close.assert_called_with(rollback_before_disconnect=True) - native.get_autocommit.assert_not_called() native.rollback.assert_not_called() From 6fcc7fb3086bcefcc29054007c5f7374ccc54d63 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Mon, 2 Nov 2026 16:40:35 +0530 Subject: [PATCH 10/11] FIX: Keep bounded checkout probe on clean pooled leases Retain the clean-session sanitation fast path while forcing the requested autocommit attribute under the temporary connection timeout. This preserves stale-socket detection and replacement recovery without restoring rollback/reset work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 10 +++------- mssql_python/pybind/connection/connection.h | 2 +- tests/native/pool_sanitation.cpp | 4 ++-- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 10913a0f4..66e7e25e9 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -323,7 +323,7 @@ void Connection::rollback() { checkError(ret); } -void Connection::setAutocommit(bool enable) { +void Connection::setAutocommit(bool enable, bool force) { PERF_TIMER("Connection::setAutocommit"); if (!enable) { _poolClean = false; @@ -335,7 +335,7 @@ void Connection::setAutocommit(bool enable) { clearResultMetadata(); // Only successful native sanitation with no retained statement/raw handle // aliases proves ON. A new login or _autocommit alone is not proof. - if (enable && _dbcHandle->get() && _poolClean && !_poolProofDisabled) { + if (!force && enable && _dbcHandle->get() && _poolClean && !_poolProofDisabled) { _autocommit = true; return; } @@ -671,10 +671,6 @@ bool Connection::reset(unsigned int timeoutSeconds, bool autocommit) { return false; } const bool sessionClean = _poolClean && _poolSessionReset && !_poolProofDisabled; - if (sessionClean && autocommit) { - updateLastUsed(); - return true; - } clearResultMetadata(); // Login and statement timeouts do not bound connection-attribute calls. // Keep a finite connection timeout through reset and lease configuration; @@ -736,7 +732,7 @@ bool Connection::reset(unsigned int timeoutSeconds, bool autocommit) { return false; } } - setAutocommit(autocommit); + setAutocommit(autocommit, true); { py::gil_scoped_release release; ret = SQLSetConnectAttr_ptr( diff --git a/mssql_python/pybind/connection/connection.h b/mssql_python/pybind/connection/connection.h index 91d1a394f..c02b137d1 100644 --- a/mssql_python/pybind/connection/connection.h +++ b/mssql_python/pybind/connection/connection.h @@ -53,7 +53,7 @@ class Connection { void rollback(); // Enable or disable autocommit mode. - void setAutocommit(bool value); + void setAutocommit(bool value, bool force = false); // Check whether autocommit is enabled. bool getAutocommit() const; diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp index 98b499051..40cb3d9f8 100644 --- a/tests/native/pool_sanitation.cpp +++ b/tests/native/pool_sanitation.cpp @@ -577,7 +577,7 @@ int main() { statement.reset(); connection->close(); }); - run("new login is not clean proof; repeated empty leases skip all sanitation", [] { + run("new login is not clean proof; repeated empty leases skip sanitation", [] { auto connection = acquire(); calls.clear(); connection->close(); @@ -585,7 +585,7 @@ int main() { for (int i = 0; i < 100; ++i) { calls.clear(); connection = acquire(); - expectCalls({"alive"}); + expectCalls({"alive", "on"}); calls.clear(); connection->setAutocommit(true); expectCalls({}); From c156bf62ade1bd2d7d4a7e2f640352ec5aae651d Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Mon, 2 Nov 2026 17:08:43 +0530 Subject: [PATCH 11/11] FIX: Probe clean pooled leases under timeout Use the bounded transaction-isolation setter as the checkout round-trip for proven-clean sessions. This preserves the rollback/reset fast path while detecting unresponsive sockets before reuse. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 26 ++++++++++--------- tests/native/pool_sanitation.cpp | 2 +- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 66e7e25e9..2ac7bd18e 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -719,18 +719,20 @@ bool Connection::reset(unsigned int timeoutSeconds, bool autocommit) { return false; } - // SQL_ATTR_RESET_CONNECTION does not reset transaction isolation. - LOG("Resetting transaction isolation level to READ COMMITTED"); - { - 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)) { - _poolClean = false; - LOG("Failed to reset transaction isolation level (ret=%d). Marking as dead.", ret); - return false; - } + } + + // This both completes a pending session reset and gives proven-clean + // leases a bounded server round-trip before reuse. + LOG("Setting transaction isolation level to READ COMMITTED"); + { + 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)) { + _poolClean = false; + LOG("Failed to set transaction isolation level (ret=%d). Marking as dead.", ret); + return false; } setAutocommit(autocommit, true); { diff --git a/tests/native/pool_sanitation.cpp b/tests/native/pool_sanitation.cpp index 40cb3d9f8..c28ee5c61 100644 --- a/tests/native/pool_sanitation.cpp +++ b/tests/native/pool_sanitation.cpp @@ -585,7 +585,7 @@ int main() { for (int i = 0; i < 100; ++i) { calls.clear(); connection = acquire(); - expectCalls({"alive", "on"}); + expectCalls({"alive", "isolation", "on"}); calls.clear(); connection->setAutocommit(true); expectCalls({});