Merge "Move user_editcount updates to a mergeable deferred update"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Fri, 26 Oct 2018 20:32:24 +0000 (20:32 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Fri, 26 Oct 2018 20:32:24 +0000 (20:32 +0000)
autoload.php
includes/deferred/MergeableUpdate.php
includes/deferred/UserEditCountUpdate.php [new file with mode: 0644]
includes/user/User.php
tests/phpunit/includes/api/ApiStashEditTest.php
tests/phpunit/includes/user/UserTest.php

index f951ce9..6ccfc86 100644 (file)
@@ -1543,6 +1543,7 @@ $wgAutoloadLocalClasses = [
        'UserBlockedError' => __DIR__ . '/includes/exception/UserBlockedError.php',
        'UserCache' => __DIR__ . '/includes/cache/UserCache.php',
        'UserDupes' => __DIR__ . '/maintenance/userDupes.inc',
+       'UserEditCountUpdate' => __DIR__ . '/includes/deferred/UserEditCountUpdate.php',
        'UserGroupExpiryJob' => __DIR__ . '/includes/jobqueue/jobs/UserGroupExpiryJob.php',
        'UserGroupMembership' => __DIR__ . '/includes/user/UserGroupMembership.php',
        'UserMailer' => __DIR__ . '/includes/mail/UserMailer.php',
index 8eeef13..6ae2bcc 100644 (file)
@@ -1,8 +1,17 @@
 <?php
 
 /**
- * Interface that deferrable updates can implement. DeferredUpdates uses this to merge
- * all pending updates of PHP class into a single update by calling merge().
+ * Interface that deferrable updates can implement to signal that updates can be combined.
+ *
+ * DeferredUpdates uses this to merge all pending updates of PHP class into a single update
+ * by calling merge(). Note that upon merge(), the combined update goes to the back of the FIFO
+ * queue so that such updates occur after related non-mergeable deferred updates. For example,
+ * suppose updates that purge URLs can be merged, and the calling pattern is:
+ *   - a) DeferredUpdates::addUpdate( $purgeCdnUrlsA );
+ *   - b) DeferredUpdates::addUpdate( $deleteContentUrlsB );
+ *   - c) DeferredUpdates::addUpdate( $purgeCdnUrlsB )
+ *
+ * The purges for urls A and B will all happen after the $deleteContentUrlsB update.
  *
  * @since 1.27
  */
diff --git a/includes/deferred/UserEditCountUpdate.php b/includes/deferred/UserEditCountUpdate.php
new file mode 100644 (file)
index 0000000..2a1205c
--- /dev/null
@@ -0,0 +1,113 @@
+<?php
+/**
+ * User edit count incrementing.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+use Wikimedia\Assert\Assert;
+use MediaWiki\MediaWikiServices;
+
+/**
+ * Handles increment the edit count for a given set of users
+ */
+class UserEditCountUpdate implements DeferrableUpdate, MergeableUpdate {
+       /** @var array[] Map of (user ID => ('increment': int, 'instances': User[])) */
+       private $infoByUser;
+
+       /**
+        * @param User $user
+        * @param int $increment
+        */
+       public function __construct( User $user, $increment ) {
+               if ( !$user->getId() ) {
+                       throw new RuntimeException( "Got user ID of zero" );
+               }
+               $this->infoByUser = [
+                       $user->getId() => [ 'increment' => $increment, 'instances' => [ $user ] ]
+               ];
+       }
+
+       public function merge( MergeableUpdate $update ) {
+               /** @var UserEditCountUpdate $update */
+               Assert::parameterType( __CLASS__, $update, '$update' );
+
+               foreach ( $update->infoByUser as $userId => $info ) {
+                       if ( !isset( $this->infoByUser[$userId] ) ) {
+                               $this->infoByUser[$userId] = [ 'increment' => 0, 'instances' => [] ];
+                       }
+                       // Merge the increment amount
+                       $this->infoByUser[$userId]['increment'] += $info['increment'];
+                       // Merge the list of User instances to update in doUpdate()
+                       foreach ( $info['instances'] as $user ) {
+                               if ( !in_array( $user, $this->infoByUser[$userId]['instances'], true ) ) {
+                                       $this->infoByUser[$userId]['instances'][] = $user;
+                               }
+                       }
+               }
+       }
+
+       /**
+        * Purges the list of URLs passed to the constructor.
+        */
+       public function doUpdate() {
+               foreach ( $this->infoByUser as $userId => $info ) {
+                       $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
+                       $dbw = $lb->getConnection( DB_MASTER );
+
+                       $dbw->startAtomic( __METHOD__ ); // define minimum row lock duration
+                       $dbw->update(
+                               'user',
+                               [ 'user_editcount=user_editcount+' . (int)$info['increment'] ],
+                               [ 'user_id' => $userId, 'user_editcount IS NOT NULL' ],
+                               __METHOD__
+                       );
+                       /** @var User[] $affectedInstances */
+                       $affectedInstances = $info['instances'];
+                       // Lazy initialization check...
+                       if ( $dbw->affectedRows() == 0 ) {
+                               // No rows will be "affected" if user_editcount is NULL.
+                               // Get the generic "replica" connection to see if it actually uses the master.
+                               $dbr = $lb->getConnection( DB_REPLICA );
+                               if ( $dbr !== $dbw ) {
+                                       // This method runs after the new revisions were committed.
+                                       // Wait for the replica to catch up so they will all be counted.
+                                       $dbr->flushSnapshot( __METHOD__ );
+                                       $lb->safeWaitForMasterPos( $dbr );
+                               }
+                               $affectedInstances[0]->initEditCountInternal();
+                       }
+                       $newCount = (int)$dbw->selectField(
+                               'user',
+                               [ 'user_editcount' ],
+                               [ 'user_id' => $userId ],
+                               __METHOD__
+                       );
+                       $dbw->endAtomic( __METHOD__ );
+
+                       // Update the edit count in the instance caches. This is mostly useful
+                       // for maintenance scripts, where deferred updates might run immediately
+                       // and user instances might be reused for a long time.
+                       foreach ( $affectedInstances as $affectedInstance ) {
+                               $affectedInstance->setEditCountInternal( $newCount );
+                       }
+                       // Clear the edit count in user cache too
+                       $affectedInstances[0]->invalidateCache();
+               }
+       }
+}
index 9b08737..9cd3ea1 100644 (file)
@@ -3698,7 +3698,7 @@ class User implements IDBAccessObject, UserIdentity {
 
                        if ( $count === null ) {
                                // it has not been initialized. do so.
-                               $count = $this->initEditCount();
+                               $count = $this->initEditCountInternal();
                        }
                        $this->mEditCount = $count;
                }
@@ -5323,73 +5323,37 @@ class User implements IDBAccessObject, UserIdentity {
        }
 
        /**
-        * Deferred version of incEditCountImmediate()
-        *
-        * This function, rather than incEditCountImmediate(), should be used for
-        * most cases as it avoids potential deadlocks caused by concurrent editing.
+        * Schedule a deferred update to update the user's edit count
         */
        public function incEditCount() {
-               wfGetDB( DB_MASTER )->onTransactionPreCommitOrIdle(
-                       function () {
-                               $this->incEditCountImmediate();
-                       },
-                       __METHOD__
+               if ( $this->isAnon() ) {
+                       return; // sanity
+               }
+
+               DeferredUpdates::addUpdate(
+                       new UserEditCountUpdate( $this, 1 ),
+                       DeferredUpdates::POSTSEND
                );
        }
 
        /**
-        * Increment the user's edit-count field.
-        * Will have no effect for anonymous users.
-        * @since 1.26
+        * This method should not be called outside User/UserEditCountUpdate
+        *
+        * @param int $count
         */
-       public function incEditCountImmediate() {
-               if ( $this->isAnon() ) {
-                       return;
-               }
-
-               $dbw = wfGetDB( DB_MASTER );
-               // No rows will be "affected" if user_editcount is NULL
-               $dbw->update(
-                       'user',
-                       [ 'user_editcount=user_editcount+1' ],
-                       [ 'user_id' => $this->getId(), 'user_editcount IS NOT NULL' ],
-                       __METHOD__
-               );
-               // Lazy initialization check...
-               if ( $dbw->affectedRows() == 0 ) {
-                       // Now here's a goddamn hack...
-                       $dbr = wfGetDB( DB_REPLICA );
-                       if ( $dbr !== $dbw ) {
-                               // If we actually have a replica DB server, the count is
-                               // at least one behind because the current transaction
-                               // has not been committed and replicated.
-                               $this->mEditCount = $this->initEditCount( 1 );
-                       } else {
-                               // But if DB_REPLICA is selecting the master, then the
-                               // count we just read includes the revision that was
-                               // just added in the working transaction.
-                               $this->mEditCount = $this->initEditCount();
-                       }
-               } else {
-                       if ( $this->mEditCount === null ) {
-                               $this->getEditCount();
-                               $dbr = wfGetDB( DB_REPLICA );
-                               $this->mEditCount += ( $dbr !== $dbw ) ? 1 : 0;
-                       } else {
-                               $this->mEditCount++;
-                       }
-               }
-               // Edit count in user cache too
-               $this->invalidateCache();
+       public function setEditCountInternal( $count ) {
+               $this->mEditCount = $count;
        }
 
        /**
         * Initialize user_editcount from data out of the revision table
         *
+        * This method should not be called outside User/UserEditCountUpdate
+        *
         * @param int $add Edits to add to the count from the revision table
         * @return int Number of edits
         */
-       protected function initEditCount( $add = 0 ) {
+       public function initEditCountInternal( $add = 0 ) {
                // Pull from a replica DB to be less cruel to servers
                // Accuracy isn't the point anyway here
                $dbr = wfGetDB( DB_REPLICA );
index 61512d5..5ef3b04 100644 (file)
@@ -364,6 +364,7 @@ class ApiStashEditTest extends ApiTestCase {
                // Now let's also increment our editcount
                $this->editPage( ucfirst( __FUNCTION__ ), '' );
 
+               $user->clearInstanceCache();
                $this->assertFalse( $this->doCheckCache( $user ),
                        "Cache should be invalidated when it's old and the user has an intervening edit" );
        }
index 1e1f739..55b3bfc 100644 (file)
@@ -263,6 +263,7 @@ class UserTest extends MediaWikiTestCase {
 
                // increase the edit count
                $user->incEditCount();
+               $user->clearInstanceCache();
 
                $this->assertEquals(
                        4,