Modified how drivers handle query timeout settings (#1037)

Author: yitam

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 ) {

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<unsigned int>(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<SQLPOINTER>((SQLLEN)query_timeout), SQL_IS_UINTEGER TSRMLS_CC);
+}

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

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 );

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<long>( 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<SQLPOINTER>( (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<long>(Z_LVAL_P(value_z));
     }
     catch( core::CoreException& ) {
         throw;

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

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<SQLPOINTER>( (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, ... )                                                        \

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--
+<?php require('skipif_mid-refactor.inc'); ?>
+--FILE--
+<?php
+require_once("MsSetup.inc");
+require_once("MsCommon_mid-refactor.inc");
+
+const _DELAY = 5;
+
+$message = '*Invalid value timeout specified for option PDO::SQLSRV_ATTR_QUERY_TIMEOUT.';
+$delay = _DELAY;
+$query = "WAITFOR DELAY '00:00:$delay'; SELECT 1";
+$error = '*Query timeout expired';
+
+function testTimeoutAttribute($conn, $timeout, $statementLevel = false)
+{
+    global $message;
+
+    $invalid = str_replace('timeout', $timeout, $message);
+
+    try {
+        if ($statementLevel) {
+            trace("statement option expects error: $invalid\n");
+            $options = array(PDO::SQLSRV_ATTR_QUERY_TIMEOUT => $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