From 255752066dc4180645b07cbdff718899b302df1a Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Wed, 18 Sep 2019 07:49:14 -0700 Subject: [PATCH] Modified how drivers handle query timeout settings (#1037) --- source/pdo_sqlsrv/pdo_dbh.cpp | 7 - source/pdo_sqlsrv/pdo_stmt.cpp | 13 ++ source/pdo_sqlsrv/php_pdo_sqlsrv_int.h | 4 + source/shared/core_sqlsrv.h | 3 +- source/shared/core_stmt.cpp | 40 +--- source/sqlsrv/php_sqlsrv_int.h | 3 + source/sqlsrv/stmt.cpp | 23 ++ .../pdo_sqlsrv/pdo_1027_query_timeout.phpt | 198 ++++++++++++++++++ .../sqlsrv/srv_1027_query_timeout.phpt | 120 +++++++++++ 9 files changed, 371 insertions(+), 40 deletions(-) create mode 100644 test/functional/pdo_sqlsrv/pdo_1027_query_timeout.phpt create mode 100644 test/functional/sqlsrv/srv_1027_query_timeout.phpt diff --git a/source/pdo_sqlsrv/pdo_dbh.cpp b/source/pdo_sqlsrv/pdo_dbh.cpp index 2a0ea479..15aec117 100644 --- a/source/pdo_sqlsrv/pdo_dbh.cpp +++ b/source/pdo_sqlsrv/pdo_dbh.cpp @@ -720,13 +720,6 @@ int pdo_sqlsrv_dbh_prepare( _Inout_ pdo_dbh_t *dbh, _In_reads_(sql_len) const ch driver_stmt->buffered_query_limit = driver_dbh->client_buffer_max_size; } - // if the user didn't set anything in the prepare options, then set the query timeout - // to the value set on the connection. - if(( driver_stmt->query_timeout == QUERY_TIMEOUT_INVALID ) && ( driver_dbh->query_timeout != QUERY_TIMEOUT_INVALID )) { - - core_sqlsrv_set_query_timeout( driver_stmt, driver_dbh->query_timeout TSRMLS_CC ); - } - // rewrite named parameters in the query to positional parameters if we aren't letting PDO do the // parameter substitution for us if( stmt->supports_placeholders != PDO_PLACEHOLDER_NONE ) { diff --git a/source/pdo_sqlsrv/pdo_stmt.cpp b/source/pdo_sqlsrv/pdo_stmt.cpp index 71c42690..0cd562d9 100644 --- a/source/pdo_sqlsrv/pdo_stmt.cpp +++ b/source/pdo_sqlsrv/pdo_stmt.cpp @@ -580,6 +580,11 @@ int pdo_sqlsrv_stmt_execute( _Inout_ pdo_stmt_t *stmt TSRMLS_DC ) query_len = static_cast(stmt->active_query_stringlen); } + // The query timeout setting is inherited from the corresponding connection attribute, but + // the user may have changed the query timeout setting again before this via + // PDOStatement::setAttribute() + driver_stmt->set_query_timeout(); + SQLRETURN execReturn = core_sqlsrv_execute( driver_stmt TSRMLS_CC, query, query_len ); if ( execReturn == SQL_NO_DATA ) { @@ -1503,3 +1508,11 @@ 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 TSRMLS_CC); +} \ 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 6a616da0..e0f5f220 100644 --- a/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h +++ b/source/pdo_sqlsrv/php_pdo_sqlsrv_int.h @@ -246,6 +246,7 @@ struct pdo_sqlsrv_stmt : public sqlsrv_stmt { fetch_datetime = db->fetch_datetime; format_decimals = db->format_decimals; decimal_places = db->decimal_places; + query_timeout = db->query_timeout; } virtual ~pdo_sqlsrv_stmt( void ); @@ -254,6 +255,9 @@ 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 a9e281c4..f468a7b6 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -1558,6 +1558,8 @@ 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 *** @@ -1616,7 +1618,6 @@ bool core_sqlsrv_has_any_result( _Inout_ sqlsrv_stmt* stmt TSRMLS_DC ); void core_sqlsrv_next_result( _Inout_ sqlsrv_stmt* stmt TSRMLS_DC, _In_ bool finalize_output_params = true, _In_ bool throw_on_errors = true ); void core_sqlsrv_post_param( _Inout_ sqlsrv_stmt* stmt, _In_ zend_ulong paramno, zval* param_z TSRMLS_DC ); void core_sqlsrv_set_scrollable( _Inout_ sqlsrv_stmt* stmt, _In_ unsigned long cursor_type TSRMLS_DC ); -void core_sqlsrv_set_query_timeout( _Inout_ sqlsrv_stmt* stmt, _In_ long timeout TSRMLS_DC ); void core_sqlsrv_set_query_timeout( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* value_z TSRMLS_DC ); void core_sqlsrv_set_send_at_exec( _Inout_ sqlsrv_stmt* stmt, _In_ zval* value_z TSRMLS_DC ); bool core_sqlsrv_send_stream_packet( _Inout_ sqlsrv_stmt* stmt TSRMLS_DC ); diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index 13bb2e5e..be1b00ec 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -322,6 +322,11 @@ sqlsrv_stmt* core_sqlsrv_create_stmt( _Inout_ sqlsrv_conn* conn, _In_ driver_stm } ZEND_HASH_FOREACH_END(); } + // The query timeout setting is inherited from the corresponding connection attribute, but + // the user may override that the query timeout setting using the statement option. + // In any case, set query timeout using the latest value + stmt->set_query_timeout(); + return_stmt = stmt; stmt.transferred(); } @@ -1361,7 +1366,7 @@ void core_sqlsrv_set_buffered_query_limit( _Inout_ sqlsrv_stmt* stmt, _In_ SQLLE } -// Overloaded. Extracts the long value and calls the core_sqlsrv_set_query_timeout +// Extracts the long value and calls the core_sqlsrv_set_query_timeout // which accepts timeout parameter as a long. If the zval is not of type long // than throws error. void core_sqlsrv_set_query_timeout( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* value_z TSRMLS_DC ) @@ -1375,37 +1380,8 @@ void core_sqlsrv_set_query_timeout( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* val THROW_CORE_ERROR( stmt, SQLSRV_ERROR_INVALID_QUERY_TIMEOUT_VALUE, Z_STRVAL_P( value_z ) ); } - core_sqlsrv_set_query_timeout( stmt, static_cast( Z_LVAL_P( value_z )) TSRMLS_CC ); - } - catch( core::CoreException& ) { - throw; - } -} - -// Overloaded. Accepts the timeout as a long. -void core_sqlsrv_set_query_timeout( _Inout_ sqlsrv_stmt* stmt, _In_ long timeout TSRMLS_DC ) -{ - try { - - DEBUG_SQLSRV_ASSERT( timeout >= 0 , "core_sqlsrv_set_query_timeout: The value of query timeout cannot be less than 0." ); - - // set the statement attribute - core::SQLSetStmtAttr( stmt, SQL_ATTR_QUERY_TIMEOUT, reinterpret_cast( (SQLLEN)timeout ), SQL_IS_UINTEGER TSRMLS_CC ); - - // 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 = (( timeout == 0 ) ? -1 : 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( stmt, lock_timeout_sql TSRMLS_CC ); - - stmt->query_timeout = timeout; + // Save the query timeout setting for processing later + stmt->query_timeout = static_cast(Z_LVAL_P(value_z)); } catch( core::CoreException& ) { throw; diff --git a/source/sqlsrv/php_sqlsrv_int.h b/source/sqlsrv/php_sqlsrv_int.h index 3ebb179f..42148b03 100644 --- a/source/sqlsrv/php_sqlsrv_int.h +++ b/source/sqlsrv/php_sqlsrv_int.h @@ -124,6 +124,9 @@ 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 bbb01190..b67af51a 100644 --- a/source/sqlsrv/stmt.cpp +++ b/source/sqlsrv/stmt.cpp @@ -267,6 +267,29 @@ 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 TSRMLS_CC ); + + // 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 TSRMLS_CC ); +} + // 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_1027_query_timeout.phpt b/test/functional/pdo_sqlsrv/pdo_1027_query_timeout.phpt new file mode 100644 index 00000000..703ae761 --- /dev/null +++ b/test/functional/pdo_sqlsrv/pdo_1027_query_timeout.phpt @@ -0,0 +1,198 @@ +--TEST-- +GitHub issue 1027 - PDO::SQLSRV_ATTR_QUERY_TIMEOUT had no effect on PDO::exec() +--DESCRIPTION-- +This test verifies that setting PDO::SQLSRV_ATTR_QUERY_TIMEOUT correctly should affect PDO::exec() as in the case for PDO::prepare() (as statement attribute or option). +--ENV-- +PHPT_EXEC=true +--SKIPIF-- + +--FILE-- + $timeout); + $sql = 'SELECT 1'; + $stmt = $conn->prepare($sql, $options); + } else { + trace("connection attribute expects error: $invalid\n"); + $conn->setAttribute(PDO::SQLSRV_ATTR_QUERY_TIMEOUT, $timeout); + } + } catch (PDOException $e) { + if (!fnmatch($invalid, $e->getMessage())) { + echo "Unexpected error returned setting invalid $timeout for SQLSRV_ATTR_QUERY_TIMEOUT\n"; + var_dump($e->getMessage()); + } + } +} + +function testErrors($conn) +{ + testTimeoutAttribute($conn, 1.8); + testTimeoutAttribute($conn, 'xyz'); + testTimeoutAttribute($conn, -99, true); + testTimeoutAttribute($conn, 'abc', true); +} + +function checkTimeElapsed($message, $t0, $t1, $expectedDelay) +{ + $elapsed = $t1 - $t0; + $diff = abs($elapsed - $expectedDelay); + $leeway = 1.0; + $missed = ($diff > $leeway); + trace("$message $elapsed secs elapsed\n"); + + if ($missed) { + echo $message; + echo "Expected $expectedDelay but $elapsed secs elapsed\n"; + } +} + +function connectionTest($timeout, $asAttribute) +{ + global $query, $error; + $keyword = ''; + + if ($asAttribute) { + $conn = connect($keyword); + $conn->setAttribute(PDO::SQLSRV_ATTR_QUERY_TIMEOUT, $timeout); + } else { + $options = array(PDO::SQLSRV_ATTR_QUERY_TIMEOUT => $timeout); + $conn = connect($keyword, $options); + } + + $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + + // if timeout is 0 it means no timeout + $delay = ($timeout > 0) ? $timeout : _DELAY; + + $result = null; + $t0 = microtime(true); + + try { + $result = $conn->exec($query); + if ($timeout > 0) { + echo "connectionTest $timeout, $asAttribute: "; + echo "this should have timed out!\n"; + } + } catch (PDOException $e) { + if (!fnmatch($error, $e->getMessage())) { + echo "Connection test error expected $timeout, $asAttribute:\n"; + var_dump($e->getMessage()); + } + } + + $t1 = microtime(true); + checkTimeElapsed("connectionTest ($timeout, $asAttribute): ", $t0, $t1, $delay); + + return $conn; +} + +function queryTest($conn, $timeout) +{ + global $query, $error; + + // if timeout is 0 it means no timeout + $delay = ($timeout > 0) ? $timeout : _DELAY; + + $t0 = microtime(true); + try { + $conn->setAttribute(PDO::SQLSRV_ATTR_QUERY_TIMEOUT, $timeout); + $stmt = $conn->query($query); + + if ($timeout > 0) { + echo "Query test $timeout: should have timed out!\n"; + } + } catch (PDOException $e) { + if (!fnmatch($error, $e->getMessage())) { + echo "Query test error expected $timeout:\n"; + var_dump($e->getMessage()); + } + } + + $t1 = microtime(true); + + checkTimeElapsed("Query test ($timeout): ", $t0, $t1, $delay); + + unset($stmt); +} + +function statementTest($conn, $timeout, $asAttribute) +{ + global $query, $error; + + // if timeout is 0 it means no timeout + $delay = ($timeout > 0) ? $timeout : _DELAY; + + $result = null; + $t0 = microtime(true); + + try { + if ($asAttribute) { + $stmt = $conn->prepare($query); + $stmt->setAttribute(PDO::SQLSRV_ATTR_QUERY_TIMEOUT, $timeout); + } else { + $options = array(PDO::SQLSRV_ATTR_QUERY_TIMEOUT => $timeout); + $stmt = $conn->prepare($query, $options); + } + + $result = $stmt->execute(); + + if ($timeout > 0) { + echo "statementTest $timeout: should have timed out!\n"; + } + } catch (PDOException $e) { + if (!fnmatch($error, $e->getMessage())) { + echo "Statement test error expected $timeout, $asAttribute:\n"; + var_dump($e->getMessage()); + } + } + + $t1 = microtime(true); + + checkTimeElapsed("statementTest ($timeout, $asAttribute): ", $t0, $t1, $delay); + + unset($stmt); +} + +try { + $rand = rand(1, 100); + $timeout = $rand % 3; + $asAttribute = $rand % 2; + + $conn = connectionTest($timeout, $asAttribute); + testErrors($conn); + unset($conn); + + $conn = connectionTest(0, !$asAttribute); + queryTest($conn, $timeout); + + for ($i = 0; $i < 2; $i++) { + statementTest($conn, $timeout, $i); + } + unset($conn); + + echo "Done\n"; +} catch (PdoException $e) { + echo $e->getMessage() . PHP_EOL; +} + +?> +--EXPECT-- +Done diff --git a/test/functional/sqlsrv/srv_1027_query_timeout.phpt b/test/functional/sqlsrv/srv_1027_query_timeout.phpt new file mode 100644 index 00000000..a22fe3a1 --- /dev/null +++ b/test/functional/sqlsrv/srv_1027_query_timeout.phpt @@ -0,0 +1,120 @@ +--TEST-- +GitHub issue 1027 - timeout option +--DESCRIPTION-- +This test is a variant of the corresponding PDO test, and it verifies that setting the query timeout option should affect sqlsrv_query or sqlsrv_prepare correctly. +--ENV-- +PHPT_EXEC=true +--SKIPIF-- + +--FILE-- + $timeout); + $sql = 'SELECT 1'; + + if ($prepare) { + $stmt = sqlsrv_prepare($conn, $sql, null, $options); + } else { + $stmt = sqlsrv_query($conn, $sql, null, $options); + } + + if ($stmt !== false) { + echo "Expect this to fail with timeout option $timeout\n"; + } + if (sqlsrv_errors()[0]['message'] !== $error) { + print_r(sqlsrv_errors()); + } +} + +function testErrors($conn) +{ + testTimeout($conn, 1.8); + testTimeout($conn, 'xyz'); + testTimeout($conn, -99, true); + testTimeout($conn, 'abc', true); +} + +function checkTimeElapsed($message, $t0, $t1, $expectedDelay) +{ + $elapsed = $t1 - $t0; + $diff = abs($elapsed - $expectedDelay); + $leeway = 1.0; + $missed = ($diff > $leeway); + trace("$message $elapsed secs elapsed\n"); + + if ($missed) { + echo $message; + echo "Expected $expectedDelay but $elapsed secs elapsed\n"; + } +} + +function statementTest($conn, $timeout, $prepare) +{ + global $query, $expired; + + $options = array('QueryTimeout' => $timeout); + $stmt = null; + $result = null; + + // if timeout is 0 it means no timeout + $delay = ($timeout > 0) ? $timeout : _DELAY; + + $t0 = microtime(true); + if ($prepare) { + $stmt = sqlsrv_prepare($conn, $query, null, $options); + $result = sqlsrv_execute($stmt); + } else { + $stmt = sqlsrv_query($conn, $query, null, $options); + } + + $t1 = microtime(true); + + if ($timeout > 0) { + if ($prepare && $result !== false) { + echo "Prepared statement should fail with timeout $timeout\n"; + } elseif (!$prepare && $stmt !== false) { + echo "Query should fail with timeout $timeout\n"; + } else { + // check error messages + $errors = sqlsrv_errors(); + if (!fnmatch($expired, $errors[0]['message'])) { + echo "Unexpected error returned ($timeout, $prepare):\n"; + print_r(sqlsrv_errors()); + } + } + } + + checkTimeElapsed("statementTest ($timeout, $prepare): ", $t0, $t1, $delay); +} + +$conn = AE\connect(); + +testErrors($conn); + +$rand = rand(1, 100); +$timeout = $rand % 3; + +for ($i = 0; $i < 2; $i++) { + statementTest($conn, $timeout, $i); +} + +sqlsrv_close($conn); + +echo "Done\n"; + +?> +--EXPECT-- +Done \ No newline at end of file