From e547cea55f654cf61d23ee0a919a8bb3bdc270c8 Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Tue, 17 Nov 2015 14:52:59 -0800 Subject: [PATCH 1/3] Make authentication calls with `_call_redis()` This will let us more easily catch an Exception if there's an error connecting. --- object-cache.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/object-cache.php b/object-cache.php index 6b6d842..17e1590 100644 --- a/object-cache.php +++ b/object-cache.php @@ -708,7 +708,7 @@ protected function _connect_redis() { $this->redis = new Redis; $this->redis->connect( $redis_server['host'], $redis_server['port'], 1, NULL, 100 ); # 1s timeout, 100ms delay between reconnections if ( ! empty( $redis_server['auth'] ) ) { - $this->redis->auth( $redis_server['auth'] ); + $this->_call_redis( 'auth', $redis_server['auth'] ); } $this->is_redis_connected = $this->redis->isConnected(); if ( ! $this->is_redis_connected ) { @@ -772,6 +772,7 @@ protected function _call_redis( $method ) { return $val; case 'delete': return 1; + case 'auth': case 'flushAll': case 'IsConnected': case 'exists': From 44beb042028a37b1f6588619b574c7f16f5b60e7 Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Tue, 24 Nov 2015 16:11:03 -0800 Subject: [PATCH 2/3] Properly catch exceptions thrown for authentication Rather than trying to use our fallback mechanism, we can just trigger an error here and let the fallback mechanism kick in later. --- object-cache.php | 12 ++++++++++-- tests/test-cache.php | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/object-cache.php b/object-cache.php index 0a9a423..30da529 100644 --- a/object-cache.php +++ b/object-cache.php @@ -714,7 +714,16 @@ protected function _connect_redis() { $port = ! empty( $redis_server['port'] ) ? $redis_server['port'] : 6379; $this->redis->connect( $redis_server['host'], $port, 1, NULL, 100 ); # 1s timeout, 100ms delay between reconnections if ( ! empty( $redis_server['auth'] ) ) { - $this->_call_redis( 'auth', $redis_server['auth'] ); + try { + $this->redis->auth( $redis_server['auth'] ); + } catch ( RedisException $e ) { + try { + $this->last_triggered_error = 'WP Redis: ' . $e->getMessage(); + trigger_error( $this->last_triggered_error, E_USER_WARNING ); + } catch( PHPUnit_Framework_Error_Warning $e ) { + // We'll inspect this in the test + } + } } $this->is_redis_connected = $this->redis->isConnected(); if ( ! $this->is_redis_connected ) { @@ -783,7 +792,6 @@ protected function _call_redis( $method ) { return $val; case 'delete': return 1; - case 'auth': case 'flushAll': case 'IsConnected': case 'exists': diff --git a/tests/test-cache.php b/tests/test-cache.php index dae7e47..95fca8b 100644 --- a/tests/test-cache.php +++ b/tests/test-cache.php @@ -87,6 +87,21 @@ public function test_redis_reload_force_cache_flush() { $this->assertEquals( "SELECT option_value FROM {$wpdb->options} WHERE option_name='wp_redis_do_redis_failback_flush'", $wpdb->last_query ); } + public function test_redis_bad_authentication() { + global $redis_server; + if ( ! class_exists( 'Redis' ) ) { + $this->markTestSkipped( 'PHPRedis extension not available.' ); + } + $redis_server['host'] = '127.0.0.1'; + $redis_server['port'] = 9999; + $redis_server['auth'] = 'foobar'; + $cache = new WP_Object_Cache; + $this->assertEquals( 'WP Redis: Redis server went away', $cache->last_triggered_error ); + $this->assertFalse( $cache->is_redis_connected ); + $cache->set( 'foo', 'bar' ); + $this->assertEquals( 'bar', $cache->get( 'foo' ) ); + } + function test_miss() { $this->assertEquals(NULL, $this->cache->get(rand_str())); } From a8e52f0037fded00a6fea4f23a5a26b4ec196824 Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Tue, 24 Nov 2015 16:56:55 -0800 Subject: [PATCH 3/3] Clarify documentation throughout, based on conversations --- object-cache.php | 14 ++++++++++++-- tests/test-cache.php | 1 + 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/object-cache.php b/object-cache.php index 30da529..7fb261e 100644 --- a/object-cache.php +++ b/object-cache.php @@ -716,12 +716,17 @@ protected function _connect_redis() { if ( ! empty( $redis_server['auth'] ) ) { try { $this->redis->auth( $redis_server['auth'] ); + // PhpRedis throws an Exception when it fails a server call. + // To prevent WordPress from fataling, we catch the Exception. } catch ( RedisException $e ) { try { $this->last_triggered_error = 'WP Redis: ' . $e->getMessage(); + // Be friendly to developers debugging production servers by triggering an error trigger_error( $this->last_triggered_error, E_USER_WARNING ); } catch( PHPUnit_Framework_Error_Warning $e ) { - // We'll inspect this in the test + // PHPUnit throws an Exception when `trigger_error()` is called. + // To ensure our tests (which expect Exceptions to be caught) continue to run, + // we catch the PHPUnit exception and inspect the RedisException message } } } @@ -749,15 +754,20 @@ protected function _call_redis( $method ) { try { $retval = call_user_func_array( array( $this->redis, $method ), $arguments ); return $retval; + // PhpRedis throws an Exception when it fails a server call. + // To prevent WordPress from fataling, we catch the Exception. } catch( RedisException $e ) { $retry_exception_messages = array( 'socket error on read socket', 'Connection closed', 'Redis server went away' ); $retry_exception_messages = apply_filters( 'wp_redis_retry_exception_messages', $retry_exception_messages ); if ( in_array( $e->getMessage(), $retry_exception_messages ) ) { try { $this->last_triggered_error = 'WP Redis: ' . $e->getMessage(); + // Be friendly to developers debugging production servers by triggering an error trigger_error( $this->last_triggered_error, E_USER_WARNING ); } catch( PHPUnit_Framework_Error_Warning $e ) { - // We'll inspect this in the test + // PHPUnit throws an Exception when `trigger_error()` is called. + // To ensure our tests (which expect Exceptions to be caught) continue to run, + // we catch the PHPUnit exception and inspect the RedisException message } // Attempt to refresh the connection if it was successfully established once // $this->is_redis_connected will be set inside _connect_redis() diff --git a/tests/test-cache.php b/tests/test-cache.php index 95fca8b..97c0d12 100644 --- a/tests/test-cache.php +++ b/tests/test-cache.php @@ -98,6 +98,7 @@ public function test_redis_bad_authentication() { $cache = new WP_Object_Cache; $this->assertEquals( 'WP Redis: Redis server went away', $cache->last_triggered_error ); $this->assertFalse( $cache->is_redis_connected ); + // Fails back to the internal object cache $cache->set( 'foo', 'bar' ); $this->assertEquals( 'bar', $cache->get( 'foo' ) ); }