jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/367867 )
Change subject: rdbms: Ensure onTransactionPreCommitOrIdle() callbacks don't
lead transactions
......................................................................
rdbms: Ensure onTransactionPreCommitOrIdle() callbacks don't lead transactions
If no writes started a transaction yet, the callback would run
but not commit (by design, joining the request round). Later
writes will then pile on top of it.
The point of this method is to avoid such cases, so this edge
case has been fixed.
Change-Id: I9b44b19261d679de4aff6e44a9cfeb4f684ce02e
---
M includes/libs/rdbms/database/Database.php
M tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
2 files changed, 71 insertions(+), 2 deletions(-)
Approvals:
Krinkle: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/libs/rdbms/database/Database.php
b/includes/libs/rdbms/database/Database.php
index 723a4a6..b8b44e6 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -2664,10 +2664,13 @@
}
final public function onTransactionPreCommitOrIdle( callable $callback,
$fname = __METHOD__ ) {
- if ( $this->mTrxLevel ) {
+ if ( $this->mTrxLevel || $this->getFlag( self::DBO_TRX ) ) {
+ // As long as DBO_TRX is set, writes will accumulate
until the load balancer issues
+ // an implicit commit of all peer databases. This is
true even if a transaction has
+ // not yet been triggered by writes; make sure
$callback runs *after* any such writes.
$this->mTrxPreCommitCallbacks[] = [ $callback, $fname ];
} else {
- // If no transaction is active, then make one for this
callback
+ // No transaction is active nor will start implicitly,
so make one for this callback
$this->startAtomic( __METHOD__ );
try {
call_user_func( $callback );
diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index 9bea7ff..70b6c36 100644
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -1,6 +1,7 @@
<?php
use Wikimedia\Rdbms\IDatabase;
+use Wikimedia\Rdbms\LBFactorySingle;
use Wikimedia\Rdbms\TransactionProfiler;
use Wikimedia\TestingAccessWrapper;
@@ -136,6 +137,71 @@
}
/**
+ * @covers Wikimedia\Rdbms\Database::onTransactionPreCommitOrIdle
+ * @covers Wikimedia\Rdbms\Database::runOnTransactionPreCommitCallbacks
+ */
+ public function testTransactionPreCommitOrIdle() {
+ $db = $this->getMockDB( [ 'isOpen' ] );
+ $db->method( 'isOpen' )->willReturn( true );
+ $db->clearFlag( DBO_TRX );
+
+ $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX is not
set' );
+
+ $called = false;
+ $db->onTransactionPreCommitOrIdle(
+ function () use ( &$called ) {
+ $called = true;
+ },
+ __METHOD__
+ );
+ $this->assertTrue( $called, 'Called when idle' );
+
+ $db->begin( __METHOD__ );
+ $called = false;
+ $db->onTransactionPreCommitOrIdle(
+ function () use ( &$called ) {
+ $called = true;
+ },
+ __METHOD__
+ );
+ $this->assertFalse( $called, 'Not called when transaction is
active' );
+ $db->commit( __METHOD__ );
+ $this->assertTrue( $called, 'Called when transaction is
committed' );
+ }
+
+ /**
+ * @covers Wikimedia\Rdbms\Database::onTransactionPreCommitOrIdle
+ * @covers Wikimedia\Rdbms\Database::runOnTransactionPreCommitCallbacks
+ */
+ public function testTransactionPreCommitOrIdle_TRX() {
+ $db = $this->getMockDB( [ 'isOpen' ] );
+ $db->method( 'isOpen' )->willReturn( true );
+ $db->setFlag( DBO_TRX );
+
+ $lbFactory = LBFactorySingle::newFromConnection( $db );
+ // Ask for the connectin so that LB sets internal state
+ // about this connection being the master connection
+ $lb = $lbFactory->getMainLB();
+ $conn = $lb->openConnection( $lb->getWriterIndex() );
+ $this->assertSame( $db, $conn, 'Same DB instance' );
+ $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
+
+ $called = false;
+ $db->onTransactionPreCommitOrIdle(
+ function () use ( &$called ) {
+ $called = true;
+ }
+ );
+ $this->assertFalse( $called, 'Not called when idle if DBO_TRX
is set' );
+
+ $lbFactory->beginMasterChanges( __METHOD__ );
+ $this->assertFalse( $called, 'Not called when lb-transaction is
active' );
+
+ $lbFactory->commitMasterChanges( __METHOD__ );
+ $this->assertTrue( $called, 'Called when lb-transaction is
committed' );
+ }
+
+ /**
* @covers Wikimedia\Rdbms\Database::onTransactionResolution
* @covers Wikimedia\Rdbms\Database::runOnTransactionIdleCallbacks
*/
--
To view, visit https://gerrit.wikimedia.org/r/367867
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9b44b19261d679de4aff6e44a9cfeb4f684ce02e
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
