From 75f06d529408e5dea33c97398f3aa17584b162d1 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 24 Sep 2015 14:47:30 -0700 Subject: [PATCH] Added $opts to WANObjectCache::set() to detect snapshot lag * This can avoid some stale write race conditions * Made use of this option in a few key places * HOLDOFF_TTL was also bumped Change-Id: I83505a59cce0119e456191c3100f7d97bc86bdbf --- includes/User.php | 3 +- includes/filerepo/LocalRepo.php | 9 ++- includes/filerepo/file/LocalFile.php | 5 +- includes/libs/objectcache/WANObjectCache.php | 79 +++++++++++++++++--- 4 files changed, 80 insertions(+), 16 deletions(-) diff --git a/includes/User.php b/includes/User.php index bb706ef9c2..029f5cf292 100644 --- a/includes/User.php +++ b/includes/User.php @@ -458,7 +458,8 @@ class User implements IDBAccessObject { $data['mVersion'] = self::VERSION; $key = wfMemcKey( 'user', 'id', $this->mId ); - ObjectCache::getMainWANInstance()->set( $key, $data, 3600 ); + $opts = array( 'since' => wfGetDB( DB_SLAVE )->trxTimestamp() ); + ObjectCache::getMainWANInstance()->set( $key, $data, 3600, $opts ); } /** @name newFrom*() static factory methods */ diff --git a/includes/filerepo/LocalRepo.php b/includes/filerepo/LocalRepo.php index 7aa7919081..6e3fb51597 100644 --- a/includes/filerepo/LocalRepo.php +++ b/includes/filerepo/LocalRepo.php @@ -203,6 +203,7 @@ class LocalRepo extends FileRepo { } else { $expiry = 86400; // has invalidation, 1 day } + $cachedValue = $cache->get( $memcKey ); if ( $cachedValue === ' ' || $cachedValue === '' ) { // Does not exist @@ -211,9 +212,11 @@ class LocalRepo extends FileRepo { return Title::newFromText( $cachedValue, NS_FILE ); } // else $cachedValue is false or null: cache miss + $opts = array( 'since' => $this->getSlaveDB()->trxTimestamp() ); + $id = $this->getArticleID( $title ); if ( !$id ) { - $cache->set( $memcKey, " ", $expiry ); + $cache->set( $memcKey, " ", $expiry, $opts ); return false; } @@ -227,11 +230,11 @@ class LocalRepo extends FileRepo { if ( $row && $row->rd_namespace == NS_FILE ) { $targetTitle = Title::makeTitle( $row->rd_namespace, $row->rd_title ); - $cache->set( $memcKey, $targetTitle->getDBkey(), $expiry ); + $cache->set( $memcKey, $targetTitle->getDBkey(), $expiry, $opts ); return $targetTitle; } else { - $cache->set( $memcKey, '', $expiry ); + $cache->set( $memcKey, '', $expiry, $opts ); return false; } diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php index 6745f186c0..d1467084fe 100644 --- a/includes/filerepo/file/LocalFile.php +++ b/includes/filerepo/file/LocalFile.php @@ -308,8 +308,9 @@ class LocalFile extends File { } // Cache presence for 1 week and negatives for 1 day - $cache = ObjectCache::getMainWANInstance(); - $cache->set( $key, $cacheVal, $this->fileExists ? 86400 * 7 : 86400 ); + $ttl = $this->fileExists ? 86400 * 7 : 86400; + $opts = array( 'since' => wfGetDB( DB_SLAVE )->trxTimestamp() ); + ObjectCache::getMainWANInstance()->set( $key, $cacheVal, $ttl, $opts ); } /** diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php index c78b299519..f44bd7b422 100644 --- a/includes/libs/objectcache/WANObjectCache.php +++ b/includes/libs/objectcache/WANObjectCache.php @@ -68,8 +68,15 @@ class WANObjectCache { /** @var int */ protected $lastRelayError = self::ERR_NONE; + /** Max time expected to pass between delete() and DB commit finishing */ + const MAX_COMMIT_DELAY = 1; + /** Max expected replication lag for a reasonable storage setup */ + const MAX_REPLICA_LAG = 7; + /** Max time since snapshot transaction start to avoid no-op of set() */ + const MAX_SNAPSHOT_LAG = 6; /** Seconds to tombstone keys on delete() */ - const HOLDOFF_TTL = 10; + const HOLDOFF_TTL = 14; // MAX_COMMIT_DELAY + MAX_REPLICA_LAG + MAX_SNAPSHOT_LAG + /** Seconds to keep dependency purge keys around */ const CHECK_KEY_TTL = 31536000; // 1 year /** Seconds to keep lock keys around */ @@ -250,13 +257,53 @@ class WANObjectCache { * the changes do not replicate to the other WAN sites. In that case, delete() * should be used instead. This method is intended for use on cache misses. * + * If the data was read from a snapshot-isolated transactions (e.g. the default + * REPEATABLE-READ in innoDB), use 'since' to avoid the following race condition: + * - a) T1 starts + * - b) T2 updates a row, calls delete(), and commits + * - c) The HOLDOFF_TTL passes, expiring the delete() tombstone + * - d) T1 reads the row and calls set() due to a cache miss + * - e) Stale value is stuck in cache + * + * Example usage: + * @code + * $dbr = wfGetDB( DB_SLAVE ); + * // Fetch the row from the DB + * $row = $dbr->selectRow( ... ); + * $key = wfMemcKey( 'building', $buildingId ); + * // Give the age of the transaction snapshot the data came from + * $opts = array( 'since' => $dbr->trxTimestamp() ); + * $cache->set( $key, $row, 86400, $opts ); + * @endcode + * * @param string $key Cache key * @param mixed $value * @param integer $ttl Seconds to live [0=forever] + * @param array $opts Options map: + * - since : UNIX timestamp of the data in $value. Typically, this is either + * the current time the data was read or (if applicable) the time when + * the snapshot-isolated transaction the data was read from started. + * [Default: 0 seconds] + * - lockTSE : if excessive possible snapshot lag is detected, + * then stash the value into a temporary location + * with this TTL. This is only useful if the reads + * use getWithSetCallback() with "lockTSE" set. + * [Default: WANObjectCache::TSE_NONE] * @return bool Success */ - final public function set( $key, $value, $ttl = 0 ) { - $key = self::VALUE_KEY_PREFIX . $key; + final public function set( $key, $value, $ttl = 0, array $opts = array() ) { + $lockTSE = isset( $opts['lockTSE'] ) ? $opts['lockTSE'] : self::TSE_NONE; + $age = isset( $opts['since'] ) ? max( 0, microtime( true ) - $opts['since'] ) : 0; + + if ( $age > self::MAX_SNAPSHOT_LAG ) { + if ( $lockTSE >= 0 ) { + $tempTTL = max( 1, (int)$lockTSE ); // set() expects seconds + $this->cache->set( self::STASH_KEY_PREFIX . $key, $value, $tempTTL ); + } + + return true; // no-op the write for being unsafe + } + $wrapped = $this->wrap( $value, $ttl ); $func = function ( $cache, $key, $cWrapped ) use ( $wrapped ) { @@ -265,7 +312,7 @@ class WANObjectCache { : $wrapped; }; - return $this->cache->merge( $key, $func, $ttl, 1 ); + return $this->cache->merge( self::VALUE_KEY_PREFIX . $key, $func, $ttl, 1 ); } /** @@ -435,13 +482,15 @@ class WANObjectCache { /** * Method to fetch/regenerate cache keys * - * On cache miss, the key will be set to the callback result, - * unless the callback returns false. The arguments supplied are: - * (current value or false, &$ttl) + * On cache miss, the key will be set to the callback result via set() + * unless the callback returns false. The arguments supplied to it are: + * (current value or false, &$ttl, &$setOpts) * The callback function returns the new value given the current * value (false if not present). Preemptive re-caching and $checkKeys * can result in a non-false current value. The TTL of the new value * can be set dynamically by altering $ttl in the callback (by reference). + * The $setOpts array can be altered and is given to set() when called; + * it is recommended to set the 'since' field to avoid race conditions. * * Usually, callbacks ignore the current value, but it can be used * to maintain "most recent X" values that come from time or sequence @@ -464,7 +513,14 @@ class WANObjectCache { * @code * $key = wfMemcKey( 'cat-recent-actions', $catId ); * // Function that derives the new key value given the old value - * $callback = function( $cValue, &$ttl ) { ... }; + * $callback = function( $cValue, &$ttl, array &$setOpts ) { + * $dbr = wfGetDB( DB_SLAVE ); + * // Fetch the row from the DB + * $row = $dbr->selectRow( ... ); + * // Give the age of the transaction snapshot the data came from + * $setOpts = array( 'since' => $dbr->trxTimestamp() ); + * return $row; + * }; * // Get the key value from cache or from source on cache miss; * // try to only let one cluster thread manage doing cache updates * $opts = array( 'lockTSE' => 5, 'lowTTL' => 10 ); @@ -490,6 +546,7 @@ class WANObjectCache { * @endcode * * @see WANObjectCache::get() + * @see WANObjectCache::set() * * @param string $key Cache key * @param callable $callback Value generation function @@ -565,7 +622,8 @@ class WANObjectCache { } // Generate the new value from the callback... - $value = call_user_func_array( $callback, array( $cValue, &$ttl ) ); + $setOpts = array(); + $value = call_user_func_array( $callback, array( $cValue, &$ttl, &$setOpts ) ); // When delete() is called, writes are write-holed by the tombstone, // so use a special stash key to pass the new value around threads. if ( $useMutex && $value !== false && $ttl >= 0 ) { @@ -579,7 +637,8 @@ class WANObjectCache { if ( $value !== false && $ttl >= 0 ) { // Update the cache; this will fail if the key is tombstoned - $this->set( $key, $value, $ttl ); + $setOpts['lockTSE'] = $lockTSE; + $this->set( $key, $value, $ttl, $setOpts ); } return $value; -- 2.20.1