From 4265d8f23f205079a48dbb03a32347b5f3a9a396 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 17 Dec 2013 16:55:09 -0800 Subject: [PATCH] Added simpler handleError() method in RedisConnectionPool * Callers should not need to give $server since it is stored in RedisConnRef Change-Id: I902d984d6a7f19dd0d8c71ee374cbed359de378e --- includes/clientpool/RedisConnectionPool.php | 17 ++++++++- .../lockmanager/RedisLockManager.php | 4 +- includes/job/JobQueueRedis.php | 37 +++++++++---------- .../aggregator/JobQueueAggregatorRedis.php | 2 +- includes/objectcache/RedisBagOStuff.php | 20 +++++----- 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/includes/clientpool/RedisConnectionPool.php b/includes/clientpool/RedisConnectionPool.php index 983d90a25b..9e702e30ad 100644 --- a/includes/clientpool/RedisConnectionPool.php +++ b/includes/clientpool/RedisConnectionPool.php @@ -277,9 +277,24 @@ class RedisConnectionPool { * @param string $server * @param RedisConnRef $cref * @param RedisException $e + * @deprecated 1.23 */ public function handleException( $server, RedisConnRef $cref, RedisException $e ) { - wfDebugLog( 'redis', "Redis exception on server $server: " . $e->getMessage() ); + return $this->handleError( $cref, $e ); + } + + /** + * The redis extension throws an exception in response to various read, write + * and protocol errors. Sometimes it also closes the connection, sometimes + * not. The safest response for us is to explicitly destroy the connection + * object and let it be reopened during the next request. + * + * @param RedisConnRef $cref + * @param RedisException $e + */ + public function handleError( RedisConnRef $cref, RedisException $e ) { + $server = $cref->getServer(); + wfDebugLog( 'redis', "Redis exception on server $server: " . $e->getMessage() . "\n" ); foreach ( $this->connections[$server] as $key => $connection ) { if ( $cref->isConnIdentical( $connection['conn'] ) ) { $this->idlePoolSize -= $connection['free'] ? 1 : 0; diff --git a/includes/filebackend/lockmanager/RedisLockManager.php b/includes/filebackend/lockmanager/RedisLockManager.php index e37e567568..ff4cba56ce 100644 --- a/includes/filebackend/lockmanager/RedisLockManager.php +++ b/includes/filebackend/lockmanager/RedisLockManager.php @@ -153,7 +153,7 @@ LUA; ); } catch ( RedisException $e ) { $res = false; - $this->redisPool->handleException( $server, $conn, $e ); + $this->redisPool->handleError( $conn, $e ); } if ( $res === false ) { @@ -221,7 +221,7 @@ LUA; ); } catch ( RedisException $e ) { $res = false; - $this->redisPool->handleException( $server, $conn, $e ); + $this->redisPool->handleError( $conn, $e ); } if ( $res === false ) { diff --git a/includes/job/JobQueueRedis.php b/includes/job/JobQueueRedis.php index e0641b5774..342266496c 100644 --- a/includes/job/JobQueueRedis.php +++ b/includes/job/JobQueueRedis.php @@ -120,7 +120,7 @@ class JobQueueRedis extends JobQueue { try { return $conn->lSize( $this->getQueueKey( 'l-unclaimed' ) ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -141,7 +141,7 @@ class JobQueueRedis extends JobQueue { return array_sum( $conn->exec() ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -158,7 +158,7 @@ class JobQueueRedis extends JobQueue { try { return $conn->zSize( $this->getQueueKey( 'z-delayed' ) ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -175,7 +175,7 @@ class JobQueueRedis extends JobQueue { try { return $conn->zSize( $this->getQueueKey( 'z-abandoned' ) ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -229,7 +229,7 @@ class JobQueueRedis extends JobQueue { JobQueue::incrStats( 'job-insert-duplicate', $this->type, count( $items ) - $failed - $pushed ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } return true; @@ -330,7 +330,7 @@ LUA; $job = $this->getJobFromFields( $item ); // may be false } while ( !$job ); // job may be false if invalid } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } return $job; @@ -442,7 +442,7 @@ LUA; return false; } } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -473,7 +473,7 @@ LUA; // Update the timestamp of the last root job started at the location... return $conn->set( $key, $params['rootJobTimestamp'], self::ROOTJOB_TTL ); // 2 weeks } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -494,7 +494,7 @@ LUA; // Get the last time this root job was enqueued $timestamp = $conn->get( $this->getRootJobCacheKey( $params['rootJobSignature'] ) ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } // Check if a new root job was started at the location after this one's... @@ -519,7 +519,7 @@ LUA; return ( $conn->delete( $keys ) !== false ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -542,7 +542,7 @@ LUA; } ) ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -565,7 +565,7 @@ LUA; } ) ); } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -580,8 +580,8 @@ LUA; protected function doGetSiblingQueueSizes( array $types ) { $sizes = array(); // (type => size) $types = array_values( $types ); // reindex + $conn = $this->getConnection(); try { - $conn = $this->getConnection(); $conn->multi( Redis::PIPELINE ); foreach ( $types as $type ) { $conn->lSize( $this->getQueueKey( 'l-unclaimed', $type ) ); @@ -593,7 +593,7 @@ LUA; } } } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } return $sizes; @@ -623,7 +623,7 @@ LUA; return $job; } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } } @@ -707,7 +707,7 @@ LUA; JobQueue::incrStats( 'job-abandon', $this->type, $abandoned ); } } catch ( RedisException $e ) { - $this->throwRedisException( $this->server, $conn, $e ); + $this->throwRedisException( $conn, $e ); } return $count; @@ -822,13 +822,12 @@ LUA; } /** - * @param $server string * @param $conn RedisConnRef * @param $e RedisException * @throws JobQueueError */ - protected function throwRedisException( $server, RedisConnRef $conn, $e ) { - $this->redisPool->handleException( $server, $conn, $e ); + protected function throwRedisException( RedisConnRef $conn, $e ) { + $this->redisPool->handleError( $conn, $e ); throw new JobQueueError( "Redis server error: {$e->getMessage()}\n" ); } diff --git a/includes/job/aggregator/JobQueueAggregatorRedis.php b/includes/job/aggregator/JobQueueAggregatorRedis.php index c654933c25..2aec3c9cf4 100644 --- a/includes/job/aggregator/JobQueueAggregatorRedis.php +++ b/includes/job/aggregator/JobQueueAggregatorRedis.php @@ -175,7 +175,7 @@ class JobQueueAggregatorRedis extends JobQueueAggregator { * @return void */ protected function handleException( RedisConnRef $conn, $e ) { - $this->redisPool->handleException( $conn->getServer(), $conn, $e ); + $this->redisPool->handleError( $conn, $e ); } /** diff --git a/includes/objectcache/RedisBagOStuff.php b/includes/objectcache/RedisBagOStuff.php index 56f21283aa..427143c328 100644 --- a/includes/objectcache/RedisBagOStuff.php +++ b/includes/objectcache/RedisBagOStuff.php @@ -84,7 +84,7 @@ class RedisBagOStuff extends BagOStuff { $result = $this->unserialize( $value ); } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'get', $key, $server, $result ); @@ -108,7 +108,7 @@ class RedisBagOStuff extends BagOStuff { } } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'set', $key, $server, $result ); @@ -142,7 +142,7 @@ class RedisBagOStuff extends BagOStuff { $result = ( $conn->exec() == array( true ) ); } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'cas', $key, $server, $result ); @@ -162,7 +162,7 @@ class RedisBagOStuff extends BagOStuff { $result = true; } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'delete', $key, $server, $result ); @@ -201,7 +201,7 @@ class RedisBagOStuff extends BagOStuff { } } } catch ( RedisException $e ) { - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } } @@ -229,7 +229,7 @@ class RedisBagOStuff extends BagOStuff { } } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'add', $key, $server, $result ); @@ -260,7 +260,7 @@ class RedisBagOStuff extends BagOStuff { } } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'replace', $key, $server, $result ); @@ -290,7 +290,7 @@ class RedisBagOStuff extends BagOStuff { $result = $this->unserialize( $conn->incrBy( $key, $value ) ); } catch ( RedisException $e ) { $result = false; - $this->handleException( $server, $conn, $e ); + $this->handleException( $conn, $e ); } $this->logRequest( 'incr', $key, $server, $result ); @@ -352,8 +352,8 @@ class RedisBagOStuff extends BagOStuff { * not. The safest response for us is to explicitly destroy the connection * object and let it be reopened during the next request. */ - protected function handleException( $server, RedisConnRef $conn, $e ) { - $this->redisPool->handleException( $server, $conn, $e ); + protected function handleException( RedisConnRef $conn, $e ) { + $this->redisPool->handleError( $conn, $e ); } /** -- 2.20.1