diff --git a/mssql_python/connection.py b/mssql_python/connection.py index 448b3a95..cc372363 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -1623,6 +1623,14 @@ def __del__(self) -> None: This is a safety net to ensure resources are cleaned up even if close() was not called explicitly. """ + import sys + + # During interpreter shutdown, ODBC handles may already be invalid. + # Attempting to free them can cause segfaults (SIGSEGV). + # The OS will reclaim all resources when the process exits. + if sys is None or sys._is_finalizing(): + return + if "_closed" not in self.__dict__ or not self._closed: try: self.close() diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index d2e11edf..403e36b1 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -3021,17 +3021,19 @@ def __del__(self): even if close() was not called explicitly. If the cursor is already closed, it will not raise an exception during cleanup. """ + import sys + + # During interpreter shutdown, ODBC handles may already be invalid. + # Attempting to free them can cause segfaults (SIGSEGV). + # The OS will reclaim all resources when the process exits. + if sys is None or sys._is_finalizing(): + return + if "closed" not in self.__dict__ or not self.closed: try: self.close() except Exception as e: # pylint: disable=broad-exception-caught # Don't raise an exception in __del__, just log it - # If interpreter is shutting down, we might not have logging set up - import sys - - if sys and sys._is_finalizing(): - # Suppress logging during interpreter shutdown - return logger.debug("Exception during cursor cleanup in __del__: %s", e) def scroll( diff --git a/mssql_python/pooling.py b/mssql_python/pooling.py index a2811d9f..e9e2d465 100644 --- a/mssql_python/pooling.py +++ b/mssql_python/pooling.py @@ -45,10 +45,6 @@ def enable(cls, max_size: int = 100, idle_timeout: int = 600) -> None: idle_timeout, ) with cls._lock: - if cls._enabled: - logger.debug("PoolingManager.enable: Pooling already enabled, skipping") - return - if max_size <= 0 or idle_timeout < 0: logger.error( "PoolingManager.enable: Invalid parameters - max_size=%d, idle_timeout=%d", @@ -57,16 +53,34 @@ def enable(cls, max_size: int = 100, idle_timeout: int = 600) -> None: ) raise ValueError("Invalid pooling parameters") - logger.info( - "PoolingManager.enable: Enabling connection pooling - max_size=%d, idle_timeout=%d seconds", - max_size, - idle_timeout, - ) + if cls._enabled: + # Already enabled — reconfigure if parameters changed + if ( + cls._config["max_size"] == max_size + and cls._config["idle_timeout"] == idle_timeout + ): + logger.debug( + "PoolingManager.enable: Pooling already enabled with same config, skipping" + ) + return + logger.info( + "PoolingManager.enable: Reconfiguring pooling - max_size=%d, idle_timeout=%d seconds", + max_size, + idle_timeout, + ) + else: + logger.info( + "PoolingManager.enable: Enabling connection pooling - max_size=%d, idle_timeout=%d seconds", + max_size, + idle_timeout, + ) + ddbc_bindings.enable_pooling(max_size, idle_timeout) cls._config["max_size"] = max_size cls._config["idle_timeout"] = idle_timeout cls._enabled = True cls._initialized = True + cls._pools_closed = False logger.info("PoolingManager.enable: Connection pooling enabled successfully") @classmethod diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 32ed5507..3603bb54 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -51,7 +51,7 @@ Connection::Connection(const std::wstring& conn_str, bool use_pool) } Connection::~Connection() { - disconnect(); // fallback if user forgets to disconnect + disconnect_nothrow(); // fallback if user forgets to disconnect } // Allocates connection handle @@ -91,7 +91,7 @@ void Connection::connect(const py::dict& attrs_before) { updateLastUsed(); } -void Connection::disconnect() { +SQLRETURN Connection::disconnect_impl() noexcept { if (_dbcHandle) { LOG("Disconnecting from database"); @@ -99,23 +99,22 @@ void Connection::disconnect() { // 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::lock_guard lock(_childHandlesMutex); - + // First compact: remove expired weak_ptrs (they're already destroyed) size_t originalSize = _childStatementHandles.size(); _childStatementHandles.erase( std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(), [](const std::weak_ptr& wp) { return wp.expired(); }), _childStatementHandles.end()); - - LOG("Compacted child handles: %zu -> %zu (removed %zu expired)", - originalSize, _childStatementHandles.size(), - originalSize - _childStatementHandles.size()); - + + LOG("Compacted child handles: %zu -> %zu (removed %zu expired)", originalSize, + _childStatementHandles.size(), originalSize - _childStatementHandles.size()); + LOG("Marking %zu child statement handles as implicitly freed", _childStatementHandles.size()); for (auto& weakHandle : _childStatementHandles) { @@ -124,8 +123,10 @@ void Connection::disconnect() { // 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) { - LOG_ERROR("CRITICAL: Non-STMT handle (type=%d) found in _childStatementHandles. " - "This will cause a handle leak!", handle->type()); + LOG_ERROR( + "CRITICAL: Non-STMT handle (type=%d) found in _childStatementHandles. " + "This will cause a handle leak!", + handle->type()); continue; // Skip marking to prevent leak } handle->markImplicitlyFreed(); @@ -136,11 +137,30 @@ void Connection::disconnect() { } // Release lock before potentially slow SQLDisconnect call SQLRETURN ret = SQLDisconnect_ptr(_dbcHandle->get()); - checkError(ret); - // triggers SQLFreeHandle via destructor, if last owner + // Always free the handle regardless of SQLDisconnect result _dbcHandle.reset(); + return ret; } else { LOG("No connection handle to disconnect"); + return SQL_SUCCESS; + } +} + +void Connection::disconnect() { + SQLRETURN ret = disconnect_impl(); + if (!SQL_SUCCEEDED(ret)) { + // For user-facing disconnect, report the error. + // The handle is already freed by disconnect_impl(), so build the + // message from the saved return code. + std::string msg = "SQLDisconnect failed with return code " + std::to_string(ret); + ThrowStdException(msg.c_str()); + } +} + +void Connection::disconnect_nothrow() noexcept { + SQLRETURN ret = disconnect_impl(); + if (!SQL_SUCCEEDED(ret)) { + LOG_ERROR("SQLDisconnect failed (ret=%d), error suppressed (nothrow path)", ret); } } @@ -221,7 +241,7 @@ SqlHandlePtr Connection::allocStatementHandle() { // or GC finalizers running from different threads { std::lock_guard lock(_childHandlesMutex); - + // Track this child handle so we can mark it as implicitly freed when connection closes // Use weak_ptr to avoid circular references and allow normal cleanup _childStatementHandles.push_back(stmtHandle); @@ -237,9 +257,8 @@ SqlHandlePtr Connection::allocStatementHandle() { [](const std::weak_ptr& wp) { return wp.expired(); }), _childStatementHandles.end()); _allocationsSinceCompaction = 0; - LOG("Periodic compaction: %zu -> %zu handles (removed %zu expired)", - originalSize, _childStatementHandles.size(), - originalSize - _childStatementHandles.size()); + LOG("Periodic compaction: %zu -> %zu handles (removed %zu expired)", originalSize, + _childStatementHandles.size(), originalSize - _childStatementHandles.size()); } } // Release lock @@ -375,7 +394,7 @@ bool Connection::reset() { (SQLPOINTER)SQL_RESET_CONNECTION_YES, SQL_IS_INTEGER); if (!SQL_SUCCEEDED(ret)) { LOG("Failed to reset connection (ret=%d). Marking as dead.", ret); - disconnect(); + disconnect_nothrow(); return false; } @@ -387,7 +406,7 @@ bool Connection::reset() { (SQLPOINTER)SQL_TXN_READ_COMMITTED, SQL_IS_INTEGER); if (!SQL_SUCCEEDED(ret)) { LOG("Failed to reset transaction isolation level (ret=%d). Marking as dead.", ret); - disconnect(); + disconnect_nothrow(); return false; } diff --git a/mssql_python/pybind/connection/connection.h b/mssql_python/pybind/connection/connection.h index 6c6f1e63..58f17960 100644 --- a/mssql_python/pybind/connection/connection.h +++ b/mssql_python/pybind/connection/connection.h @@ -4,8 +4,8 @@ #pragma once #include "../ddbc_bindings.h" #include -#include #include +#include // Represents a single ODBC database connection. // Manages connection handles. @@ -29,8 +29,12 @@ class Connection { void connect(const py::dict& attrs_before = py::dict()); // Disconnect and free the connection handle. + // Throws on SQLDisconnect failure (use for user-facing close()). void disconnect(); + // Disconnect without throwing — safe for destructors, pool cleanup, and reset() failure paths. + void disconnect_nothrow() noexcept; + // Commit the current transaction. void commit(); @@ -63,6 +67,10 @@ class Connection { void checkError(SQLRETURN ret) const; void applyAttrsBefore(const py::dict& attrs_before); + // Shared disconnect logic: marks child handles, calls SQLDisconnect, frees handle. + // Returns the SQLRETURN from SQLDisconnect (or SQL_SUCCESS if no handle). + SQLRETURN disconnect_impl() noexcept; + std::wstring _connStr; bool _fromPool = false; bool _autocommit = true; @@ -75,13 +83,13 @@ class Connection { // Uses weak_ptr to avoid circular references and allow normal cleanup // THREAD-SAFETY: All accesses must be guarded by _childHandlesMutex std::vector> _childStatementHandles; - + // Counter for periodic compaction of expired weak_ptrs // Compact every N allocations to avoid O(n²) overhead in hot path // THREAD-SAFETY: Protected by _childHandlesMutex size_t _allocationsSinceCompaction = 0; static constexpr size_t COMPACTION_INTERVAL = 100; - + // Mutex protecting _childStatementHandles and _allocationsSinceCompaction // Prevents data races between allocStatementHandle() and disconnect(), // or concurrent GC finalizers running from different threads diff --git a/mssql_python/pybind/connection/connection_pool.cpp b/mssql_python/pybind/connection/connection_pool.cpp index 3000a970..bce91f21 100644 --- a/mssql_python/pybind/connection/connection_pool.cpp +++ b/mssql_python/pybind/connection/connection_pool.cpp @@ -21,6 +21,9 @@ std::shared_ptr ConnectionPool::acquire(const std::wstring& connStr, auto now = std::chrono::steady_clock::now(); size_t before = _pool.size(); + LOG("ConnectionPool::acquire: pool_size=%zu, max_size=%zu, idle_timeout=%d", before, + _max_size, _idle_timeout_secs); + // Phase 1: Remove stale connections, collect for later disconnect _pool.erase(std::remove_if(_pool.begin(), _pool.end(), [&](const std::shared_ptr& conn) { @@ -69,11 +72,7 @@ std::shared_ptr ConnectionPool::acquire(const std::wstring& connStr, // Phase 3: Disconnect expired/bad connections outside lock for (auto& conn : to_disconnect) { - try { - conn->disconnect(); - } catch (const std::exception& ex) { - LOG("Disconnect bad/expired connections failed: %s", ex.what()); - } + conn->disconnect_nothrow(); } return valid_conn; } @@ -84,7 +83,7 @@ void ConnectionPool::release(std::shared_ptr conn) { conn->updateLastUsed(); _pool.push_back(conn); } else { - conn->disconnect(); + conn->disconnect_nothrow(); if (_current_size > 0) --_current_size; } @@ -101,11 +100,7 @@ void ConnectionPool::close() { _current_size = 0; } for (auto& conn : to_close) { - try { - conn->disconnect(); - } catch (const std::exception& ex) { - LOG("ConnectionPool::close: disconnect failed: %s", ex.what()); - } + conn->disconnect_nothrow(); } } diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index ee548319..46ec50fa 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -5683,10 +5683,8 @@ SQLLEN SQLRowCount_wrap(SqlHandlePtr StatementHandle) { return rowCount; } -static std::once_flag pooling_init_flag; void enable_pooling(int maxSize, int idleTimeout) { - std::call_once(pooling_init_flag, - [&]() { ConnectionPoolManager::getInstance().configure(maxSize, idleTimeout); }); + ConnectionPoolManager::getInstance().configure(maxSize, idleTimeout); } // Thread-safe decimal separator setting diff --git a/tests/test_009_pooling.py b/tests/test_009_pooling.py index 1a3e5f09..98508674 100644 --- a/tests/test_009_pooling.py +++ b/tests/test_009_pooling.py @@ -104,20 +104,16 @@ def test_connection_pooling_isolation_level_reset(conn_str): # Set isolation level to SERIALIZABLE (non-default) conn1.set_attr(mssql_python.SQL_ATTR_TXN_ISOLATION, mssql_python.SQL_TXN_SERIALIZABLE) - # Verify the isolation level was set + # Verify the isolation level was set (use DBCC USEROPTIONS to avoid + # requiring VIEW SERVER PERFORMANCE STATE permission for sys.dm_exec_sessions) cursor1 = conn1.cursor() - cursor1.execute( - "SELECT CASE transaction_isolation_level " - "WHEN 0 THEN 'Unspecified' " - "WHEN 1 THEN 'ReadUncommitted' " - "WHEN 2 THEN 'ReadCommitted' " - "WHEN 3 THEN 'RepeatableRead' " - "WHEN 4 THEN 'Serializable' " - "WHEN 5 THEN 'Snapshot' END AS isolation_level " - "FROM sys.dm_exec_sessions WHERE session_id = @@SPID" - ) - isolation_level_1 = cursor1.fetchone()[0] - assert isolation_level_1 == "Serializable", f"Expected Serializable, got {isolation_level_1}" + cursor1.execute("DBCC USEROPTIONS WITH NO_INFOMSGS") + isolation_level_1 = None + for row in cursor1.fetchall(): + if row[0] == "isolation level": + isolation_level_1 = row[1] + break + assert isolation_level_1 == "serializable", f"Expected serializable, got {isolation_level_1}" # Get SPID for verification of connection reuse cursor1.execute("SELECT @@SPID") @@ -138,24 +134,20 @@ def test_connection_pooling_isolation_level_reset(conn_str): # Verify connection was reused assert spid1 == spid2, "Connection was not reused from pool" - # Check if isolation level is reset to default - cursor2.execute( - "SELECT CASE transaction_isolation_level " - "WHEN 0 THEN 'Unspecified' " - "WHEN 1 THEN 'ReadUncommitted' " - "WHEN 2 THEN 'ReadCommitted' " - "WHEN 3 THEN 'RepeatableRead' " - "WHEN 4 THEN 'Serializable' " - "WHEN 5 THEN 'Snapshot' END AS isolation_level " - "FROM sys.dm_exec_sessions WHERE session_id = @@SPID" - ) - isolation_level_2 = cursor2.fetchone()[0] + # Check if isolation level is reset to default (use DBCC USEROPTIONS to avoid + # requiring VIEW SERVER PERFORMANCE STATE permission for sys.dm_exec_sessions) + cursor2.execute("DBCC USEROPTIONS WITH NO_INFOMSGS") + isolation_level_2 = None + for row in cursor2.fetchall(): + if row[0] == "isolation level": + isolation_level_2 = row[1] + break # Verify isolation level is reset to default (READ COMMITTED) # This is the CORRECT behavior for connection pooling - we should reset # session state to prevent settings from one usage affecting the next - assert isolation_level_2 == "ReadCommitted", ( - f"Isolation level was not reset! Expected 'ReadCommitted', got '{isolation_level_2}'. " + assert isolation_level_2 == "read committed", ( + f"Isolation level was not reset! Expected 'read committed', got '{isolation_level_2}'. " f"This indicates session state leaked from the previous connection usage." ) @@ -278,30 +270,28 @@ def try_overflow(): c.close() -@pytest.mark.skip("Flaky test - idle timeout behavior needs investigation") def test_pool_idle_timeout_removes_connections(conn_str): """Test that idle_timeout removes connections from the pool after the timeout.""" pooling(max_size=2, idle_timeout=1) conn1 = connect(conn_str) - spid_list = [] cursor1 = conn1.cursor() + # Use @@SPID to identify the connection without requiring + # VIEW SERVER PERFORMANCE STATE permission for sys.dm_exec_connections. cursor1.execute("SELECT @@SPID") spid1 = cursor1.fetchone()[0] - spid_list.append(spid1) conn1.close() - # Wait for longer than idle_timeout - time.sleep(3) + # Wait well beyond the idle_timeout to account for slow CI and integer-second granularity + time.sleep(5) - # Get a new connection, which should not reuse the previous SPID + # Get a new connection — the idle one should have been evicted during acquire() conn2 = connect(conn_str) cursor2 = conn2.cursor() cursor2.execute("SELECT @@SPID") spid2 = cursor2.fetchone()[0] - spid_list.append(spid2) conn2.close() - assert spid1 != spid2, "Idle timeout did not remove connection from pool" + assert spid1 != spid2, "Idle timeout did not remove connection from pool — same SPID reused" # ============================================================================= @@ -309,51 +299,63 @@ def test_pool_idle_timeout_removes_connections(conn_str): # ============================================================================= -@pytest.mark.skip( - "Test causes fatal crash - forcibly closing underlying connection leads to undefined behavior" -) def test_pool_removes_invalid_connections(conn_str): - """Test that the pool removes connections that become invalid (simulate by closing underlying connection).""" + """Test that the pool removes connections that become invalid and recovers gracefully. + + This test simulates a connection being returned to the pool in a dirty state + (with an open transaction) by calling _conn.close() directly, bypassing the + normal Python close() which does a rollback. The pool's acquire() should detect + the bad connection during reset(), discard it, and create a fresh one. + """ pooling(max_size=1, idle_timeout=30) conn = connect(conn_str) cursor = conn.cursor() cursor.execute("SELECT 1") - # Simulate invalidation by forcibly closing the connection at the driver level - try: - # Try to access a private attribute or method to forcibly close the underlying connection - # This is implementation-specific; if not possible, skip - if hasattr(conn, "_conn") and hasattr(conn._conn, "close"): - conn._conn.close() - else: - pytest.skip("Cannot forcibly close underlying connection for this driver") - except Exception: - pass - # Safely close the connection, ignoring errors due to forced invalidation + cursor.fetchone() + + # Record the SPID of the original connection (avoids requiring + # VIEW SERVER PERFORMANCE STATE permission for sys.dm_exec_connections) + cursor.execute("SELECT @@SPID") + original_spid = cursor.fetchone()[0] + + # Force-return the connection to the pool WITHOUT rollback. + # This leaves the pooled connection in a dirty state (open implicit transaction) + # which will cause reset() to fail on next acquire(). + conn._conn.close() + + # Python close() will fail since the underlying handle is already gone try: conn.close() - except RuntimeError as e: - if "not initialized" not in str(e): - raise - # Now, get a new connection from the pool and ensure it works + except RuntimeError: + pass + + # Now get a new connection — the pool should discard the dirty one and create fresh new_conn = connect(conn_str) new_cursor = new_conn.cursor() - try: - new_cursor.execute("SELECT 1") - result = new_cursor.fetchone() - assert result is not None and result[0] == 1, "Pool did not remove invalid connection" - finally: - new_conn.close() + new_cursor.execute("SELECT 1") + result = new_cursor.fetchone() + assert result is not None and result[0] == 1, "Pool did not recover from invalid connection" + + # Verify it's a different physical connection + new_cursor.execute("SELECT @@SPID") + new_spid = new_cursor.fetchone()[0] + assert ( + original_spid != new_spid + ), "Expected a new physical connection after pool discarded the dirty one" + + new_conn.close() 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) # First, try to connect with a bad password (should fail) - if "Pwd=" in conn_str: - bad_conn_str = conn_str.replace("Pwd=", "Pwd=wrongpassword") - elif "Password=" in conn_str: - bad_conn_str = conn_str.replace("Password=", "Password=wrongpassword") - else: + import re + + # Replace the value of the first Pwd/Password key-value pair with "wrongpassword" + pattern = re.compile(r"(?i)((?:Pwd|Password)\s*=\s*)([^;]*)") + bad_conn_str, num_subs = pattern.subn(lambda m: m.group(1) + "wrongpassword", conn_str, count=1) + if num_subs == 0: pytest.skip("No password found in connection string to modify") with pytest.raises(Exception): connect(bad_conn_str) @@ -515,3 +517,51 @@ def test_pooling_state_consistency(conn_str): assert PoolingManager.is_initialized(), "Should remain initialized after disable call" print("Pooling state consistency verified") + + +def test_pooling_reconfigure_while_enabled(conn_str): + """Test that calling pooling() with new parameters reconfigures the pool without disable/enable.""" + pooling(max_size=50, idle_timeout=600) + assert PoolingManager.is_enabled(), "Pooling should be enabled" + assert PoolingManager._config["max_size"] == 50 + + # Reconfigure with smaller pool — should take effect immediately + pooling(max_size=10, idle_timeout=300) + assert PoolingManager.is_enabled(), "Pooling should still be enabled after reconfigure" + assert ( + PoolingManager._config["max_size"] == 10 + ), f"max_size not updated: expected 10, got {PoolingManager._config['max_size']}" + assert ( + PoolingManager._config["idle_timeout"] == 300 + ), f"idle_timeout not updated: expected 300, got {PoolingManager._config['idle_timeout']}" + + # Verify connections still work after reconfiguration + conn = connect(conn_str) + cursor = conn.cursor() + cursor.execute("SELECT 1") + assert cursor.fetchone()[0] == 1 + conn.close() + + +def test_pooling_disable_enable_cycle_state(conn_str): + """Test that disable >> enable properly resets _pools_closed so a second disable cleans up.""" + pooling(max_size=5, idle_timeout=30) + conn = connect(conn_str) + conn.close() + + # Disable — sets _pools_closed = True + pooling(enabled=False) + assert not PoolingManager.is_enabled() + + # Re-enable — should reset _pools_closed so future disable works + pooling(max_size=5, idle_timeout=30) + assert PoolingManager.is_enabled() + assert not PoolingManager._pools_closed, "_pools_closed should be False after re-enable" + + conn = connect(conn_str) + conn.close() + + # Second disable should actually call close_pooling (not skip it) + pooling(enabled=False) + assert not PoolingManager.is_enabled() + assert PoolingManager._pools_closed