From 5dd409cd385f8b986e19b079fb3cf925405ec76e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 7 Mar 2019 13:15:29 -0800 Subject: [PATCH] objectcache: add $flags argument to BagOStuff::delete() This makes it consistent with set() and merge(). Also, one subclass was already using the field in this manner. Clean up the code related to WRITE_SYNC in SqlBagOStuff. Change-Id: I0fb84f4475311889507d3ef98afd4476fb81174f --- includes/libs/objectcache/APCBagOStuff.php | 2 +- includes/libs/objectcache/APCUBagOStuff.php | 2 +- includes/libs/objectcache/BagOStuff.php | 3 +- includes/libs/objectcache/CachedBagOStuff.php | 2 +- includes/libs/objectcache/EmptyBagOStuff.php | 2 +- includes/libs/objectcache/HashBagOStuff.php | 2 +- .../libs/objectcache/MemcachedBagOStuff.php | 2 +- .../objectcache/MemcachedPeclBagOStuff.php | 2 +- .../libs/objectcache/MultiWriteBagOStuff.php | 2 +- includes/libs/objectcache/RESTBagOStuff.php | 19 +----- includes/libs/objectcache/RedisBagOStuff.php | 2 +- .../libs/objectcache/ReplicatedBagOStuff.php | 4 +- .../libs/objectcache/WinCacheBagOStuff.php | 2 +- includes/objectcache/SqlBagOStuff.php | 58 +++++++++++-------- 14 files changed, 52 insertions(+), 52 deletions(-) diff --git a/includes/libs/objectcache/APCBagOStuff.php b/includes/libs/objectcache/APCBagOStuff.php index e41c3a2592..1fedfaf616 100644 --- a/includes/libs/objectcache/APCBagOStuff.php +++ b/includes/libs/objectcache/APCBagOStuff.php @@ -104,7 +104,7 @@ class APCBagOStuff extends BagOStuff { return $value; } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { apc_delete( $key . self::KEY_SUFFIX ); return true; diff --git a/includes/libs/objectcache/APCUBagOStuff.php b/includes/libs/objectcache/APCUBagOStuff.php index a26e56020f..fb43d4ddb3 100644 --- a/includes/libs/objectcache/APCUBagOStuff.php +++ b/includes/libs/objectcache/APCUBagOStuff.php @@ -55,7 +55,7 @@ class APCUBagOStuff extends APCBagOStuff { return true; } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { apcu_delete( $key . self::KEY_SUFFIX ); return true; diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index a7ef3d5781..9635543a74 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -262,8 +262,9 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { * * @param string $key * @return bool True if the item was deleted or not found, false on failure + * @param int $flags Bitfield of BagOStuff::WRITE_* constants */ - abstract public function delete( $key ); + abstract public function delete( $key, $flags = 0 ); /** * Merge changes into the existing cache value (possibly creating a new one) diff --git a/includes/libs/objectcache/CachedBagOStuff.php b/includes/libs/objectcache/CachedBagOStuff.php index 726836e886..3927ed3004 100644 --- a/includes/libs/objectcache/CachedBagOStuff.php +++ b/includes/libs/objectcache/CachedBagOStuff.php @@ -68,7 +68,7 @@ class CachedBagOStuff extends HashBagOStuff { } public function delete( $key, $flags = 0 ) { - parent::delete( $key ); + parent::delete( $key, $flags ); if ( !( $flags & self::WRITE_CACHE_ONLY ) ) { $this->backend->delete( $key ); } diff --git a/includes/libs/objectcache/EmptyBagOStuff.php b/includes/libs/objectcache/EmptyBagOStuff.php index 3f66c06c9f..3bf58df892 100644 --- a/includes/libs/objectcache/EmptyBagOStuff.php +++ b/includes/libs/objectcache/EmptyBagOStuff.php @@ -39,7 +39,7 @@ class EmptyBagOStuff extends BagOStuff { return true; } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { return true; } diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php index ec66492044..7d074fa560 100644 --- a/includes/libs/objectcache/HashBagOStuff.php +++ b/includes/libs/objectcache/HashBagOStuff.php @@ -106,7 +106,7 @@ class HashBagOStuff extends BagOStuff { return true; } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { unset( $this->bag[$key] ); return true; diff --git a/includes/libs/objectcache/MemcachedBagOStuff.php b/includes/libs/objectcache/MemcachedBagOStuff.php index f7bf86b905..bf0da11725 100644 --- a/includes/libs/objectcache/MemcachedBagOStuff.php +++ b/includes/libs/objectcache/MemcachedBagOStuff.php @@ -70,7 +70,7 @@ class MemcachedBagOStuff extends BagOStuff { $value, $this->fixExpiry( $exptime ) ); } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { return $this->client->delete( $this->validateKeyEncoding( $key ) ); } diff --git a/includes/libs/objectcache/MemcachedPeclBagOStuff.php b/includes/libs/objectcache/MemcachedPeclBagOStuff.php index fe31c258b2..a6646bcd89 100644 --- a/includes/libs/objectcache/MemcachedPeclBagOStuff.php +++ b/includes/libs/objectcache/MemcachedPeclBagOStuff.php @@ -172,7 +172,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff { return $this->checkResult( $key, parent::cas( $casToken, $key, $value, $exptime ) ); } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { $this->debugLog( "delete($key)" ); $result = parent::delete( $key ); if ( $result === false && $this->client->getResultCode() === Memcached::RES_NOTFOUND ) { diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php index 043f8cbbe2..f549876554 100644 --- a/includes/libs/objectcache/MultiWriteBagOStuff.php +++ b/includes/libs/objectcache/MultiWriteBagOStuff.php @@ -139,7 +139,7 @@ class MultiWriteBagOStuff extends BagOStuff { return $this->doWrite( $this->cacheIndexes, $asyncWrites, 'set', $key, $value, $exptime ); } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { return $this->doWrite( $this->cacheIndexes, $this->asyncWrites, 'delete', $key ); } diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php index fc3d575547..b0b82d86ed 100644 --- a/includes/libs/objectcache/RESTBagOStuff.php +++ b/includes/libs/objectcache/RESTBagOStuff.php @@ -124,16 +124,8 @@ class RESTBagOStuff extends BagOStuff { return false; } - /** - * Set an item - * - * @param string $key - * @param mixed $value - * @param int $exptime Either an interval in seconds or a unix timestamp for expiry - * @param int $flags Bitfield of BagOStuff::WRITE_* constants - * @return bool Success - */ public function set( $key, $value, $exptime = 0, $flags = 0 ) { + // @TODO: respect WRITE_SYNC (e.g. EACH_QUORUM) $req = [ 'method' => 'PUT', 'url' => $this->url . rawurlencode( $key ), @@ -146,13 +138,8 @@ class RESTBagOStuff extends BagOStuff { return $this->handleError( "Failed to store $key", $rcode, $rerr ); } - /** - * Delete an item. - * - * @param string $key - * @return bool True if the item was deleted or not found, false on failure - */ - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { + // @TODO: respect WRITE_SYNC (e.g. EACH_QUORUM) $req = [ 'method' => 'DELETE', 'url' => $this->url . rawurlencode( $key ), diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php index abf9e8b4ea..a0febfc48d 100644 --- a/includes/libs/objectcache/RedisBagOStuff.php +++ b/includes/libs/objectcache/RedisBagOStuff.php @@ -132,7 +132,7 @@ class RedisBagOStuff extends BagOStuff { return $result; } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { list( $server, $conn ) = $this->getConnection( $key ); if ( !$conn ) { return false; diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php index f2c3732230..e2b9a52df1 100644 --- a/includes/libs/objectcache/ReplicatedBagOStuff.php +++ b/includes/libs/objectcache/ReplicatedBagOStuff.php @@ -90,8 +90,8 @@ class ReplicatedBagOStuff extends BagOStuff { return $this->writeStore->set( $key, $value, $exptime, $flags ); } - public function delete( $key ) { - return $this->writeStore->delete( $key ); + public function delete( $key, $flags = 0 ) { + return $this->writeStore->delete( $key, $flags ); } public function add( $key, $value, $exptime = 0 ) { diff --git a/includes/libs/objectcache/WinCacheBagOStuff.php b/includes/libs/objectcache/WinCacheBagOStuff.php index 764230bf8f..6b0bec0025 100644 --- a/includes/libs/objectcache/WinCacheBagOStuff.php +++ b/includes/libs/objectcache/WinCacheBagOStuff.php @@ -45,7 +45,7 @@ class WinCacheBagOStuff extends BagOStuff { return ( is_array( $result ) && $result === [] ) || $result; } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { wincache_ucache_delete( $key ); return true; diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index 2866a6c9cf..522233b555 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -423,7 +423,9 @@ class SqlBagOStuff extends BagOStuff { return (bool)$db->affectedRows(); } - public function delete( $key ) { + public function delete( $key, $flags = 0 ) { + $ok = true; + list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); $db = null; $silenceScope = $this->silenceTransactionProfiler(); @@ -435,10 +437,13 @@ class SqlBagOStuff extends BagOStuff { __METHOD__ ); } catch ( DBError $e ) { $this->handleWriteError( $e, $db, $serverIndex ); - return false; + $ok = false; + } + if ( ( $flags & self::WRITE_SYNC ) == self::WRITE_SYNC ) { + $ok = $this->waitForReplication() && $ok; } - return true; + return $ok; } public function incr( $key, $step = 1 ) { @@ -711,14 +716,8 @@ class SqlBagOStuff extends BagOStuff { if ( $exception instanceof DBConnectionError ) { $this->markServerDown( $exception, $serverIndex ); } - $this->logger->error( "DBError: {$exception->getMessage()}" ); - if ( $exception instanceof DBConnectionError ) { - $this->setLastError( BagOStuff::ERR_UNREACHABLE ); - $this->logger->debug( __METHOD__ . ": ignoring connection error" ); - } else { - $this->setLastError( BagOStuff::ERR_UNEXPECTED ); - $this->logger->debug( __METHOD__ . ": ignoring query error" ); - } + + $this->setAndLogDBError( $exception ); } /** @@ -741,6 +740,13 @@ class SqlBagOStuff extends BagOStuff { } } + $this->setAndLogDBError( $exception ); + } + + /** + * @param DBError $exception + */ + private function setAndLogDBError( DBError $exception ) { $this->logger->error( "DBError: {$exception->getMessage()}" ); if ( $exception instanceof DBConnectionError ) { $this->setLastError( BagOStuff::ERR_UNREACHABLE ); @@ -813,20 +819,26 @@ class SqlBagOStuff extends BagOStuff { } // Main LB is used; wait for any replica DBs to catch up - $masterPos = $lb->getMasterPos(); - if ( !$masterPos ) { - return true; // not applicable - } + try { + $masterPos = $lb->getMasterPos(); + if ( !$masterPos ) { + return true; // not applicable + } - $loop = new WaitConditionLoop( - function () use ( $lb, $masterPos ) { - return $lb->waitForAll( $masterPos, 1 ); - }, - $this->syncTimeout, - $this->busyCallbacks - ); + $loop = new WaitConditionLoop( + function () use ( $lb, $masterPos ) { + return $lb->waitForAll( $masterPos, 1 ); + }, + $this->syncTimeout, + $this->busyCallbacks + ); - return ( $loop->invoke() === $loop::CONDITION_REACHED ); + return ( $loop->invoke() === $loop::CONDITION_REACHED ); + } catch ( DBError $e ) { + $this->setAndLogDBError( $e ); + + return false; + } } /** -- 2.20.1