Updated key WANObjectCache::delete() callers to avoid races

* They now issue the delete() write before COMMIT of
  the relevant DB (or immediately if no trx is active)
* This can avoid some stale write race conditions
* Updated the WAN cache delete() docs

Change-Id: Id54887976051120b76528070d5f2ceb357d57897

diff --git a/includes/User.php b/includes/User.php
index d57dfaa..753061d 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -2290,10 +2290,14 @@ class User implements IDBAccessObject {
         */
        public function clearSharedCache() {
                $id = $this->getId();
-               if ( $id ) {
-                       $key = wfMemcKey( 'user', 'id', $id );
-                       ObjectCache::getMainWANInstance()->delete( $key );
+               if ( !$id ) {
+                       return;
                }
+
+               $key = wfMemcKey( 'user', 'id', $id );
+               wfGetDB( DB_MASTER )->onTransactionPreCommitOrIdle( function() use ( $key ) {
+                       ObjectCache::getMainWANInstance()->delete( $key );
+               } );
        }
 
        /**

diff --git a/includes/UserRightsProxy.php b/includes/UserRightsProxy.php
index a19f698..2c7032f 100644 (file)
--- a/includes/UserRightsProxy.php
+++ b/includes/UserRightsProxy.php
@@ -278,8 +278,9 @@ class UserRightsProxy {
                        array( 'user_id' => $this->id ),
                        __METHOD__ );
 
-               $cache = ObjectCache::getMainWANInstance();
                $key = wfForeignMemcKey( $this->database, false, 'user', 'id', $this->id );
-               $cache->delete( $key );
+               $this->db->onTransactionPreCommitOrIdle( function() use ( $key ) {
+                       ObjectCache::getMainWANInstance()->delete( $key );
+               } );
        }
 }

diff --git a/includes/filerepo/LocalRepo.php b/includes/filerepo/LocalRepo.php
index 6a2c064..b209bd6 100644 (file)
--- a/includes/filerepo/LocalRepo.php
+++ b/includes/filerepo/LocalRepo.php
@@ -521,15 +521,11 @@ class LocalRepo extends FileRepo {
         * @return void
         */
        function invalidateImageRedirect( Title $title ) {
-               $cache = ObjectCache::getMainWANInstance();
-
-               $memcKey = $this->getSharedCacheKey( 'image_redirect', md5( $title->getDBkey() ) );
-               if ( $memcKey ) {
-                       // Set a temporary value for the cache key, to ensure
-                       // that this value stays purged long enough so that
-                       // it isn't refreshed with a stale value due to a
-                       // lagged slave.
-                       $cache->delete( $memcKey, 12 );
+               $key = $this->getSharedCacheKey( 'image_redirect', md5( $title->getDBkey() ) );
+               if ( $key ) {
+                       $this->getMasterDB()->onTransactionPreCommitOrIdle( function() use ( $key ) {
+                               ObjectCache::getMainWANInstance()->delete( $key );
+                       } );
                }
        }
 

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index 4c9c2aa..3225d78 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -321,7 +321,9 @@ class LocalFile extends File {
                        return;
                }
 
-               ObjectCache::getMainWANInstance()->delete( $key );
+               $this->repo->getMasterDB()->onTransactionPreCommitOrIdle( function() use ( $key ) {
+                       ObjectCache::getMainWANInstance()->delete( $key );
+               } );
        }
 
        /**

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index fb96269..c78b299 100644 (file)
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -286,6 +286,31 @@ class WANObjectCache {
         *   - a) Replication lag is bounded to being less than HOLDOFF_TTL; or
         *   - b) If lag is higher, the DB will have gone into read-only mode already
         *
+        * When using potentially long-running ACID transactions, a good pattern is
+        * to use a pre-commit hook to issue the delete. This means that immediately
+        * after commit, callers will see the tombstone in cache in the local datacenter
+        * and in the others upon relay. It also avoids the following race condition:
+        *   - a) T1 begins, changes a row, and calls delete()
+        *   - b) The HOLDOFF_TTL passes, expiring the delete() tombstone
+        *   - c) T2 starts, reads the row and calls set() due to a cache miss
+        *   - d) T1 finally commits
+        *   - e) Stale value is stuck in cache
+        *
+        * Example usage:
+        * @code
+        *     $dbw->begin(); // start of request
+        *     ... <execute some stuff> ...
+        *     // Update the row in the DB
+        *     $dbw->update( ... );
+        *     $key = wfMemcKey( 'homes', $homeId );
+        *     // Purge the corresponding cache entry just before committing
+        *     $dbw->onTransactionPreCommitOrIdle( function() use ( $cache, $key ) {
+        *         $cache->delete( $key );
+        *     } );
+        *     ... <execute some stuff> ...
+        *     $dbw->commit(); // end of request
+        * @endcode
+        *
         * If called twice on the same key, then the last hold-off TTL takes
         * precedence. For idempotence, the $ttl should not vary for different
         * delete() calls on the same key. Also note that lowering $ttl reduces