From c8085ad43f9a79b57c92ee659387c735baa33517 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 16 Apr 2018 21:39:02 -0700 Subject: [PATCH] rdbms: make IDatabase::onTransaction* methods pass the DB handle for convenience Change-Id: Ia45a26830d62326b103593268fbf34c907783c90 --- includes/FileDeleteForm.php | 2 +- includes/jobqueue/JobQueueDB.php | 4 ++-- includes/libs/rdbms/database/Database.php | 8 ++++---- includes/libs/rdbms/database/IDatabase.php | 9 +++++++-- includes/page/WikiPage.php | 2 +- includes/profiler/output/ProfilerOutputDb.php | 3 ++- .../libs/rdbms/database/DatabaseSQLTest.php | 15 +++++++++------ .../includes/libs/rdbms/database/DatabaseTest.php | 14 +++++++------- 8 files changed, 33 insertions(+), 24 deletions(-) diff --git a/includes/FileDeleteForm.php b/includes/FileDeleteForm.php index 783de1c0c4..898005ecc9 100644 --- a/includes/FileDeleteForm.php +++ b/includes/FileDeleteForm.php @@ -212,7 +212,7 @@ class FileDeleteForm { $logEntry->setTags( $tags ); $logid = $logEntry->insert(); $dbw->onTransactionPreCommitOrIdle( - function () use ( $dbw, $logEntry, $logid ) { + function () use ( $logEntry, $logid ) { $logEntry->publish( $logid ); }, __METHOD__ diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php index f01ba63f02..c3193c3ba7 100644 --- a/includes/jobqueue/JobQueueDB.php +++ b/includes/jobqueue/JobQueueDB.php @@ -196,7 +196,7 @@ class JobQueueDB extends JobQueue { // errors that bubble up will rollback the main commit round. $fname = __METHOD__; $dbw->onTransactionPreCommitOrIdle( - function () use ( $dbw, $jobs, $flags, $fname ) { + function ( IDatabase $dbw ) use ( $jobs, $flags, $fname ) { $this->doBatchPushInternal( $dbw, $jobs, $flags, $fname ); }, $fname @@ -508,7 +508,7 @@ class JobQueueDB extends JobQueue { $dbw = $this->getMasterDB(); $cache = $this->dupCache; $dbw->onTransactionIdle( - function () use ( $cache, $params, $key, $dbw ) { + function () use ( $cache, $params, $key ) { $timestamp = $cache->get( $key ); // current last timestamp of this job if ( $timestamp && $timestamp >= $params['rootJobTimestamp'] ) { return true; // a newer version of this root job was enqueued diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 5549f28a80..189eaf2f12 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -3322,7 +3322,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware // No transaction is active nor will start implicitly, so make one for this callback $this->startAtomic( __METHOD__, self::ATOMIC_CANCELABLE ); try { - call_user_func( $callback ); + call_user_func( $callback, $this ); $this->endAtomic( __METHOD__ ); } catch ( Exception $e ) { $this->cancelAtomic( __METHOD__ ); @@ -3391,7 +3391,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( in_array( $entry[2], $sectionIds, true ) ) { $callback = $entry[0]; $this->trxEndCallbacks[$key][0] = function () use ( $callback ) { - return $callback( self::TRIGGER_ROLLBACK ); + return $callback( self::TRIGGER_ROLLBACK, $this ); }; } } @@ -3445,7 +3445,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware try { list( $phpCallback ) = $callback; $this->clearFlag( self::DBO_TRX ); // make each query its own transaction - call_user_func_array( $phpCallback, [ $trigger ] ); + call_user_func( $phpCallback, $trigger, $this ); if ( $autoTrx ) { $this->setFlag( self::DBO_TRX ); // restore automatic begin() } else { @@ -3484,7 +3484,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware foreach ( $callbacks as $callback ) { try { list( $phpCallback ) = $callback; - call_user_func( $phpCallback ); + call_user_func( $phpCallback, $this ); } catch ( Exception $ex ) { call_user_func( $this->errorLogger, $ex ); $e = $e ?: $ex; diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 675ba7f2bc..6b3efa2bf0 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -1480,8 +1480,9 @@ interface IDatabase { * * @note: do not assume that *other* IDatabase instances will be AUTOCOMMIT mode * - * The callback takes one argument: + * The callback takes the following arguments: * - How the transaction ended (IDatabase::TRIGGER_COMMIT or IDatabase::TRIGGER_ROLLBACK) + * - This IDatabase instance (since 1.32) * * @param callable $callback * @param string $fname Caller name @@ -1511,8 +1512,9 @@ interface IDatabase { * * @note: do not assume that *other* IDatabase instances will be AUTOCOMMIT mode * - * The callback takes one argument: + * The callback takes the following arguments: * - How the transaction ended (IDatabase::TRIGGER_COMMIT or IDatabase::TRIGGER_IDLE) + * - This IDatabase instance (since 1.32) * * @param callable $callback * @param string $fname Caller name @@ -1536,6 +1538,9 @@ interface IDatabase { * * Updates will execute in the order they were enqueued. * + * The callback takes the one argument: + * - This IDatabase instance (since 1.32) + * * @param callable $callback * @param string $fname Caller name * @since 1.22 diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index f6e2bd073e..cbce884dfc 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -2978,7 +2978,7 @@ class WikiPage implements Page, IDBAccessObject { $logid = $logEntry->insert(); $dbw->onTransactionPreCommitOrIdle( - function () use ( $dbw, $logEntry, $logid ) { + function () use ( $logEntry, $logid ) { // T58776: avoid deadlocks (especially from FileDeleteForm) $logEntry->publish( $logid ); }, diff --git a/includes/profiler/output/ProfilerOutputDb.php b/includes/profiler/output/ProfilerOutputDb.php index 28dc2cc21e..1c3d479d05 100644 --- a/includes/profiler/output/ProfilerOutputDb.php +++ b/includes/profiler/output/ProfilerOutputDb.php @@ -21,6 +21,7 @@ * @ingroup Profiler */ +use Wikimedia\Rdbms\Database; use Wikimedia\Rdbms\DBError; /** @@ -55,7 +56,7 @@ class ProfilerOutputDb extends ProfilerOutput { } $fname = __METHOD__; - $dbw->onTransactionIdle( function () use ( $stats, $dbw, $fname ) { + $dbw->onTransactionIdle( function ( Database $dbw ) use ( $stats, $fname ) { $pfhost = $this->perHost ? wfHostname() : ''; // Sqlite: avoid excess b-tree rebuilds (mostly for non-WAL mode) // non-Sqlite: lower contention with small transactions diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 3d15b03e8a..4596c764fc 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -1434,6 +1434,9 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { IDatabase::TRIGGER_COMMIT => 'tCommit', IDatabase::TRIGGER_ROLLBACK => 'tRollback' ]; + $pcCallback = function ( IDatabase $db ) use ( $fname ) { + $this->database->query( "SELECT 0", $fname ); + }; $callback1 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) { $this->database->query( "SELECT 1, {$triggerMap[$trigger]} AS t", $fname ); }; @@ -1445,7 +1448,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { }; $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); - $this->database->onTransactionPreCommitOrIdle( $callback1, __METHOD__ ); + $this->database->onTransactionPreCommitOrIdle( $pcCallback, __METHOD__ ); $this->database->cancelAtomic( __METHOD__ ); $this->assertLastSql( 'BEGIN; ROLLBACK' ); @@ -1460,18 +1463,18 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->assertLastSql( 'BEGIN; ROLLBACK; SELECT 1, tRollback AS t' ); $this->database->startAtomic( __METHOD__ . '_outer' ); - $this->database->onTransactionPreCommitOrIdle( $callback1, __METHOD__ ); + $this->database->onTransactionPreCommitOrIdle( $pcCallback, __METHOD__ ); $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); - $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ ); + $this->database->onTransactionPreCommitOrIdle( $pcCallback, __METHOD__ ); $this->database->cancelAtomic( __METHOD__ ); - $this->database->onTransactionPreCommitOrIdle( $callback3, __METHOD__ ); + $this->database->onTransactionPreCommitOrIdle( $pcCallback, __METHOD__ ); $this->database->endAtomic( __METHOD__ . '_outer' ); $this->assertLastSql( implode( "; ", [ 'BEGIN', 'SAVEPOINT wikimedia_rdbms_atomic1', 'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1', - 'SELECT 1, - AS t', - 'SELECT 3, - AS t', + 'SELECT 0', + 'SELECT 0', 'COMMIT' ] ) ); diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php index e328cba6aa..3335a2b911 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php @@ -180,7 +180,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $db->clearFlag( DBO_TRX ); $called = false; $flagSet = null; - $callback = function () use ( $db, &$flagSet, &$called ) { + $callback = function ( $trigger, IDatabase $db ) use ( &$flagSet, &$called ) { $called = true; $flagSet = $db->getFlag( DBO_TRX ); }; @@ -203,7 +203,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $db->clearFlag( DBO_TRX ); $db->onTransactionIdle( - function () use ( $db ) { + function ( $trigger, IDatabase $db ) { $db->setFlag( DBO_TRX ); }, __METHOD__ @@ -274,7 +274,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $called = false; $db->onTransactionPreCommitOrIdle( - function () use ( &$called ) { + function ( IDatabase $db ) use ( &$called ) { $called = true; }, __METHOD__ @@ -284,7 +284,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $db->begin( __METHOD__ ); $called = false; $db->onTransactionPreCommitOrIdle( - function () use ( &$called ) { + function ( IDatabase $db ) use ( &$called ) { $called = true; }, __METHOD__ @@ -314,7 +314,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $this->assertFalse( $lb->hasMasterChanges() ); $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' ); $called = false; - $callback = function () use ( &$called ) { + $callback = function ( IDatabase $db ) use ( &$called ) { $called = true; }; $db->onTransactionPreCommitOrIdle( $callback, __METHOD__ ); @@ -352,7 +352,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $db->clearFlag( DBO_TRX ); $db->begin( __METHOD__ ); $called = false; - $db->onTransactionResolution( function () use ( $db, &$called ) { + $db->onTransactionResolution( function ( $trigger, IDatabase $db ) use ( &$called ) { $called = true; $db->setFlag( DBO_TRX ); } ); @@ -363,7 +363,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase { $db->clearFlag( DBO_TRX ); $db->begin( __METHOD__ ); $called = false; - $db->onTransactionResolution( function () use ( $db, &$called ) { + $db->onTransactionResolution( function ( $trigger, IDatabase $db ) use ( &$called ) { $called = true; $db->setFlag( DBO_TRX ); } ); -- 2.20.1