Merge pull request #229 from chrisboulton/exceptions

Surface Redis exceptions instead of silently returning false

lib/Resque/Redis.php

@@ -111,34 +111,38 @@ class Resque_Redis
 	 */
     public function __construct($server, $database = null)
 	{
-		if (is_array($server)) {
+		try {
-			$this->driver = new Credis_Cluster($server);
+			if (is_array($server)) {
-		}
+				$this->driver = new Credis_Cluster($server);
-		else {
+			}
+			else {
+				list($host, $port, $dsnDatabase, $user, $password, $options) = self::parseDsn($server);
+				// $user is not used, only $password
 
-			list($host, $port, $dsnDatabase, $user, $password, $options) = self::parseDsn($server);
+				// Look for known Credis_Client options
-			// $user is not used, only $password
+				$timeout = isset($options['timeout']) ? intval($options['timeout']) : null;
+				$persistent = isset($options['persistent']) ? $options['persistent'] : '';
+				$maxRetries = isset($options['max_connect_retries']) ? $options['max_connect_retries'] : 0;
 
-			// Look for known Credis_Client options
+				$this->driver = new Credis_Client($host, $port, $timeout, $persistent);
-			$timeout = isset($options['timeout']) ? intval($options['timeout']) : null;
+				$this->driver->setMaxConnectRetries($maxRetries);
-			$persistent = isset($options['persistent']) ? $options['persistent'] : '';
+				if ($password){
-			$maxRetries = isset($options['max_connect_retries']) ? $options['max_connect_retries'] : 0;
+					$this->driver->auth($password);
+				}
 
-			$this->driver = new Credis_Client($host, $port, $timeout, $persistent);
+				// If we have found a database in our DSN, use it instead of the `$database`
-			$this->driver->setMaxConnectRetries($maxRetries);
+				// value passed into the constructor.
-			if ($password){
+				if ($dsnDatabase !== false) {
-				$this->driver->auth($password);
+					$database = $dsnDatabase;
+				}
 			}
 
-			// If we have found a database in our DSN, use it instead of the `$database`
+			if ($database !== null) {
-			// value passed into the constructor.
+				$this->driver->select($database);
-			if ($dsnDatabase !== false) {
-				$database = $dsnDatabase;
 			}
 		}
-
+		catch(CredisException $e) {
-		if ($database !== null) {
+			throw new Resque_RedisException('Error communicating with Redis: ' . $e->getMessage(), 0, $e);
-			$this->driver->select($database);
 		}
 	}
 
@@ -241,7 +245,7 @@ class Resque_Redis
 			return $this->driver->__call($name, $args);
 		}
 		catch (CredisException $e) {
-			return false;
+			throw new Resque_RedisException('Error communicating with Redis: ' . $e->getMessage(), 0, $e);
 		}
 	}
 

lib/Resque/RedisException.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * Redis related exceptions
+ *
+ * @package		Resque
+ * @author		Chris Boulton <chris@bigcommerce.com>
+ * @license		http://www.opensource.org/licenses/mit-license.php
+ */
+class Resque_RedisException extends Resque_Exception
+{
+}
+?>

test/Resque/Tests/JobTest.php

@@ -26,6 +26,15 @@ class Resque_Tests_JobTest extends Resque_Tests_TestCase
 		$this->assertTrue((bool)Resque::enqueue('jobs', 'Test_Job'));
 	}
 
+	/**
+	 * @expectedException Resque_RedisException
+	 */
+	public function testRedisErrorThrowsExceptionOnJobCreation()
+	{
+		Resque::setBackend('redis://255.255.255.255:1234');
+		Resque::enqueue('jobs', 'This is a test');
+	}
+
 	public function testQeueuedJobCanBeReserved()
 	{
 		Resque::enqueue('jobs', 'Test_Job');

test/Resque/Tests/RedisTest.php

@@ -1,13 +1,21 @@
 <?php
 /**
- * Resque_Redis DSN tests.
+ * Resque_Event tests.
  *
  * @package		Resque/Tests
- * @author		Iskandar Najmuddin <github@iskandar.co.uk>
+ * @author		Chris Boulton <chris@bigcommerce.com>
  * @license		http://www.opensource.org/licenses/mit-license.php
  */
-class Resque_Tests_DsnTest extends Resque_Tests_TestCase
+class Resque_Tests_RedisTest extends Resque_Tests_TestCase
 {
+	/**
+	 * @expectedException Resque_RedisException
+	 */
+	public function testRedisExceptionsAreSurfaced()
+	{
+		$redis = new Resque_Redis('redis://255.255.255.255:1234');
+		$redis->ping();
+	}
 
 	/**
 	 * These DNS strings are considered valid.
@@ -178,5 +186,4 @@ class Resque_Tests_DsnTest extends Resque_Tests_TestCase
 		// The next line should throw an InvalidArgumentException
 		$result = Resque_Redis::parseDsn($dsn);
 	}
-
+}
-}

test/Resque/Tests/TestCase.php

@@ -22,6 +22,8 @@ class Resque_Tests_TestCase extends PHPUnit_Framework_TestCase
 		preg_match('#^\s*port\s+([0-9]+)#m', $config, $matches);
 		$this->redis = new Credis_Client('localhost', $matches[1]);
 
+		Resque::setBackend('redis://localhost:' . $matches[1]);
+
 		// Flush redis
 		$this->redis->flushAll();
 	}