rdbms: add more comments and sanity checks for CONN_TRX_AUTOCOMMIT

Change-Id: I69992cf2e2ae3ef62125b0bc733a0cb7274f814e

diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php
index bc73718..689326e 100644 (file)
--- a/includes/jobqueue/JobQueueDB.php
+++ b/includes/jobqueue/JobQueueDB.php
@@ -221,7 +221,7 @@ class JobQueueDB extends JobQueue {
                $rowSet = []; // (sha1 => job) map for jobs that are de-duplicated
                $rowList = []; // list of jobs for jobs that are not de-duplicated
                foreach ( $jobs as $job ) {
-                       $row = $this->insertFields( $job );
+                       $row = $this->insertFields( $job, $dbw );
                        if ( $job->ignoreDuplicates() ) {
                                $rowSet[$row['job_sha1']] = $row;
                        } else {
@@ -722,11 +722,10 @@ class JobQueueDB extends JobQueue {
 
        /**
         * @param IJobSpecification $job
+        * @param IDatabase $db
         * @return array
         */
-       protected function insertFields( IJobSpecification $job ) {
-               $dbw = $this->getMasterDB();
-
+       protected function insertFields( IJobSpecification $job, IDatabase $db ) {
                return [
                        // Fields that describe the nature of the job
                        'job_cmd' => $job->getType(),
@@ -734,7 +733,7 @@ class JobQueueDB extends JobQueue {
                        'job_title' => $job->getTitle()->getDBkey(),
                        'job_params' => self::makeBlob( $job->getParams() ),
                        // Additional job metadata
-                       'job_timestamp' => $dbw->timestamp(),
+                       'job_timestamp' => $db->timestamp(),
                        'job_sha1' => Wikimedia\base_convert(
                                sha1( serialize( $job->getDeduplicationInfo() ) ),
                                16, 36, 31

diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
index 0ff6a32..83ebd51 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
@@ -187,7 +187,8 @@ interface ILoadBalancer {
        /**
         * Get any open connection to a given server index, local or foreign
         *
-        * Use CONN_TRX_AUTOCOMMIT to only look for connections opened with that flag
+        * Use CONN_TRX_AUTOCOMMIT to only look for connections opened with that flag.
+        * Avoid the use of begin() or startAtomic() on any such connections.
         *
         * @param int $i Server index or DB_MASTER/DB_REPLICA
         * @param int $flags Bitfield of CONN_* class constants
@@ -202,9 +203,10 @@ interface ILoadBalancer {
         * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
         * can be used to check such flags beforehand.
         *
-        * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must also
-        * call ILoadBalancer::reuseConnection() on the handle when finished using it.
+        * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must
+        * also call ILoadBalancer::reuseConnection() on the handle when finished using it.
         * In all other cases, this is not necessary, though not harmful either.
+        * Avoid the use of begin() or startAtomic() on any such connections.
         *
         * @param int $i Server index (overrides $groups) or DB_MASTER/DB_REPLICA
         * @param array|string|bool $groups Query group(s), or false for the generic reader
@@ -236,7 +238,8 @@ interface ILoadBalancer {
         *
         * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
         * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
-        * can be used to check such flags beforehand.
+        * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
+        * on any CONN_TRX_AUTOCOMMIT connections.
         *
         * @see ILoadBalancer::getConnection() for parameter information
         *
@@ -255,7 +258,8 @@ interface ILoadBalancer {
         *
         * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
         * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
-        * can be used to check such flags beforehand.
+        * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
+        * on any CONN_TRX_AUTOCOMMIT connections.
         *
         * @see ILoadBalancer::getConnection() for parameter information
         *
@@ -274,7 +278,8 @@ interface ILoadBalancer {
         *
         * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
         * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
-        * can be used to check such flags beforehand.
+        * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
+        * on any CONN_TRX_AUTOCOMMIT connections.
         *
         * @see ILoadBalancer::getConnection() for parameter information
         *
@@ -292,13 +297,14 @@ interface ILoadBalancer {
         * The index must be an actual index into the array. If a connection to the server is
         * already open and not considered an "in use" foreign connection, this simply returns it.
         *
-        * Avoid using CONN_TRX_AUTOCOMMIT for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) in
-        * order to avoid deadlocks. ILoadBalancer::getServerAttributes() can be used to check
+        * Avoid using CONN_TRX_AUTOCOMMIT for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite)
+        * in order to avoid deadlocks. ILoadBalancer::getServerAttributes() can be used to check
         * such flags beforehand.
         *
-        * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must also
-        * call ILoadBalancer::reuseConnection() on the handle when finished using it.
+        * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must
+        * also call ILoadBalancer::reuseConnection() on the handle when finished using it.
         * In all other cases, this is not necessary, though not harmful either.
+        * Avoid the use of begin() or startAtomic() on any such connections.
         *
         * @note This method throws DBAccessError if ILoadBalancer::disable() was called
         *

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index f2f9eb0..92cabca 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -926,6 +926,13 @@ class LoadBalancer implements ILoadBalancer {
                }
 
                if ( $autoCommit && $conn instanceof IDatabase ) {
+                       if ( $conn->trxLevel() ) { // sanity
+                               throw new DBUnexpectedError(
+                                       $conn,
+                                       __METHOD__ . ': CONN_TRX_AUTOCOMMIT handle has a transaction.'
+                               );
+                       }
+
                        $conn->clearFlag( $conn::DBO_TRX ); // auto-commit mode
                }
 

diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php
index 5a748cc..2c4e6b4 100644 (file)
--- a/tests/phpunit/includes/db/LoadBalancerTest.php
+++ b/tests/phpunit/includes/db/LoadBalancerTest.php
@@ -311,6 +311,20 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                $lb->getAnyOpenConnection( $i, $lb::CONN_TRX_AUTOCOMMIT ) );
                        $this->assertEquals( $conn2,
                                $lb->getConnection( $i, [], false, $lb::CONN_TRX_AUTOCOMMIT ) );
+
+                       $conn2->startAtomic( __METHOD__ );
+                       try {
+                               $lb->getConnection( $i, [], false, $lb::CONN_TRX_AUTOCOMMIT );
+                               $conn2->endAtomic( __METHOD__ );
+                               $this->fail( "No exception thrown." );
+                       } catch ( DBUnexpectedError $e ) {
+                               $this->assertEquals(
+                                       'Wikimedia\Rdbms\LoadBalancer::openConnection: ' .
+                                       'CONN_TRX_AUTOCOMMIT handle has a transaction.',
+                                       $e->getMessage()
+                               );
+                       }
+                       $conn2->endAtomic( __METHOD__ );
                }
 
                $lb->closeAll();