Alter two uses of hasOrMadeRecentMasterChanges() for consistency

Rather than have the behavior vary and possibly break code or tests
when small changes happen, make User/NameTableStore more explicit
about when cache key purges happens.

This should reduce problems with certain fragile tests, such as those
that could be affected by 03908112635f when --use-normal tables is not
used. Ideally, any fragility should be ironed out of effected code.

Change-Id: Ibe5d1bb4bece2526bc0da99648f7ba73bdc0ffa5

diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php
index bf88b20..a14e339 100644 (file)
--- a/includes/Storage/NameTableStore.php
+++ b/includes/Storage/NameTableStore.php
@@ -183,11 +183,10 @@ class NameTableStore {
                                $searchResult = $id;
 
                                // As store returned an ID we know we inserted so delete from WAN cache
-                               $this->purgeWANCache(
-                                       function () {
-                                               $this->cache->delete( $this->getCacheKey() );
-                                       }
-                               );
+                               $dbw = $this->getDBConnection( DB_MASTER );
+                               $dbw->onTransactionPreCommitOrIdle( function () {
+                                       $this->cache->delete( $this->getCacheKey() );
+                               } );
                        }
                        $this->tableCache = $table;
                }
@@ -208,14 +207,11 @@ class NameTableStore {
         * @return string[] The freshly reloaded name map
         */
        public function reloadMap( $connFlags = 0 ) {
-               $this->tableCache = $this->loadTable(
-                       $this->getDBConnection( DB_MASTER, $connFlags )
-               );
-               $this->purgeWANCache(
-                       function () {
-                               $this->cache->reap( $this->getCacheKey(), INF );
-                       }
-               );
+               $dbw = $this->getDBConnection( DB_MASTER, $connFlags );
+               $this->tableCache = $this->loadTable( $dbw );
+               $dbw->onTransactionPreCommitOrIdle( function () {
+                       $this->cache->reap( $this->getCacheKey(), INF );
+               } );
 
                return $this->tableCache;
        }
@@ -342,22 +338,6 @@ class NameTableStore {
                return $table;
        }
 
-       /**
-        * Reap the WANCache entry for this table.
-        *
-        * @param callable $purgeCallback Callback to 'purge' the WAN cache
-        */
-       private function purgeWANCache( $purgeCallback ) {
-               // If the LB has no DB changes don't bother with onTransactionPreCommitOrIdle
-               if ( !$this->loadBalancer->hasOrMadeRecentMasterChanges() ) {
-                       $purgeCallback();
-                       return;
-               }
-
-               $this->getDBConnection( DB_MASTER )
-                       ->onTransactionPreCommitOrIdle( $purgeCallback, __METHOD__ );
-       }
-
        /**
         * Gets the table from the db
         *

diff --git a/includes/user/User.php b/includes/user/User.php
index 33d216d..ba6bb08 100644 (file)
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1706,7 +1706,7 @@ class User implements IDBAccessObject, UserIdentity {
 
                if ( $success ) {
                        $this->mTouched = $newTouched;
-                       $this->clearSharedCache();
+                       $this->clearSharedCache( 'changed' );
                } else {
                        // Clears on failure too since that is desired if the cache is stale
                        $this->clearSharedCache( 'refresh' );
@@ -2762,29 +2762,26 @@ class User implements IDBAccessObject, UserIdentity {
         *
         * Called implicitly from invalidateCache() and saveSettings().
         *
-        * @param string $mode Use 'refresh' to clear now; otherwise before DB commit
+        * @param string $mode Use 'refresh' to clear now or 'changed' to clear before DB commit
         */
-       public function clearSharedCache( $mode = 'changed' ) {
+       public function clearSharedCache( $mode = 'refresh' ) {
                if ( !$this->getId() ) {
                        return;
                }
 
+               $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
                $cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
                $key = $this->getCacheKey( $cache );
+
                if ( $mode === 'refresh' ) {
-                       $cache->delete( $key, 1 );
+                       $cache->delete( $key, 1 ); // low tombstone/"hold-off" TTL
                } else {
-                       $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
-                       if ( $lb->hasOrMadeRecentMasterChanges() ) {
-                               $lb->getConnection( DB_MASTER )->onTransactionPreCommitOrIdle(
-                                       function () use ( $cache, $key ) {
-                                               $cache->delete( $key );
-                                       },
-                                       __METHOD__
-                               );
-                       } else {
-                               $cache->delete( $key );
-                       }
+                       $lb->getConnection( DB_MASTER )->onTransactionPreCommitOrIdle(
+                               function () use ( $cache, $key ) {
+                                       $cache->delete( $key );
+                               },
+                               __METHOD__
+                       );
                }
        }
 
@@ -2795,7 +2792,7 @@ class User implements IDBAccessObject, UserIdentity {
         */
        public function invalidateCache() {
                $this->touch();
-               $this->clearSharedCache();
+               $this->clearSharedCache( 'changed' );
        }
 
        /**
@@ -4246,7 +4243,7 @@ class User implements IDBAccessObject, UserIdentity {
                $this->saveOptions();
 
                Hooks::run( 'UserSaveSettings', [ $this ] );
-               $this->clearSharedCache();
+               $this->clearSharedCache( 'changed' );
                $this->getUserPage()->purgeSquid();
        }
 

diff --git a/tests/phpunit/includes/Storage/NameTableStoreTest.php b/tests/phpunit/includes/Storage/NameTableStoreTest.php
index f42e557..ca87b49 100644 (file)
--- a/tests/phpunit/includes/Storage/NameTableStoreTest.php
+++ b/tests/phpunit/includes/Storage/NameTableStoreTest.php
@@ -80,6 +80,14 @@ class NameTableStoreTest extends MediaWikiTestCase {
                        ->willReturnCallback( function ( ...$args ) {
                                return call_user_func_array( [ $this->db, 'insertId' ], $args );
                        } );
+               $mock->expects( $this->any() )
+                       ->method( 'query' )
+                       ->willReturn( [] );
+               $mock->expects( $this->any() )
+                       ->method( 'isOpen' )
+                       ->willReturn( true );
+               $wrapper = TestingAccessWrapper::newFromObject( $mock );
+               $wrapper->queryLogger = new NullLogger();
                return $mock;
        }