Merge branch 'sentinel-tests' into zmack-fix_unconnectable_sentinel

Conflicts:
	lib/redis/client.rb

Author: djanowski

CHANGELOG.md

@@ -20,6 +20,11 @@
 
 * Added `:connect_timeout` option.
 
+* Added support for `:limit` option for `ZREVRANGEBYLEX`.
+
+* Fixed an issue where connections become inconsistent when using Ruby's
+  Timeout module outside of the client (see #501, #502).
+
 # 3.2.0
 
 * Redis Sentinel support.

README.md

@@ -105,7 +105,8 @@ and one or more slaves (`mymaster` in the example).
 
 * It is possible to optionally provide a role. The allowed roles are `master`
 and `slave`. When the role is `slave`, the client will try to connect to a
-random slave of the specified master.
+random slave of the specified master. If a role is not specified, the client
+will connect to the master.
 
 * When using the Sentinel support you need to specify a list of sentinels to
 connect to. The list does not need to enumerate all your Sentinel instances,
@@ -170,7 +171,7 @@ end
 Replies to commands in a pipeline can be accessed via the *futures* they
 emit (since redis-rb 3.0). All calls inside a pipeline block return a
 `Future` object, which responds to the `#value` method. When the
-pipeline has succesfully executed, all futures are assigned their
+pipeline has successfully executed, all futures are assigned their
 respective replies and can be used.
 
 ```ruby
@@ -186,6 +187,26 @@ end
 # => 1
 ```
 
+## Error Handling
+
+In general, if something goes wrong you'll get an exception. For example, if
+it can't connect to the server a `Redis::CannotConnectError` error will be raised.
+
+```ruby
+begin
+  redis.ping
+rescue Exception => e
+  e.inspect
+# => #<Redis::CannotConnectError: Timed out connecting to Redis on 10.0.1.1:6380>
+
+  e.message
+# => Timed out connecting to Redis on 10.0.1.1:6380
+end
+```
+
+See lib/redis/errors.rb for information about what exceptions are possible.
+
+
 ## Expert-Mode Options
 
  - `inherit_socket: true`: disable safety check that prevents a forked child

lib/redis.rb

@@ -764,7 +764,7 @@ def msetnx(*args)
   # Set one or more values, only if none of the keys exist.
   #
   # @example
-  #   redis.msetnx({ "key1" => "v1", "key2" => "v2" })
+  #   redis.mapped_msetnx({ "key1" => "v1", "key2" => "v2" })
   #     # => true
   #
   # @param [Hash] hash keys mapping to values
@@ -1620,6 +1620,28 @@ def zrangebylex(key, min, max, options = {})
     end
   end
 
+  # Return a range of members with the same score in a sorted set, by reversed lexicographical ordering.
+  # Apart from the reversed ordering, #zrevrangebylex is similar to #zrangebylex.
+  #
+  # @example Retrieve members matching a
+  #   redis.zrevrangebylex("zset", "[a", "[a\xff")
+  #     # => ["abbygail", "abby", "abagael", "aaren"]
+  # @example Retrieve the last 2 members matching a
+  #   redis.zrevrangebylex("zset", "[a", "[a\xff", :limit => [0, 2])
+  #     # => ["abbygail", "abby"]
+  #
+  # @see #zrangebylex
+  def zrevrangebylex(key, max, min, options = {})
+    args = []
+
+    limit = options[:limit]
+    args.concat(["LIMIT"] + limit) if limit
+
+    synchronize do |client|
+      client.call([:zrevrangebylex, key, max, min] + args)
+    end
+  end
+
   # Return a range of members in a sorted set, by score.
   #
   # @example Retrieve members with score `>= 5` and `< 100`
@@ -1895,7 +1917,7 @@ def hmget(key, *fields, &blk)
   # Get the values of all the given hash fields.
   #
   # @example
-  #   redis.hmget("hash", "f1", "f2")
+  #   redis.mapped_hmget("hash", "f1", "f2")
   #     # => { "f1" => "v1", "f2" => "v2" }
   #
   # @param [String] key
@@ -2032,7 +2054,7 @@ def punsubscribe(*channels)
     end
   end
 
-  # Inspect the state of the Pub/Sub subsystem. 
+  # Inspect the state of the Pub/Sub subsystem.
   # Possible subcommands: channels, numsub, numpat.
   def pubsub(subcommand, *args)
     synchronize do |client|
@@ -2458,7 +2480,7 @@ def sscan(key, cursor, options={})
   # Scan a set
   #
   # @example Retrieve all of the keys in a set
-  #   redis.sscan("set").to_a
+  #   redis.sscan_each("set").to_a
   #   # => ["key1", "key2", "key3"]
   #
   # @param [Hash] options

lib/redis/client.rb

@@ -77,6 +77,8 @@ def initialize(options = {})
       @connection = nil
       @command_map = {}
 
+      @pending_reads = 0
+
       if options.include?(:sentinels)
         @connector = Connector::Sentinel.new(@options)
       else
@@ -243,12 +245,15 @@ def io
 
     def read
       io do
-        connection.read
+        value = connection.read
+        @pending_reads -= 1
+        value
       end
     end
 
     def write(command)
       io do
+        @pending_reads += 1
         connection.write(command)
       end
     end
@@ -312,9 +317,10 @@ def establish_connection
       server = @connector.resolve.dup
 
       @options[:host] = server[:host]
-      @options[:port] = server[:port]
+      @options[:port] = Integer(server[:port]) if server.include?(:port)
 
-      @connection = @options[:driver].connect(server)
+      @connection = @options[:driver].connect(@options)
+      @pending_reads = 0
     rescue TimeoutError,
            Errno::ECONNREFUSED,
            Errno::EHOSTDOWN,
@@ -326,6 +332,8 @@ def establish_connection
     end
 
     def ensure_connected
+      disconnect if @pending_reads > 0
+
       attempts = 0
 
       begin
@@ -383,7 +391,7 @@ def _parse_options(options)
           defaults[:path]   = uri.path
         elsif uri.scheme == "redis"
           # Require the URL to have at least a host
-          raise ArgumentError, "invalid url" unless uri.host
+          raise ArgumentError, "invalid url: #{uri}" unless uri.host
 
           defaults[:scheme]   = uri.scheme
           defaults[:host]     = uri.host
@@ -462,7 +470,7 @@ def _parse_driver(driver)
 
     class Connector
       def initialize(options)
-        @options = options
+        @options = options.dup
       end
 
       def resolve
@@ -476,9 +484,9 @@ class Sentinel < Connector
         def initialize(options)
           super(options)
 
-          @sentinels = options.fetch(:sentinels).dup
-          @role = options[:role].to_s
-          @master = options[:host]
+          @sentinels = @options.delete(:sentinels).dup
+          @role = @options.fetch(:role, "master").to_s
+          @master = @options[:host]
         end
 
         def check(client)
@@ -493,7 +501,7 @@ def check(client)
           end
 
           if role != @role
-            disconnect
+            client.disconnect
             raise ConnectionError, "Instance role mismatch. Expected #{@role}, got #{role}."
           end
         end
@@ -514,7 +522,10 @@ def resolve
         def sentinel_detect
           3.times do
             @sentinels.each do |sentinel|
-              client = Client.new(:host => sentinel[:host], :port => sentinel[:port], :timeout => 0.3)
+              client = Client.new(@options.merge({
+                :host => sentinel[:host],
+                :port => sentinel[:port]
+              }))
 
               begin
                 if result = yield(client)

lib/redis/connection/hiredis.rb

@@ -9,11 +9,12 @@ class Hiredis
 
       def self.connect(config)
         connection = ::Hiredis::Connection.new
+        connect_timeout = (config.fetch(:connect_timeout, 0) * 1_000_000).to_i
 
         if config[:scheme] == "unix"
-          connection.connect_unix(config[:path], Integer(config[:connect_timeout] * 1_000_000))
+          connection.connect_unix(config[:path], connect_timeout)
         else
-          connection.connect(config[:host], config[:port], Integer(config[:connect_timeout] * 1_000_000))
+          connection.connect(config[:host], config[:port], connect_timeout)
         end
 
         instance = new(connection)

test/commands_on_sorted_sets_test.rb

@@ -22,6 +22,20 @@ def test_zrangebylex
     end
   end
 
+  def test_zrevrangebylex
+    target_version "2.9.9" do
+      r.zadd "foo", 0, "aaren"
+      r.zadd "foo", 0, "abagael"
+      r.zadd "foo", 0, "abby"
+      r.zadd "foo", 0, "abbygail"
+
+      assert_equal ["abbygail", "abby", "abagael", "aaren"], r.zrevrangebylex("foo", "[a\xff", "[a")
+      assert_equal ["abbygail", "abby"], r.zrevrangebylex("foo", "[a\xff", "[a", :limit => [0, 2])
+      assert_equal ["abbygail", "abby"], r.zrevrangebylex("foo", "(abb\xff", "(abb")
+      assert_equal ["abbygail"], r.zrevrangebylex("foo", "(abby\xff", "(abby")
+    end
+  end
+
   def test_zcount
     r.zadd "foo", 1, "s1"
     r.zadd "foo", 2, "s2"

test/connection_handling_test.rb

@@ -186,4 +186,25 @@ def test_config_set
       r.config :set, "timeout", 300
     end
   end
+
+  driver(:ruby, :hiredis) do
+    def test_consistency_on_multithreaded_env
+      t = nil
+
+      commands = {
+        :set => lambda { |key, value| t.kill; "+OK\r\n" },
+        :incr => lambda { |key| ":1\r\n" },
+      }
+
+      redis_mock(commands) do |redis|
+        t = Thread.new do
+          redis.set("foo", "bar")
+        end
+
+        t.join
+
+        assert_equal 1, redis.incr("baz")
+      end
+    end
+  end
 end