Merge "Make doAtomicSection() return the callback result"

diff --combined includes/db/Database.php
index d492def,2041aca..940c3f7
--- 1/includes/db/Database.php
--- 2/includes/db/Database.php
+++ b/includes/db/Database.php
@@@ -815,7 -815,7 +815,7 @@@ abstract class DatabaseBase implements 
                if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX )
                        && $this->isTransactableQuery( $sql )
                ) {
 -                      $this->begin( __METHOD__ . " ($fname)" );
 +                      $this->begin( __METHOD__ . " ($fname)", self::TRANSACTION_INTERNAL );
                        $this->mTrxAutomatic = true;
                }
  
@@@ -2203,7 -2203,7 +2203,7 @@@
  
                $useTrx = !$this->mTrxLevel;
                if ( $useTrx ) {
 -                      $this->begin( $fname );
 +                      $this->begin( $fname, self::TRANSACTION_INTERNAL );
                }
                try {
                        # Update any existing conflicting row(s)
@@@ -2221,7 -2221,7 +2221,7 @@@
                        throw $e;
                }
                if ( $useTrx ) {
 -                      $this->commit( $fname );
 +                      $this->commit( $fname, self::TRANSACTION_INTERNAL );
                }
  
                return $ok;
@@@ -2520,7 -2520,7 +2520,7 @@@
                        $this->mTrxPreCommitCallbacks[] = [ $callback, wfGetCaller() ];
                } else {
                        // If no transaction is active, then make one for this callback
 -                      $this->begin( __METHOD__ );
 +                      $this->begin( __METHOD__, self::TRANSACTION_INTERNAL );
                        try {
                                call_user_func( $callback );
                                $this->commit( __METHOD__ );
@@@ -2628,7 -2628,7 +2628,7 @@@
  
        final public function startAtomic( $fname = __METHOD__ ) {
                if ( !$this->mTrxLevel ) {
 -                      $this->begin( $fname );
 +                      $this->begin( $fname, self::TRANSACTION_INTERNAL );
                        $this->mTrxAutomatic = true;
                        // If DBO_TRX is set, a series of startAtomic/endAtomic pairs will result
                        // in all changes being in one transaction to keep requests transactional.
@@@ -2658,34 -2658,53 +2658,36 @@@
        final public function doAtomicSection( $fname, callable $callback ) {
                $this->startAtomic( $fname );
                try {
-                       call_user_func_array( $callback, [ $this, $fname ] );
+                       $res = call_user_func_array( $callback, [ $this, $fname ] );
                } catch ( Exception $e ) {
                        $this->rollback( $fname );
                        throw $e;
                }
                $this->endAtomic( $fname );
+ 
+               return $res;
        }
  
 -      final public function begin( $fname = __METHOD__ ) {
 -              if ( $this->mTrxLevel ) { // implicit commit
 +      final public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT ) {
 +              // Protect against mismatched atomic section, transaction nesting, and snapshot loss
 +              if ( $this->mTrxLevel ) {
                        if ( $this->mTrxAtomicLevels ) {
 -                              // If the current transaction was an automatic atomic one, then we definitely have
 -                              // a problem. Same if there is any unclosed atomic level.
                                $levels = implode( ', ', $this->mTrxAtomicLevels );
 -                              throw new DBUnexpectedError(
 -                                      $this,
 -                                      "Got explicit BEGIN from $fname while atomic section(s) $levels are open."
 -                              );
 +                              $msg = "Got explicit BEGIN from $fname while atomic section(s) $levels are open.";
 +                              throw new DBUnexpectedError( $this, $msg );
                        } elseif ( !$this->mTrxAutomatic ) {
 -                              // We want to warn about inadvertently nested begin/commit pairs, but not about
 -                              // auto-committing implicit transactions that were started by query() via DBO_TRX
 -                              throw new DBUnexpectedError(
 -                                      $this,
 -                                      "$fname: Transaction already in progress (from {$this->mTrxFname}), " .
 -                                              " performing implicit commit!"
 -                              );
 -                      } elseif ( $this->mTrxDoneWrites ) {
 -                              // The transaction was automatic and has done write operations
 -                              throw new DBUnexpectedError(
 -                                      $this,
 -                                      "$fname: Automatic transaction with writes in progress" .
 -                                              " (from {$this->mTrxFname}), performing implicit commit!\n"
 -                              );
 -                      }
 -
 -                      $this->runOnTransactionPreCommitCallbacks();
 -                      $writeTime = $this->pendingWriteQueryDuration();
 -                      $this->doCommit( $fname );
 -                      if ( $this->mTrxDoneWrites ) {
 -                              $this->mDoneWrites = microtime( true );
 -                              $this->getTransactionProfiler()->transactionWritingOut(
 -                                      $this->mServer, $this->mDBname, $this->mTrxShortId, $writeTime );
 +                              $msg = "$fname: Explicit transaction already active (from {$this->mTrxFname}).";
 +                              throw new DBUnexpectedError( $this, $msg );
 +                      } else {
 +                              // @TODO: make this an exception at some point
 +                              $msg = "$fname: Implicit transaction already active (from {$this->mTrxFname}).";
 +                              wfLogDBError( $msg );
 +                              return; // join the main transaction set
                        }
 -
 -                      $this->runOnTransactionIdleCallbacks( self::TRIGGER_COMMIT );
 +              } elseif ( $this->getFlag( DBO_TRX ) && $mode !== self::TRANSACTION_INTERNAL ) {
 +                      // @TODO: make this an exception at some point
 +                      wfLogDBError( "$fname: Implicit transaction expected (DBO_TRX set)." );
 +                      return; // let any writes be in the main transaction
                }
  
                // Avoid fatals if close() was called
@@@ -2725,7 -2744,7 +2727,7 @@@
                        $levels = implode( ', ', $this->mTrxAtomicLevels );
                        throw new DBUnexpectedError(
                                $this,
 -                              "Got COMMIT while atomic sections $levels are still open"
 +                              "Got COMMIT while atomic sections $levels are still open."
                        );
                }
  
@@@ -2735,17 -2754,18 +2737,17 @@@
                        } elseif ( !$this->mTrxAutomatic ) {
                                throw new DBUnexpectedError(
                                        $this,
 -                                      "$fname: Flushing an explicit transaction, getting out of sync!"
 +                                      "$fname: Flushing an explicit transaction, getting out of sync."
                                );
                        }
                } else {
                        if ( !$this->mTrxLevel ) {
 -                              wfWarn( "$fname: No transaction to commit, something got out of sync!" );
 +                              wfWarn( "$fname: No transaction to commit, something got out of sync." );
                                return; // nothing to do
                        } elseif ( $this->mTrxAutomatic ) {
 -                              throw new DBUnexpectedError(
 -                                      $this,
 -                                      "$fname: Explicit commit of implicit transaction."
 -                              );
 +                              // @TODO: make this an exception at some point
 +                              wfLogDBError( "$fname: Explicit commit of implicit transaction." );
 +                              return; // wait for the main transaction set commit round
                        }
                }
  
@@@ -2778,19 -2798,14 +2780,19 @@@
        }
  
        final public function rollback( $fname = __METHOD__, $flush = '' ) {
 -              if ( $flush !== self::FLUSHING_INTERNAL && $flush !== self::FLUSHING_ALL_PEERS ) {
 +              if ( $flush === self::FLUSHING_INTERNAL || $flush === self::FLUSHING_ALL_PEERS ) {
                        if ( !$this->mTrxLevel ) {
 -                              wfWarn( "$fname: No transaction to rollback, something got out of sync!" );
                                return; // nothing to do
                        }
                } else {
                        if ( !$this->mTrxLevel ) {
 +                              wfWarn( "$fname: No transaction to rollback, something got out of sync." );
                                return; // nothing to do
 +                      } elseif ( $this->getFlag( DBO_TRX ) ) {
 +                              throw new DBUnexpectedError(
 +                                      $this,
 +                                      "$fname: Expected mass rollback of all peer databases (DBO_TRX set)."
 +                              );
                        }
                }
  

diff --combined includes/db/IDatabase.php
index fce5aa8,a871082..772d824
--- 1/includes/db/IDatabase.php
--- 2/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@@ -40,11 -40,6 +40,11 @@@ interface IDatabase 
        /** @var int Callback triggered by rollback */
        const TRIGGER_ROLLBACK = 3;
  
 +      /** @var string Transaction is requested by regular caller outside of the DB layer */
 +      const TRANSACTION_EXPLICIT = '';
 +      /** @var string Transaction is requested interally via DBO_TRX/startAtomic() */
 +      const TRANSACTION_INTERNAL = 'implicit';
 +
        /** @var string Transaction operation comes from service managing all DBs */
        const FLUSHING_ALL_PEERS = 'flush';
        /** @var string Transaction operation comes from the database class internally */
@@@ -1347,6 -1342,7 +1347,7 @@@
         *
         * @param string $fname Caller name (usually __METHOD__)
         * @param callable $callback Callback that issues DB updates
+        * @return mixed $res Result of the callback (since 1.28)
         * @throws DBError
         * @throws RuntimeException
         * @throws UnexpectedValueException
@@@ -1367,10 -1363,9 +1368,10 @@@
         * automatically because of the DBO_TRX flag.
         *
         * @param string $fname
 +       * @param string $mode A situationally valid IDatabase::TRANSACTION_* constant [optional]
         * @throws DBError
         */
 -      public function begin( $fname = __METHOD__ );
 +      public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT );
  
        /**
         * Commits a transaction previously started using begin().