From f29e195d02628f8333ba73a61b97c077ee927f2e Mon Sep 17 00:00:00 2001 From: "Saurabh Singh (SQL Drivers)" Date: Fri, 14 Aug 2026 15:38:33 -0700 Subject: [PATCH] Wire ODBC connection-pooling attributes Add SQL_ATTR_CONNECTION_DEAD and SQL_ATTR_RESET_CONNECTION support for connection pooling (Stage 2, ADO #47338). - B1: add SQL_ATTR_CONNECTION_DEAD/SQL_CD_TRUE/SQL_CD_FALSE and SQL_ATTR_RESET_CONNECTION/SQL_RESET_CONNECTION_YES constants. - B2: SQLGetConnectAttr(SQL_ATTR_CONNECTION_DEAD) returns a cached, never-probe liveness read; disconnected/never-connected reads DEAD. - B3: SQLSetConnectAttr(SQL_ATTR_RESET_CONNECTION) validates the value (HY024 otherwise), rolls back a live local transaction, arms the full RESETCONNECTION bit via prepare_reset_connection(false), and clears local_tran_started; disconnected surfaces 08003, busy is rejected. - Unit tests cover both attributes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63e24621-7665-4572-a5b8-e5dfa55060a0 --- mssql-odbc/src/api/get_connect_attr.rs | 118 ++++++++++++++++- mssql-odbc/src/api/odbc_types.rs | 18 +++ mssql-odbc/src/api/set_connect_attr.rs | 12 +- mssql-odbc/src/api/txn.rs | 172 ++++++++++++++++++++++++- 4 files changed, 309 insertions(+), 11 deletions(-) diff --git a/mssql-odbc/src/api/get_connect_attr.rs b/mssql-odbc/src/api/get_connect_attr.rs index 04e2cc73..29aa5f27 100644 --- a/mssql-odbc/src/api/get_connect_attr.rs +++ b/mssql-odbc/src/api/get_connect_attr.rs @@ -14,13 +14,15 @@ use tracing::{debug, error}; use super::sqlstate::*; use crate::api::odbc_types::{ - SQL_ATTR_ACCESS_MODE, SQL_ATTR_AUTOCOMMIT, SQL_ATTR_CONNECTION_TIMEOUT, SQL_ATTR_LOGIN_TIMEOUT, - SQL_ATTR_PACKET_SIZE, SQL_ATTR_TXN_ISOLATION, SQL_AUTOCOMMIT_OFF, SQL_AUTOCOMMIT_ON, + SQL_ATTR_ACCESS_MODE, SQL_ATTR_AUTOCOMMIT, SQL_ATTR_CONNECTION_DEAD, + SQL_ATTR_CONNECTION_TIMEOUT, SQL_ATTR_LOGIN_TIMEOUT, SQL_ATTR_PACKET_SIZE, + SQL_ATTR_TXN_ISOLATION, SQL_AUTOCOMMIT_OFF, SQL_AUTOCOMMIT_ON, SQL_CD_FALSE, SQL_CD_TRUE, SQL_COPT_SS_TXN_ISOLATION, SQL_ERROR, SQL_INVALID_HANDLE, SQL_SUCCESS, SqlHandle, SqlInteger, SqlPointer, SqlReturn, }; use crate::api::util::write_if_some; use crate::error::{free_errors, post_sql_error}; +use crate::handles::dbc::ConnectionState; use crate::handles::{DbcHandle, HandleType, handle_from_raw}; /// Login timeout reported when the application has not set @@ -144,6 +146,29 @@ fn sql_get_connect_attr_w_safe( debug!(attribute, value, "SQLGetConnectAttrW: attribute returned"); SQL_SUCCESS } + SQL_ATTR_CONNECTION_DEAD => { + if value_ptr.is_null() { + error!("SQLGetConnectAttrW: SQL_ATTR_CONNECTION_DEAD value pointer is null"); + post_diag(&mut state, ERR_INVALID_NULL_POINTER); + return SQL_ERROR; + } + // Cached, never-probe liveness (D1/D2): report DEAD unless the DBC is + // connected and its client has not been observed dead. Peek the + // client under the DBC mutex — never `take` it (D8) and never touch + // the socket. `SQL_CD_FALSE` means "not known dead", not "proven + // healthy"; a connection that failed silently while idle is + // recovered on the next operation. Disconnected/never-connected + // reads DEAD so a pool discards it. + let alive = state.connection_state == ConnectionState::Connected + && state + .client + .as_ref() + .is_some_and(|client| !client.is_connection_dead()); + let value = if alive { SQL_CD_FALSE } else { SQL_CD_TRUE }; + unsafe { write_if_some(value_ptr as *mut u32, value) }; + debug!(value, "SQLGetConnectAttrW: connection-dead returned"); + SQL_SUCCESS + } // Any other attribute is genuinely unsupported: surface HYC00 instead of // claiming success while leaving the caller's buffer untouched. // `SQL_ATTR_ANSI_APP` lands here deliberately — the Driver Manager sets @@ -168,8 +193,8 @@ fn sql_get_connect_attr_w_safe( mod tests { use super::*; use crate::api::odbc_types::{ - DEFAULT_PACKET_SIZE, SQL_ATTR_ANSI_APP, SQL_MODE_READ_WRITE, SQL_TXN_READ_COMMITTED, - SQL_TXN_SS_SNAPSHOT, + DEFAULT_PACKET_SIZE, SQL_ATTR_ANSI_APP, SQL_ATTR_CONNECTION_DEAD, SQL_CD_FALSE, + SQL_CD_TRUE, SQL_MODE_READ_WRITE, SQL_TXN_READ_COMMITTED, SQL_TXN_SS_SNAPSHOT, }; use crate::api::set_connect_attr::sql_set_connect_attr_w; use crate::test_support::TestHandles; @@ -320,4 +345,89 @@ mod tests { }; assert_eq!(get, SQL_ERROR); } + + #[test] + fn connection_dead_reports_true_when_never_connected() { + // D1: msodbcsql defaults the attribute to DEAD until a token read + // succeeds; a freshly allocated DBC has no client, so a pool must + // discard it. + let h = TestHandles::with_env_dbc(); + let mut out: u32 = 12345; + let get = unsafe { + sql_get_connect_attr_w( + h.dbc, + SQL_ATTR_CONNECTION_DEAD, + &mut out as *mut u32 as SqlPointer, + 0, + std::ptr::null_mut(), + ) + }; + assert_eq!(get, SQL_SUCCESS); + assert_eq!(out, SQL_CD_TRUE); + } + + #[test] + fn connection_dead_reports_true_when_marked_connected_without_client() { + // `Connected` state but no client (mid-operation, taken) still reads + // DEAD: liveness is a property of the client, not the state flag. + let h = TestHandles::with_env_dbc(); + h.mark_dbc_connected(); + let mut out: u32 = 0; + let get = unsafe { + sql_get_connect_attr_w( + h.dbc, + SQL_ATTR_CONNECTION_DEAD, + &mut out as *mut u32 as SqlPointer, + 0, + std::ptr::null_mut(), + ) + }; + assert_eq!(get, SQL_SUCCESS); + assert_eq!(out, SQL_CD_TRUE); + } + + #[test] + fn connection_dead_reports_false_when_connected_and_alive() { + use crate::handles::DbcHandle; + use crate::handles::handle_from_raw; + use mssql_tds::test_client_support::tds_client_from_tokens; + + let h = TestHandles::with_env_dbc(); + h.mark_dbc_connected(); + let dbc = unsafe { handle_from_raw::(h.dbc) }; + // A replay-transport client has not been observed dead, so the cached + // liveness read reports ALIVE. + dbc.inner.lock().unwrap().client = Some(tds_client_from_tokens(vec![])); + + let mut out: u32 = 12345; + let get = unsafe { + sql_get_connect_attr_w( + h.dbc, + SQL_ATTR_CONNECTION_DEAD, + &mut out as *mut u32 as SqlPointer, + 0, + std::ptr::null_mut(), + ) + }; + assert_eq!(get, SQL_SUCCESS); + assert_eq!(out, SQL_CD_FALSE); + + // Clear the client so TestHandles drop does not try to disconnect it. + dbc.inner.lock().unwrap().client = None; + } + + #[test] + fn connection_dead_null_pointer_is_rejected() { + let h = TestHandles::with_env_dbc(); + let get = unsafe { + sql_get_connect_attr_w( + h.dbc, + SQL_ATTR_CONNECTION_DEAD, + std::ptr::null_mut(), + 0, + std::ptr::null_mut(), + ) + }; + assert_eq!(get, SQL_ERROR); + } } diff --git a/mssql-odbc/src/api/odbc_types.rs b/mssql-odbc/src/api/odbc_types.rs index b4e9742e..6a5bcb2c 100644 --- a/mssql-odbc/src/api/odbc_types.rs +++ b/mssql-odbc/src/api/odbc_types.rs @@ -69,6 +69,24 @@ pub const SQL_ATTR_PACKET_SIZE: SqlInteger = 112; pub const SQL_ATTR_CONNECTION_TIMEOUT: SqlInteger = 113; pub const SQL_ATTR_ANSI_APP: SqlInteger = 115; +// Connection-pooling attributes. +// +// `SQL_ATTR_RESET_CONNECTION` is armed by the pool at check-in so the next +// request resets the session to its login defaults; `SQL_ATTR_CONNECTION_DEAD` +// is a read-only liveness flag the pool consults before handing a connection +// out. +pub const SQL_ATTR_RESET_CONNECTION: SqlInteger = 116; +pub const SQL_ATTR_CONNECTION_DEAD: SqlInteger = 1209; + +// `SQL_ATTR_RESET_CONNECTION` value. Only `SQL_RESET_CONNECTION_YES` is defined; +// any other value is HY024. +pub const SQL_RESET_CONNECTION_YES: u32 = 1; + +// `SQL_ATTR_CONNECTION_DEAD` values. msodbcsql reports DEAD until a token read +// succeeds, so disconnected/never-connected reads DEAD. +pub const SQL_CD_TRUE: u32 = 1; +pub const SQL_CD_FALSE: u32 = 0; + // `SQL_ATTR_ACCESS_MODE` values. pub const SQL_MODE_READ_WRITE: u32 = 0; diff --git a/mssql-odbc/src/api/set_connect_attr.rs b/mssql-odbc/src/api/set_connect_attr.rs index e022fe5b..16d251c2 100644 --- a/mssql-odbc/src/api/set_connect_attr.rs +++ b/mssql-odbc/src/api/set_connect_attr.rs @@ -11,12 +11,13 @@ use tracing::{debug, error}; use super::sqlstate::*; -use super::txn::{set_autocommit, set_txn_isolation}; +use super::txn::{reset_connection, set_autocommit, set_txn_isolation}; use crate::api::odbc_types::{ SQL_ATTR_ACCESS_MODE, SQL_ATTR_ANSI_APP, SQL_ATTR_AUTOCOMMIT, SQL_ATTR_CONNECTION_TIMEOUT, - SQL_ATTR_LOGIN_TIMEOUT, SQL_ATTR_PACKET_SIZE, SQL_ATTR_TXN_ISOLATION, SQL_COPT_SS_ACCESS_TOKEN, - SQL_COPT_SS_TXN_ISOLATION, SQL_ERROR, SQL_INVALID_HANDLE, SQL_SUCCESS, SQL_SUCCESS_WITH_INFO, - SqlHandle, SqlInteger, SqlPointer, SqlReturn, + SQL_ATTR_LOGIN_TIMEOUT, SQL_ATTR_PACKET_SIZE, SQL_ATTR_RESET_CONNECTION, + SQL_ATTR_TXN_ISOLATION, SQL_COPT_SS_ACCESS_TOKEN, SQL_COPT_SS_TXN_ISOLATION, SQL_ERROR, + SQL_INVALID_HANDLE, SQL_SUCCESS, SQL_SUCCESS_WITH_INFO, SqlHandle, SqlInteger, SqlPointer, + SqlReturn, }; use crate::error::{free_errors, post_sql_error}; use crate::handles::dbc::ConnectionState; @@ -86,6 +87,9 @@ unsafe fn sql_set_connect_attr_w_impl( SQL_ATTR_TXN_ISOLATION | SQL_COPT_SS_TXN_ISOLATION => { return set_txn_isolation(dbc, value_ptr as usize as u64); } + // Pooling check-in reset: rolls back any live local transaction and arms + // the RESETCONNECTION bit, so it must not run under the DBC mutex either. + SQL_ATTR_RESET_CONNECTION => return reset_connection(dbc, value_ptr as usize as u64), _ => {} } diff --git a/mssql-odbc/src/api/txn.rs b/mssql-odbc/src/api/txn.rs index 5df2d8d4..d1b9f5c5 100644 --- a/mssql-odbc/src/api/txn.rs +++ b/mssql-odbc/src/api/txn.rs @@ -17,9 +17,9 @@ use tracing::{debug, error}; use super::close_cursor::close_cursor_for_connection_op; use super::odbc_types::{ - SQL_AUTOCOMMIT_OFF, SQL_AUTOCOMMIT_ON, SQL_ERROR, SQL_SUCCESS, SQL_SUCCESS_WITH_INFO, - SQL_TXN_READ_COMMITTED, SQL_TXN_READ_UNCOMMITTED, SQL_TXN_REPEATABLE_READ, - SQL_TXN_SERIALIZABLE, SQL_TXN_SS_SNAPSHOT, SqlReturn, + SQL_AUTOCOMMIT_OFF, SQL_AUTOCOMMIT_ON, SQL_ERROR, SQL_RESET_CONNECTION_YES, SQL_SUCCESS, + SQL_SUCCESS_WITH_INFO, SQL_TXN_READ_COMMITTED, SQL_TXN_READ_UNCOMMITTED, + SQL_TXN_REPEATABLE_READ, SQL_TXN_SERIALIZABLE, SQL_TXN_SS_SNAPSHOT, SqlReturn, }; use super::sqlstate::{ ERR_ATTRIBUTE_CANNOT_BE_SET_NOW, ERR_CONNECTION_BUSY, ERR_CONNECTION_DOES_NOT_EXIST, @@ -358,6 +358,73 @@ pub(super) fn set_autocommit(dbc: &DbcHandle, value: u64) -> SqlReturn { } } +/// Applies `SQL_ATTR_RESET_CONNECTION` — the connection-pool check-in reset. +/// +/// Mirrors msodbcsql's `SQL_COPT_SS_RESET_CONNECTION` handler +/// (`sqlcmisc.cpp:2373-2461`): reject any value but `SQL_RESET_CONNECTION_YES` +/// with HY024 (D7), roll back a live local transaction first (D4), then arm the +/// full RESETCONNECTION bit so the next request resets the session to its login +/// defaults. Pool checkout does not preserve transactions, so this never uses +/// RESETCONNECTIONSKIPTRAN. +/// +/// The bit only rides the next request here; making the reset self-acking with +/// its own round trip is a later stage. +pub(super) fn reset_connection(dbc: &DbcHandle, value: u64) -> SqlReturn { + const OP: &str = "SQLSetConnectAttrW(SQL_ATTR_RESET_CONNECTION)"; + + { + let Ok(mut state) = dbc.inner.lock() else { + error!("{OP}: dbc mutex poisoned"); + return SQL_ERROR; + }; + free_errors(&mut state); + if value != u64::from(SQL_RESET_CONNECTION_YES) { + error!(value, "{OP}: invalid value"); + post_diag(&mut state, ERR_INVALID_ATTRIBUTE_VALUE); + return SQL_ERROR; + } + } + + // Claim the idle client: this rejects a busy connection (open cursor / + // `active_stmt`) with ERR_CONNECTION_BUSY and a disconnected DBC with + // ERR_CONNECTION_DOES_NOT_EXIST (08003, D7). + let mut client = match claim_dbc_client(dbc, OP) { + Ok(c) => c, + Err(ret) => return ret, + }; + + // D4: roll back a live local transaction before the reset so the next + // borrower cannot inherit it. Guard on the server actually having one, the + // same way `end_transaction` does — the flag can be stale. + let started = match dbc.inner.lock() { + Ok(state) => state.local_tran_started, + Err(_) => false, + }; + let result = if started && client.has_active_transaction() { + debug!("{OP}: rolling back live local transaction before reset"); + dbc.runtime + .block_on(client.rollback_transaction(None, None)) + } else { + Ok(()) + }; + + client.prepare_reset_connection(false); + release_dbc_client(dbc, client); + + let Ok(mut state) = dbc.inner.lock() else { + error!("{OP}: dbc mutex poisoned"); + return SQL_ERROR; + }; + state.local_tran_started = false; + if let Err(e) = result { + error!(%e, "{OP}: rollback before reset failed"); + post_tds_error(&mut state, &e, SQLSTATE_HY000); + return SQL_ERROR; + } + debug!("{OP}: reset armed for next request"); + SQL_SUCCESS +} + /// Manual-commit → autocommit. Any transaction holding user work is **committed** /// with a `01000` warning; a driver-begun piggyback transaction is rolled back /// silently (`sqlcconn.cpp:3692-3741`). @@ -759,4 +826,103 @@ mod tests { "the no-transaction path must still sweep cursors" ); } + + #[test] + fn reset_connection_rejects_non_yes_value() { + // D7: only SQL_RESET_CONNECTION_YES(1) is valid; anything else is HY024. + // Value validation runs before the connection is claimed. + use crate::api::odbc_types::SQL_ATTR_RESET_CONNECTION; + use crate::api::set_connect_attr::sql_set_connect_attr_w; + use crate::api::sqlstate::SQLSTATE_HY024; + use crate::error::HasDiagnostics; + use crate::test_support::TestHandles; + + let h = TestHandles::with_env_dbc(); + let ret = + unsafe { sql_set_connect_attr_w(h.dbc, SQL_ATTR_RESET_CONNECTION, 2usize as _, 0) }; + assert_eq!(ret, SQL_ERROR); + let dbc = unsafe { handle_from_raw::(h.dbc) }; + assert_eq!( + dbc.inner.lock().unwrap().diag_records()[0].sql_state, + SQLSTATE_HY024 + ); + } + + #[test] + fn reset_connection_on_disconnected_dbc_is_08003() { + // D7: reset on a connection that does not exist surfaces 08003, the + // diagnostic `claim_dbc_client` posts for the disconnected case. + use crate::api::odbc_types::{SQL_ATTR_RESET_CONNECTION, SQL_RESET_CONNECTION_YES}; + use crate::api::set_connect_attr::sql_set_connect_attr_w; + use crate::api::sqlstate::SQLSTATE_08003; + use crate::error::HasDiagnostics; + use crate::test_support::TestHandles; + + let h = TestHandles::with_env_dbc(); + let ret = unsafe { + sql_set_connect_attr_w( + h.dbc, + SQL_ATTR_RESET_CONNECTION, + SQL_RESET_CONNECTION_YES as usize as _, + 0, + ) + }; + assert_eq!(ret, SQL_ERROR); + let dbc = unsafe { handle_from_raw::(h.dbc) }; + assert_eq!( + dbc.inner.lock().unwrap().diag_records()[0].sql_state, + SQLSTATE_08003 + ); + } + + #[test] + fn reset_connection_rejects_busy_connection() { + // An open cursor / in-progress result set pins `active_stmt`; the reset + // must not touch a busy connection. + use std::ffi::c_void; + + use crate::error::HasDiagnostics; + use crate::test_support::TestHandles; + use mssql_tds::test_client_support::tds_client_from_tokens; + + let h = TestHandles::with_env_dbc(); + h.mark_dbc_connected(); + let dbc = unsafe { handle_from_raw::(h.dbc) }; + { + let mut state = dbc.inner.lock().unwrap(); + state.client = Some(tds_client_from_tokens(vec![])); + state.active_stmt = Some(std::ptr::dangling_mut::()); + } + + assert_eq!(reset_connection(dbc, 1), SQL_ERROR); + let state = dbc.inner.lock().unwrap(); + assert_eq!(state.diag_records()[0].sql_state, SQLSTATE_HY000); + // The busy connection was rejected without taking the client. + assert!(state.client.is_some()); + } + + #[test] + fn reset_connection_arms_and_clears_local_tran() { + // A successful reset leaves the idle client in place and clears the + // driver-side transaction flag (D4). The RESETCONNECTION bit riding the + // next request is covered by the mssql-tds transport test. + use crate::error::HasDiagnostics; + use crate::test_support::TestHandles; + use mssql_tds::test_client_support::tds_client_from_tokens; + + let h = TestHandles::with_env_dbc(); + h.mark_dbc_connected(); + let dbc = unsafe { handle_from_raw::(h.dbc) }; + { + let mut state = dbc.inner.lock().unwrap(); + state.client = Some(tds_client_from_tokens(vec![])); + state.local_tran_started = true; + } + + assert_eq!(reset_connection(dbc, 1), SQL_SUCCESS); + let state = dbc.inner.lock().unwrap(); + assert!(!state.local_tran_started); + assert!(state.client.is_some()); + assert!(state.diag_records().is_empty()); + } }