From 84d8c2cddfa6d5612cb8a28fbc640e52cc1da6b9 Mon Sep 17 00:00:00 2001 From: Sumit Sarabhai Date: Fri, 25 Sep 2026 17:29:16 +0100 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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: