From e1e0108b1f5c43d88f787e96516c5e3198f43406 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Thu, 23 Jul 2020 16:07:41 -0700 Subject: [PATCH] Dropped the use of LOCK TIMEOUT (#1165) --- source/pdo_sqlsrv/pdo_stmt.cpp | 9 --- source/pdo_sqlsrv/php_pdo_sqlsrv_int.h | 3 - source/shared/core_sqlsrv.h | 4 +- source/shared/core_stmt.cpp | 9 +++ source/sqlsrv/php_sqlsrv_int.h | 3 - source/sqlsrv/stmt.cpp | 23 ------ .../pdo_1100_query_timeout_disconnect.phpt | 72 +++++++++++++++++++ test/functional/sqlsrv/test_timeout.phpt | 11 +-- 8 files changed, 90 insertions(+), 44 deletions(-) create mode 100644 test/functional/pdo_sqlsrv/pdo_1100_query_timeout_disconnect.phpt diff --git a/source/pdo_sqlsrv/pdo_stmt.cpp b/source/pdo_sqlsrv/pdo_stmt.cpp index 062c43e5..2c4fc16d 100644 --- a/source/pdo_sqlsrv/pdo_stmt.cpp +++ b/source/pdo_sqlsrv/pdo_stmt.cpp @@ -1523,12 +1523,3 @@ sqlsrv_phptype pdo_sqlsrv_stmt::sql_type_to_php_type( _In_ SQLINTEGER sql_type, return sqlsrv_phptype; } - -void pdo_sqlsrv_stmt::set_query_timeout() -{ - if (query_timeout == QUERY_TIMEOUT_INVALID || query_timeout < 0) { - return; - } - - core::SQLSetStmtAttr(this, SQL_ATTR_QUERY_TIMEOUT, reinterpret_cast((SQLLEN)query_timeout), SQL_IS_UINTEGER); -} \ No newline at end of file diff --git a/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h b/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h index e81cf09d..7d377496 100644 --- a/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h +++ b/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h @@ -260,9 +260,6 @@ struct pdo_sqlsrv_stmt : public sqlsrv_stmt { // for PDO, everything is a string, so we return SQLSRV_PHPTYPE_STRING for all SQL types virtual sqlsrv_phptype sql_type_to_php_type( _In_ SQLINTEGER sql_type, _In_ SQLUINTEGER size, _In_ bool prefer_string_to_stream ); - // driver specific way to set query timeout - virtual void set_query_timeout(); - bool direct_query; // flag set if the query should be executed directly or prepared const char* direct_query_subst_string; // if the query is direct, hold the substitution string if using named parameters size_t direct_query_subst_string_len; // length of query string used for direct queries diff --git a/source/shared/core_sqlsrv.h b/source/shared/core_sqlsrv.h index 0b55a523..f1cd5e3d 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -1520,6 +1520,8 @@ struct sqlsrv_stmt : public sqlsrv_context { // free sensitivity classification metadata void clean_up_sensitivity_metadata(); + // set query timeout + void set_query_timeout(); sqlsrv_conn* conn; // Connection that created this statement @@ -1571,8 +1573,6 @@ struct sqlsrv_stmt : public sqlsrv_context { // driver specific conversion rules from a SQL Server/ODBC type to one of the SQLSRV_PHPTYPE_* constants virtual sqlsrv_phptype sql_type_to_php_type( _In_ SQLINTEGER sql_type, _In_ SQLUINTEGER size, _In_ bool prefer_string_to_stream ) = 0; - // driver specific way to set query timeout - virtual void set_query_timeout() = 0; }; // *** field metadata struct *** diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index f7629a91..c4eb5cba 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -275,6 +275,15 @@ void sqlsrv_stmt::clean_up_sensitivity_metadata() } } +void sqlsrv_stmt::set_query_timeout() +{ + if (query_timeout == QUERY_TIMEOUT_INVALID || query_timeout < 0) { + return; + } + + core::SQLSetStmtAttr(this, SQL_ATTR_QUERY_TIMEOUT, reinterpret_cast((SQLLEN)query_timeout), SQL_IS_UINTEGER); +} + // core_sqlsrv_create_stmt // Common code to allocate a statement from either driver. Returns a valid driver statement object or // throws an exception if an error occurs. diff --git a/source/sqlsrv/php_sqlsrv_int.h b/source/sqlsrv/php_sqlsrv_int.h index 43f91f2e..e04bceeb 100644 --- a/source/sqlsrv/php_sqlsrv_int.h +++ b/source/sqlsrv/php_sqlsrv_int.h @@ -130,9 +130,6 @@ struct ss_sqlsrv_stmt : public sqlsrv_stmt { // driver specific conversion rules from a SQL Server/ODBC type to one of the SQLSRV_PHPTYPE_* constants sqlsrv_phptype sql_type_to_php_type( _In_ SQLINTEGER sql_type, _In_ SQLUINTEGER size, _In_ bool prefer_string_to_stream ); - // driver specific way to set query timeout - virtual void set_query_timeout(); - bool prepared; // whether the statement has been prepared yet (used for error messages) zend_ulong conn_index; // index into the connection hash that contains this statement structure zval* params_z; // hold parameters passed to sqlsrv_prepare but not used until sqlsrv_execute diff --git a/source/sqlsrv/stmt.cpp b/source/sqlsrv/stmt.cpp index 36355f7b..ea1bfe54 100644 --- a/source/sqlsrv/stmt.cpp +++ b/source/sqlsrv/stmt.cpp @@ -261,29 +261,6 @@ sqlsrv_phptype ss_sqlsrv_stmt::sql_type_to_php_type( _In_ SQLINTEGER sql_type, _ return ss_phptype; } -void ss_sqlsrv_stmt::set_query_timeout() -{ - if (query_timeout == QUERY_TIMEOUT_INVALID || query_timeout < 0) { - return; - } - - // set the statement attribute - core::SQLSetStmtAttr(this, SQL_ATTR_QUERY_TIMEOUT, reinterpret_cast( (SQLLEN)query_timeout ), SQL_IS_UINTEGER ); - - // a query timeout of 0 indicates "no timeout", which means that lock_timeout should also be set to "no timeout" which - // is represented by -1. - int lock_timeout = (( query_timeout == 0 ) ? -1 : query_timeout * 1000 /*convert to milliseconds*/ ); - - // set the LOCK_TIMEOUT on the server. - char lock_timeout_sql[32] = {'\0'}; - - int written = snprintf( lock_timeout_sql, sizeof( lock_timeout_sql ), "SET LOCK_TIMEOUT %d", lock_timeout ); - SQLSRV_ASSERT( (written != -1 && written != sizeof( lock_timeout_sql )), - "stmt_option_query_timeout: snprintf failed. Shouldn't ever fail." ); - - core::SQLExecDirect(this, lock_timeout_sql ); -} - // statement specific parameter proccessing. Uses the generic function specialised to return a statement // resource. #define PROCESS_PARAMS( rsrc, param_spec, calling_func, param_count, ... ) \ diff --git a/test/functional/pdo_sqlsrv/pdo_1100_query_timeout_disconnect.phpt b/test/functional/pdo_sqlsrv/pdo_1100_query_timeout_disconnect.phpt new file mode 100644 index 00000000..36cf8fc8 --- /dev/null +++ b/test/functional/pdo_sqlsrv/pdo_1100_query_timeout_disconnect.phpt @@ -0,0 +1,72 @@ +--TEST-- +GitHub issue 1100 - PDO::SQLSRV_ATTR_QUERY_TIMEOUT had no effect when reconnecting +--DESCRIPTION-- +This test verifies that setting PDO::SQLSRV_ATTR_QUERY_TIMEOUT should work when reconnecting after disconnecting +--ENV-- +PHPT_EXEC=true +--SKIPIF-- + +--FILE-- + $leeway); + trace("$elapsed secs elapsed\n"); + + if ($missed) { + echo "Expected $expectedDelay but $elapsed secs elapsed\n"; + } +} + +function testTimeout($conn, $timeout) +{ + $delay = 5; + $query = "WAITFOR DELAY '00:00:$delay'; SELECT 1"; + $error = '*Query timeout expired'; + + $t0 = microtime(true); + try { + $conn->exec($query); + $elapsed = microtime(true) - $t0; + echo "Should have failed after $timeout secs but $elapsed secs have elapsed" . PHP_EOL; + } catch (PDOException $e) { + $t1 = microtime(true); + + $message = '*Query timeout expired'; + if (!fnmatch($message, $e->getMessage())) { + var_dump($e->getMessage()); + } + checkTimeElapsed($t0, $t1, $timeout); + } +} + +try { + $keywords = 'MultipleActiveResultSets=false;'; + $timeout = 1; + + $options = array(PDO::SQLSRV_ATTR_QUERY_TIMEOUT => $timeout); + $conn = connect($keywords, $options); + + testTimeout($conn, $timeout); + unset($conn); + + $conn = connect($keywords); + $conn->setAttribute(PDO::SQLSRV_ATTR_QUERY_TIMEOUT, $timeout); + + testTimeout($conn, $timeout); + unset($conn); + + echo "Done\n"; +} catch (PdoException $e) { + echo $e->getMessage() . PHP_EOL; +} + +?> +--EXPECT-- +Done diff --git a/test/functional/sqlsrv/test_timeout.phpt b/test/functional/sqlsrv/test_timeout.phpt index 195faf63..28c83ce0 100644 --- a/test/functional/sqlsrv/test_timeout.phpt +++ b/test/functional/sqlsrv/test_timeout.phpt @@ -10,19 +10,23 @@ sqlsrv_configure( 'LogSeverity', SQLSRV_LOG_SEVERITY_ALL ); require( 'MsCommon.inc' ); -$throwaway = Connect(array( 'ConnectionPooling' => 1 )); +// MARS allows applications to have more than one pending request per connection and to have more +// than one active default result set per connection, which is not required for this test. +$options = array('ConnectionPooling' => 1, 'MultipleActiveResultSets' => 0); + +$throwaway = Connect(); if( !$throwaway ) { die( print_r( sqlsrv_errors(), true )); } for( $i = 1; $i <= 3; ++$i ) { - $conn = Connect(array( 'ConnectionPooling' => 1 )); + $conn = Connect($options); if( !$conn ) { die( print_r( sqlsrv_errors(), true )); } - $conn2 = Connect(array( 'ConnectionPooling' => 1 )); + $conn2 = Connect($options); if( !$conn2 ) { die( print_r( sqlsrv_errors(), true )); } @@ -47,7 +51,6 @@ for( $i = 1; $i <= 3; ++$i ) { die( print_r( sqlsrv_errors(), true )); } - $stmt2 = sqlsrv_query( $conn2, "WAITFOR DELAY '00:00:05'; SELECT * FROM [test_query_timeout]", array(null), array( 'QueryTimeout' => 1 )); if( $stmt2 === false ) { print_r( sqlsrv_errors() );