rdbms: make cancelAtomic() handle callbacks and work with DBO_TRX

Make transaction callbacks aware of cancelled sections. If the
statements of a section are reverted via cancelAtomic(), then the
dependant callbacks are now cancelled as well. Any callbacks for
onTransactionResolution(), which does not depend on COMMIT, will
see the triggering event as a ROLLBACK, since the unit of work it
was part of was rolled back.

Also fix the handling of topmost atomic sections with DBO_TRX.
These still need their own savepoint to make cancelAtomic() work.

Follow-up to 52aeaa7a5.

Change-Id: If4d455c98155283797678cfb9df31d5317dd91a2

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index f681795..1af94cc 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -109,11 +109,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        /** @var bool */
        protected $opened = false;
 
-       /** @var array[] List of (callable, method name) */
+       /** @var array[] List of (callable, method name, atomic section id) */
        protected $trxIdleCallbacks = [];
-       /** @var array[] List of (callable, method name) */
+       /** @var array[] List of (callable, method name, atomic section id) */
        protected $trxPreCommitCallbacks = [];
-       /** @var array[] List of (callable, method name) */
+       /** @var array[] List of (callable, method name, atomic section id) */
        protected $trxEndCallbacks = [];
        /** @var callable[] Map of (name => callable) */
        protected $trxRecurringCallbacks = [];
@@ -274,6 +274,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        /** @var int */
        protected $nonNativeInsertSelectBatchSize = 10000;
 
+       /** @var string Idiom used when a cancelable atomic section started the transaction */
+       private static $NOT_APPLICABLE = 'n/a';
+       /** @var string Prefix to the atomic section counter used to make savepoint IDs */
+       private static $SAVEPOINT_PREFIX = 'wikimedia_rdbms_atomic';
+
        /** @var int Transaction is in a error state requiring a full or savepoint rollback */
        const STATUS_TRX_ERROR = 1;
        /** @var int Transaction is active and in a normal state */
@@ -3231,7 +3236,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                if ( !$this->trxLevel ) {
                        throw new DBUnexpectedError( $this, "No transaction is active." );
                }
-               $this->trxEndCallbacks[] = [ $callback, $fname ];
+               $this->trxEndCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ];
        }
 
        final public function onTransactionIdle( callable $callback, $fname = __METHOD__ ) {
@@ -3241,7 +3246,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $this->trxAutomatic = true;
                }
 
-               $this->trxIdleCallbacks[] = [ $callback, $fname ];
+               $this->trxIdleCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ];
                if ( !$this->trxLevel ) {
                        $this->runOnTransactionIdleCallbacks( self::TRIGGER_IDLE );
                }
@@ -3255,7 +3260,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                }
 
                if ( $this->trxLevel ) {
-                       $this->trxPreCommitCallbacks[] = [ $callback, $fname ];
+                       $this->trxPreCommitCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ];
                } else {
                        // No transaction is active nor will start implicitly, so make one for this callback
                        $this->startAtomic( __METHOD__, self::ATOMIC_CANCELABLE );
@@ -3269,6 +3274,72 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                }
        }
 
+       /**
+        * @return AtomicSectionIdentifier|null ID of the topmost atomic section level
+        */
+       private function currentAtomicSectionId() {
+               if ( $this->trxLevel && $this->trxAtomicLevels ) {
+                       $levelInfo = end( $this->trxAtomicLevels );
+
+                       return $levelInfo[1];
+               }
+
+               return null;
+       }
+
+       /**
+        * @param AtomicSectionIdentifier $old
+        * @param AtomicSectionIdentifier $new
+        */
+       private function reassignCallbacksForSection(
+               AtomicSectionIdentifier $old, AtomicSectionIdentifier $new
+       ) {
+               foreach ( $this->trxPreCommitCallbacks as $key => $info ) {
+                       if ( $info[2] === $old ) {
+                               $this->trxPreCommitCallbacks[$key][2] = $new;
+                       }
+               }
+               foreach ( $this->trxIdleCallbacks as $key => $info ) {
+                       if ( $info[2] === $old ) {
+                               $this->trxIdleCallbacks[$key][2] = $new;
+                       }
+               }
+               foreach ( $this->trxEndCallbacks as $key => $info ) {
+                       if ( $info[2] === $old ) {
+                               $this->trxEndCallbacks[$key][2] = $new;
+                       }
+               }
+       }
+
+       /**
+        * @param AtomicSectionIdentifier[] $sectionIds ID of an actual savepoint
+        * @throws UnexpectedValueException
+        */
+       private function modifyCallbacksForCancel( array $sectionIds ) {
+               // Cancel the "on commit" callbacks owned by this savepoint
+               $this->trxIdleCallbacks = array_filter(
+                       $this->trxIdleCallbacks,
+                       function ( $entry ) use ( $sectionIds ) {
+                               return !in_array( $entry[2], $sectionIds, true );
+                       }
+               );
+               $this->trxPreCommitCallbacks = array_filter(
+                       $this->trxPreCommitCallbacks,
+                       function ( $entry ) use ( $sectionIds ) {
+                               return !in_array( $entry[2], $sectionIds, true );
+                       }
+               );
+               // Make "on resolution" callbacks owned by this savepoint to perceive a rollback
+               foreach ( $this->trxEndCallbacks as $key => $entry ) {
+                       if ( in_array( $entry[2], $sectionIds, true ) ) {
+                               $callback = $entry[0];
+                               $this->trxEndCallbacks[$key][0] = function () use ( $callback ) {
+                                       return $callback( self::TRIGGER_ROLLBACK );
+                               };
+                       }
+               }
+       }
+
        final public function setTransactionListener( $name, callable $callback = null ) {
                if ( $callback ) {
                        $this->trxRecurringCallbacks[$name] = $callback;
@@ -3442,28 +3513,44 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                $this->query( 'ROLLBACK TO SAVEPOINT ' . $this->addIdentifierQuotes( $identifier ), $fname );
        }
 
+       /**
+        * @param string $fname
+        * @return string
+        */
+       private function nextSavepointId( $fname ) {
+               $savepointId = self::$SAVEPOINT_PREFIX . ++$this->trxAtomicCounter;
+               if ( strlen( $savepointId ) > 30 ) {
+                       // 30 == Oracle's identifier length limit (pre 12c)
+                       // With a 22 character prefix, that puts the highest number at 99999999.
+                       throw new DBUnexpectedError(
+                               $this,
+                               'There have been an excessively large number of atomic sections in a transaction'
+                               . " started by $this->trxFname (at $fname)"
+                       );
+               }
+
+               return $savepointId;
+       }
+
        final public function startAtomic(
                $fname = __METHOD__, $cancelable = self::ATOMIC_NOT_CANCELABLE
        ) {
-               $savepointId = $cancelable === self::ATOMIC_CANCELABLE ? 'n/a' : null;
+               $savepointId = $cancelable === self::ATOMIC_CANCELABLE ? self::$NOT_APPLICABLE : null;
+
                if ( !$this->trxLevel ) {
                        $this->begin( $fname, self::TRANSACTION_INTERNAL );
                        // If DBO_TRX is set, a series of startAtomic/endAtomic pairs will result
                        // in all changes being in one transaction to keep requests transactional.
-                       if ( !$this->getFlag( self::DBO_TRX ) ) {
+                       if ( $this->getFlag( self::DBO_TRX ) ) {
+                               // Since writes could happen in between the topmost atomic sections as part
+                               // of the transaction, those sections will need savepoints.
+                               $savepointId = $this->nextSavepointId( $fname );
+                               $this->doSavepoint( $savepointId, $fname );
+                       } else {
                                $this->trxAutomaticAtomic = true;
                        }
                } elseif ( $cancelable === self::ATOMIC_CANCELABLE ) {
-                       $savepointId = 'wikimedia_rdbms_atomic' . ++$this->trxAtomicCounter;
-                       if ( strlen( $savepointId ) > 30 ) { // 30 == Oracle's identifier length limit (pre 12c)
-                               $this->queryLogger->warning(
-                                       'There have been an excessively large number of atomic sections in a transaction'
-                                       . " started by $this->trxFname, reusing IDs (at $fname)",
-                                       [ 'trace' => ( new RuntimeException() )->getTraceAsString() ]
-                               );
-                               $this->trxAtomicCounter = 0;
-                               $savepointId = 'wikimedia_rdbms_atomic' . ++$this->trxAtomicCounter;
-                       }
+                       $savepointId = $this->nextSavepointId( $fname );
                        $this->doSavepoint( $savepointId, $fname );
                }
 
@@ -3480,7 +3567,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
                // Check if the current section matches $fname
                $pos = count( $this->trxAtomicLevels ) - 1;
-               list( $savedFname, , $savepointId ) = $this->trxAtomicLevels[$pos];
+               list( $savedFname, $sectionId, $savepointId ) = $this->trxAtomicLevels[$pos];
 
                if ( $savedFname !== $fname ) {
                        throw new DBUnexpectedError(
@@ -3489,14 +3576,21 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        );
                }
 
-               // Remove the last section and re-index the array
-               $this->trxAtomicLevels = array_slice( $this->trxAtomicLevels, 0, $pos );
+               // Remove the last section (no need to re-index the array)
+               array_pop( $this->trxAtomicLevels );
 
                if ( !$this->trxAtomicLevels && $this->trxAutomaticAtomic ) {
                        $this->commit( $fname, self::FLUSHING_INTERNAL );
-               } elseif ( $savepointId !== null && $savepointId !== 'n/a' ) {
+               } elseif ( $savepointId !== null && $savepointId !== self::$NOT_APPLICABLE ) {
                        $this->doReleaseSavepoint( $savepointId, $fname );
                }
+
+               // Hoist callback ownership for callbacks in the section that just ended;
+               // all callbacks should have an owner that is present in trxAtomicLevels.
+               $currentSectionId = $this->currentAtomicSectionId();
+               if ( $currentSectionId ) {
+                       $this->reassignCallbacksForSection( $sectionId, $currentSectionId );
+               }
        }
 
        final public function cancelAtomic(
@@ -3518,12 +3612,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                                throw new DBUnexpectedError( "Atomic section not found (for $fname)" );
                        }
                        // Remove all descendant sections and re-index the array
+                       $excisedIds = [];
+                       $len = count( $this->trxAtomicLevels );
+                       for ( $i = $pos + 1; $i < $len; ++$i ) {
+                               $excisedIds[] = $this->trxAtomicLevels[$i][1];
+                       }
                        $this->trxAtomicLevels = array_slice( $this->trxAtomicLevels, 0, $pos + 1 );
+                       $this->modifyCallbacksForCancel( $excisedIds );
                }
 
                // Check if the current section matches $fname
                $pos = count( $this->trxAtomicLevels ) - 1;
-               list( $savedFname, , $savepointId ) = $this->trxAtomicLevels[$pos];
+               list( $savedFname, $savedSectionId, $savepointId ) = $this->trxAtomicLevels[$pos];
 
                if ( $savedFname !== $fname ) {
                        throw new DBUnexpectedError(
@@ -3532,12 +3632,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        );
                }
 
-               // Remove the last section and re-index the array
-               $this->trxAtomicLevels = array_slice( $this->trxAtomicLevels, 0, $pos );
+               // Remove the last section (no need to re-index the array)
+               array_pop( $this->trxAtomicLevels );
+               $this->modifyCallbacksForCancel( [ $savedSectionId ] );
 
                if ( $savepointId !== null ) {
                        // Rollback the transaction to the state just before this atomic section
-                       if ( $savepointId === 'n/a' ) {
+                       if ( $savepointId === self::$NOT_APPLICABLE ) {
                                $this->rollback( $fname, self::FLUSHING_INTERNAL );
                        } else {
                                $this->doRollbackToSavepoint( $savepointId, $fname );

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index 3756da2..cf10801 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -1418,6 +1418,140 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
                // phpcs:ignore Generic.Files.LineLength
                $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; RELEASE SAVEPOINT wikimedia_rdbms_atomic1; ROLLBACK' );
 
+               $fname = __METHOD__;
+               $triggerMap = [
+                       '-' => '-',
+                       IDatabase::TRIGGER_COMMIT => 'tCommit',
+                       IDatabase::TRIGGER_ROLLBACK => 'tRollback'
+               ];
+               $callback1 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) {
+                       $this->database->query( "SELECT 1, {$triggerMap[$trigger]} AS t", $fname );
+               };
+               $callback2 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) {
+                       $this->database->query( "SELECT 2, {$triggerMap[$trigger]} AS t", $fname );
+               };
+               $callback3 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) {
+                       $this->database->query( "SELECT 3, {$triggerMap[$trigger]} AS t", $fname );
+               };
+
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionPreCommitOrIdle( $callback1, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->assertLastSql( 'BEGIN; ROLLBACK' );
+
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->assertLastSql( 'BEGIN; ROLLBACK' );
+
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionResolution( $callback1, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->assertLastSql( 'BEGIN; ROLLBACK; SELECT 1, tRollback AS t' );
+
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->onTransactionPreCommitOrIdle( $callback1, __METHOD__ );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback3, __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',
+                       'COMMIT'
+               ] ) );
+
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionIdle( $callback2, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->onTransactionIdle( $callback3, __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertLastSql( implode( "; ", [
+                       'BEGIN',
+                       'SAVEPOINT wikimedia_rdbms_atomic1',
+                       'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+                       'COMMIT',
+                       'SELECT 1, tCommit AS t',
+                       'SELECT 3, tCommit AS t'
+               ] ) );
+
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->onTransactionResolution( $callback1, __METHOD__ );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionResolution( $callback2, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertLastSql( implode( "; ", [
+                       'BEGIN',
+                       'SAVEPOINT wikimedia_rdbms_atomic1',
+                       'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+                       'COMMIT',
+                       'SELECT 1, tCommit AS t',
+                       'SELECT 2, tRollback AS t',
+                       'SELECT 3, tCommit AS t'
+               ] ) );
+
+               $makeCallback = function ( $id ) use ( $fname, $triggerMap ) {
+                       return function ( $trigger = '-' ) use ( $id, $fname, $triggerMap ) {
+                               $this->database->query( "SELECT $id, {$triggerMap[$trigger]} AS t", $fname );
+                       };
+               };
+
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionResolution( $makeCallback( 1 ), __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertLastSql( implode( "; ", [
+                       'BEGIN',
+                       'SAVEPOINT wikimedia_rdbms_atomic1',
+                       'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+                       'COMMIT',
+                       'SELECT 1, tRollback AS t'
+               ] ) );
+
+               $this->database->startAtomic( __METHOD__ . '_level1', IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionResolution( $makeCallback( 1 ), __METHOD__ );
+               $this->database->startAtomic( __METHOD__ . '_level2' );
+               $this->database->startAtomic( __METHOD__ . '_level3', IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionResolution( $makeCallback( 2 ), __METHOD__ );
+               $this->database->endAtomic( __METHOD__ );
+               $this->database->onTransactionResolution( $makeCallback( 3 ), __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ . '_level3' );
+               $this->database->endAtomic( __METHOD__ . '_level2' );
+               $this->database->onTransactionResolution( $makeCallback( 4 ), __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_level1' );
+               $this->assertLastSql( implode( "; ", [
+                       'BEGIN',
+                       'SAVEPOINT wikimedia_rdbms_atomic1',
+                       'SAVEPOINT wikimedia_rdbms_atomic2',
+                       'RELEASE SAVEPOINT wikimedia_rdbms_atomic2',
+                       'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+                       'COMMIT; SELECT 1, tCommit AS t',
+                       'SELECT 2, tRollback AS t',
+                       'SELECT 3, tRollback AS t',
+                       'SELECT 4, tCommit AS t'
+               ] ) );
+       }
+
+       /**
+        * @covers \Wikimedia\Rdbms\Database::doSavepoint
+        * @covers \Wikimedia\Rdbms\Database::doReleaseSavepoint
+        * @covers \Wikimedia\Rdbms\Database::doRollbackToSavepoint
+        * @covers \Wikimedia\Rdbms\Database::startAtomic
+        * @covers \Wikimedia\Rdbms\Database::endAtomic
+        * @covers \Wikimedia\Rdbms\Database::cancelAtomic
+        * @covers \Wikimedia\Rdbms\Database::doAtomicSection
+        */
+       public function testAtomicSectionsRecovery() {
                $this->database->begin( __METHOD__ );
                try {
                        $this->database->doAtomicSection(
@@ -1450,10 +1584,169 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
                } catch ( RuntimeException $ex ) {
                        $this->assertSame( 'Test exception', $ex->getMessage() );
                }
+               try {
+                       $this->database->commit( __METHOD__ );
+                       $this->fail( 'Test exception not thrown' );
+               } catch ( DBTransactionError $ex ) {
+                       $this->assertSame(
+                               'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.',
+                               $ex->getMessage()
+                       );
+               }
                $this->database->rollback( __METHOD__ );
                $this->assertLastSql( 'BEGIN; ROLLBACK' );
        }
 
+       /**
+        * @covers \Wikimedia\Rdbms\Database::doSavepoint
+        * @covers \Wikimedia\Rdbms\Database::doReleaseSavepoint
+        * @covers \Wikimedia\Rdbms\Database::doRollbackToSavepoint
+        * @covers \Wikimedia\Rdbms\Database::startAtomic
+        * @covers \Wikimedia\Rdbms\Database::endAtomic
+        * @covers \Wikimedia\Rdbms\Database::cancelAtomic
+        * @covers \Wikimedia\Rdbms\Database::doAtomicSection
+        */
+       public function testAtomicSectionsCallbackCancellation() {
+               $fname = __METHOD__;
+               $callback1Called = null;
+               $callback1 = function ( $trigger = '-' ) use ( $fname, &$callback1Called ) {
+                       $callback1Called = $trigger;
+                       $this->database->query( "SELECT 1", $fname );
+               };
+               $callback2Called = null;
+               $callback2 = function ( $trigger = '-' ) use ( $fname, &$callback2Called ) {
+                       $callback2Called = $trigger;
+                       $this->database->query( "SELECT 2", $fname );
+               };
+               $callback3Called = null;
+               $callback3 = function ( $trigger = '-' ) use ( $fname, &$callback3Called ) {
+                       $callback3Called = $trigger;
+                       $this->database->query( "SELECT 3", $fname );
+               };
+
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__ . '_inner' );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_inner' );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertNull( $callback1Called );
+               $this->assertNull( $callback2Called );
+               $this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+               // phpcs:ignore Generic.Files.LineLength
+               $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1; COMMIT; SELECT 3' );
+
+               $callback1Called = null;
+               $callback2Called = null;
+               $callback3Called = null;
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__ . '_inner', IDatabase::ATOMIC_CANCELABLE );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_inner' );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertNull( $callback1Called );
+               $this->assertNull( $callback2Called );
+               $this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+               // phpcs:ignore Generic.Files.LineLength
+               $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; SAVEPOINT wikimedia_rdbms_atomic2; RELEASE SAVEPOINT wikimedia_rdbms_atomic2; ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1; COMMIT; SELECT 3' );
+
+               $callback1Called = null;
+               $callback2Called = null;
+               $callback3Called = null;
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $atomicId = $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__ . '_inner' );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__, $atomicId );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertNull( $callback1Called );
+               $this->assertNull( $callback2Called );
+               $this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+
+               $callback1Called = null;
+               $callback2Called = null;
+               $callback3Called = null;
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $atomicId = $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__ . '_inner' );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               try {
+                       $this->database->cancelAtomic( __METHOD__ . '_X', $atomicId );
+               } catch ( DBUnexpectedError $e ) {
+                       $m = __METHOD__;
+                       $this->assertSame(
+                               "Invalid atomic section ended (got {$m}_X but expected {$m}).",
+                               $e->getMessage()
+                       );
+               }
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertNull( $callback1Called );
+               $this->assertNull( $callback2Called );
+               $this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__ . '_inner' );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               $this->database->cancelAtomic( __METHOD__ . '_inner' );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertNull( $callback1Called );
+               $this->assertNull( $callback2Called );
+               $this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+
+               $wrapper = TestingAccessWrapper::newFromObject( $this->database );
+               $callback1Called = null;
+               $callback2Called = null;
+               $callback3Called = null;
+               $this->database->startAtomic( __METHOD__ . '_outer' );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->startAtomic( __METHOD__ . '_inner' );
+               $this->database->onTransactionIdle( $callback1, __METHOD__ );
+               $this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+               $this->database->onTransactionResolution( $callback3, __METHOD__ );
+               $wrapper->trxStatus = Database::STATUS_TRX_ERROR;
+               $this->database->cancelAtomic( __METHOD__ . '_inner' );
+               $this->database->cancelAtomic( __METHOD__ );
+               $this->database->endAtomic( __METHOD__ . '_outer' );
+               $this->assertNull( $callback1Called );
+               $this->assertNull( $callback2Called );
+               $this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+       }
+
+       /**
+        * @covers \Wikimedia\Rdbms\Database::doSavepoint
+        * @covers \Wikimedia\Rdbms\Database::doReleaseSavepoint
+        * @covers \Wikimedia\Rdbms\Database::doRollbackToSavepoint
+        * @covers \Wikimedia\Rdbms\Database::startAtomic
+        * @covers \Wikimedia\Rdbms\Database::endAtomic
+        * @covers \Wikimedia\Rdbms\Database::cancelAtomic
+        * @covers \Wikimedia\Rdbms\Database::doAtomicSection
+        */
+       public function testAtomicSectionsTrxRound() {
+               $this->database->setFlag( IDatabase::DBO_TRX );
+               $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+               $this->database->query( 'SELECT 1', __METHOD__ );
+               $this->database->endAtomic( __METHOD__ );
+               $this->database->commit( __METHOD__, IDatabase::FLUSHING_ALL_PEERS );
+               // phpcs:ignore Generic.Files.LineLength
+               $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; SELECT 1; RELEASE SAVEPOINT wikimedia_rdbms_atomic1; COMMIT' );
+       }
+
        public static function provideAtomicSectionMethodsForErrors() {
                return [
                        [ 'endAtomic' ],

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index 94b4e0d..e328cba 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -310,14 +310,18 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
                $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' );
 
+               $this->assertFalse( $lb->hasMasterChanges() );
+               $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
                $called = false;
                $callback = function () use ( &$called ) {
                        $called = true;
                };
                $db->onTransactionPreCommitOrIdle( $callback, __METHOD__ );
                $this->assertTrue( $called, 'Called when idle if DBO_TRX is set' );
+               $called = false;
+               $lbFactory->commitMasterChanges();
+               $this->assertFalse( $called );
 
                $called = false;
                $lbFactory->beginMasterChanges( __METHOD__ );