Merge pull request #1129 from casperisfine/double-timeouts

Remove positional timeout and better accept Float values

Author: byroot

CHANGELOG.md

@@ -5,6 +5,8 @@
 - Use `MD5` for hashing server nodes in `Redis::Distributed`. This should improve keys distribution among servers. See #1089.
 - Cluster support has been moved to a `redis_cluster` companion gem.
 - `select` no longer record the current database. If the client has to reconnect after `select` was used, it will reconnect to the original database.
+- Better support Float timeout in blocking commands. See #977.
+- Removed positional timeout in blocking commands (`BLPOP`, etc). Timeout now must be passed as an option: `r.blpop("key", timeout: 2.5)`
 - Removed `logger` option.
 - Removed `reconnect_delay_max` and `reconnect_delay`, you can pass precise sleep durations to `reconnect_attempts` instead.
 - Require Ruby 2.5+.

lib/redis/commands/lists.rb

@@ -48,7 +48,7 @@ def lmove(source, destination, where_source, where_destination)
       # @param [String, Symbol] where_destination where to push the element to the source list
       #     e.g. 'LEFT' - to head, 'RIGHT' - to tail
       # @param [Hash] options
-      #   - `:timeout => Numeric`: timeout in seconds, defaults to no timeout
+      #   - `:timeout => [Float, Integer]`: timeout in seconds, defaults to no timeout
       #
       # @return [nil, String] the element, or nil when the source key does not exist or the timeout expired
       #
@@ -142,7 +142,7 @@ def rpoplpush(source, destination)
       # @param [String, Array<String>] keys one or more keys to perform the
       #   blocking pop on
       # @param [Hash] options
-      #   - `:timeout => Integer`: timeout in seconds, defaults to no timeout
+      #   - `:timeout => [Float, Integer]`: timeout in seconds, defaults to no timeout
       #
       # @return [nil, [String, String]]
       #   - `nil` when the operation timed out
@@ -156,7 +156,7 @@ def blpop(*args)
       # @param [String, Array<String>] keys one or more keys to perform the
       #   blocking pop on
       # @param [Hash] options
-      #   - `:timeout => Integer`: timeout in seconds, defaults to no timeout
+      #   - `:timeout => [Float, Integer]`: timeout in seconds, defaults to no timeout
       #
       # @return [nil, [String, String]]
       #   - `nil` when the operation timed out
@@ -173,12 +173,12 @@ def brpop(*args)
       # @param [String] source source key
       # @param [String] destination destination key
       # @param [Hash] options
-      #   - `:timeout => Integer`: timeout in seconds, defaults to no timeout
+      #   - `:timeout => [Float, Integer]`: timeout in seconds, defaults to no timeout
       #
       # @return [nil, String]
       #   - `nil` when the operation timed out
       #   - the element was popped and pushed otherwise
-      def brpoplpush(source, destination, deprecated_timeout = 0, timeout: deprecated_timeout)
+      def brpoplpush(source, destination, timeout: 0)
         command = [:brpoplpush, source, destination, timeout]
         send_blocking_command(command, timeout)
       end
@@ -253,20 +253,16 @@ def _bpop(cmd, args, &blk)
         timeout = if args.last.is_a?(Hash)
           options = args.pop
           options[:timeout]
-        elsif args.last.respond_to?(:to_int)
-          # Issue deprecation notice in obnoxious mode...
-          args.pop.to_int
         end
 
         timeout ||= 0
-
-        if args.size > 1
-          # Issue deprecation notice in obnoxious mode...
+        unless timeout.is_a?(Integer) || timeout.is_a?(Float)
+          raise ArgumentError, "timeout must be an Integer or Float, got: #{timeout.class}"
         end
 
-        keys = args.flatten
-
-        command = [cmd, keys, timeout]
+        command = [cmd]
+        command.concat(args.flatten)
+        command << timeout
         send_blocking_command(command, timeout, &blk)
       end
 

lib/redis/distributed.rb

@@ -460,13 +460,6 @@ def _bpop(cmd, args)
       timeout = if args.last.is_a?(Hash)
         options = args.pop
         options[:timeout]
-      elsif args.last.respond_to?(:to_int)
-        # Issue deprecation notice in obnoxious mode...
-        args.pop.to_int
-      end
-
-      if args.size > 1
-        # Issue deprecation notice in obnoxious mode...
       end
 
       keys = args.flatten
@@ -486,6 +479,18 @@ def blpop(*args)
       _bpop(:blpop, args)
     end
 
+    def bzpopmax(*args)
+      _bpop(:bzpopmax, args) do |reply|
+        reply.is_a?(Array) ? [reply[0], reply[1], Floatify.call(reply[2])] : reply
+      end
+    end
+
+    def bzpopmin(*args)
+      _bpop(:bzpopmin, args) do |reply|
+        reply.is_a?(Array) ? [reply[0], reply[1], Floatify.call(reply[2])] : reply
+      end
+    end
+
     # Remove and get the last element in a list, or block until one is
     # available.
     def brpop(*args)
@@ -494,9 +499,9 @@ def brpop(*args)
 
     # Pop a value from a list, push it to another list and return it; or block
     # until one is available.
-    def brpoplpush(source, destination, deprecated_timeout = 0, **options)
+    def brpoplpush(source, destination, **options)
       ensure_same_node(:brpoplpush, [source, destination]) do |node|
-        node.brpoplpush(source, destination, deprecated_timeout, **options)
+        node.brpoplpush(source, destination, **options)
       end
     end
 

test/distributed/blocking_commands_test.rb

@@ -20,41 +20,15 @@ def test_blpop_raises
     end
   end
 
-  def test_blpop_raises_with_old_prototype
-    assert_raises(Redis::Distributed::CannotDistribute) do
-      r.blpop('foo', 'bar', 0)
-    end
-  end
-
   def test_brpop_raises
     assert_raises(Redis::Distributed::CannotDistribute) do
       r.brpop(%w[foo bar])
     end
   end
 
-  def test_brpop_raises_with_old_prototype
-    assert_raises(Redis::Distributed::CannotDistribute) do
-      r.brpop('foo', 'bar', 0)
-    end
-  end
-
   def test_brpoplpush_raises
     assert_raises(Redis::Distributed::CannotDistribute) do
       r.brpoplpush('foo', 'bar')
     end
   end
-
-  def test_brpoplpush_raises_with_old_prototype
-    assert_raises(Redis::Distributed::CannotDistribute) do
-      r.brpoplpush('foo', 'bar', 0)
-    end
-  end
-
-  def test_bzpopmin
-    # Not implemented yet
-  end
-
-  def test_bzpopmax
-    # Not implemented yet
-  end
 end

test/lint/blocking_commands.rb

@@ -101,22 +101,8 @@ def to_int
     end
 
     def test_blpop_integer_like_timeout
-      mock do |r|
-        assert_equal ["{zap}foo", "1"], r.blpop("{zap}foo", FakeDuration.new(1))
-      end
-    end
-
-    def test_blpop_with_old_prototype
-      assert_equal ['{zap}foo', 's1'], r.blpop('{zap}foo', 0)
-      assert_equal ['{zap}foo', 's2'], r.blpop('{zap}foo', 0)
-      assert_equal ['{zap}bar', 's1'], r.blpop('{zap}bar', '{zap}foo', 0)
-      assert_equal ['{zap}bar', 's2'], r.blpop('{zap}foo', '{zap}bar', 0)
-    end
-
-    def test_blpop_timeout_with_old_prototype
-      mock do |r|
-        assert_equal ['{zap}foo', '0'], r.blpop('{zap}foo', 0)
-        assert_equal ['{zap}foo', '1'], r.blpop('{zap}foo', 1)
+      assert_raises ArgumentError do
+        assert_equal ["{zap}foo", "1"], r.blpop("{zap}foo", timeout: FakeDuration.new(1))
       end
     end
 
@@ -134,20 +120,6 @@ def test_brpop_timeout
       end
     end
 
-    def test_brpop_with_old_prototype
-      assert_equal ['{zap}foo', 's2'], r.brpop('{zap}foo', 0)
-      assert_equal ['{zap}foo', 's1'], r.brpop('{zap}foo', 0)
-      assert_equal ['{zap}bar', 's2'], r.brpop('{zap}bar', '{zap}foo', 0)
-      assert_equal ['{zap}bar', 's1'], r.brpop('{zap}foo', '{zap}bar', 0)
-    end
-
-    def test_brpop_timeout_with_old_prototype
-      mock do |r|
-        assert_equal ['{zap}foo', '0'], r.brpop('{zap}foo', 0)
-        assert_equal ['{zap}foo', '1'], r.brpop('{zap}foo', 1)
-      end
-    end
-
     def test_brpoplpush
       assert_equal 's2', r.brpoplpush('{zap}foo', '{zap}qux')
       assert_equal ['s2'], r.lrange('{zap}qux', 0, -1)
@@ -160,26 +132,24 @@ def test_brpoplpush_timeout
       end
     end
 
-    def test_brpoplpush_with_old_prototype
-      assert_equal 's2', r.brpoplpush('{zap}foo', '{zap}qux', 0)
-      assert_equal ['s2'], r.lrange('{zap}qux', 0, -1)
+    def test_bzpopmin
+      assert_equal ['{szap}foo', 'a', 0.0], r.bzpopmin('{szap}foo', '{szap}bar', timeout: 1)
     end
 
-    def test_brpoplpush_timeout_with_old_prototype
-      mock do |r|
-        assert_equal '0', r.brpoplpush('{zap}foo', '{zap}bar', 0)
-        assert_equal '1', r.brpoplpush('{zap}foo', '{zap}bar', 1)
+    def test_bzpopmin_float_timeout
+      target_version "6.0" do
+        assert_nil r.bzpopmin('{szap}aaa', '{szap}bbb', timeout: LOW_TIMEOUT)
       end
     end
 
-    def test_bzpopmin
-      assert_equal ['{szap}foo', 'a', 0.0], r.bzpopmin('{szap}foo', '{szap}bar', 1)
-      assert_nil r.bzpopmin('{szap}aaa', '{szap}bbb', 2)
+    def test_bzpopmax
+      assert_equal ['{szap}foo', 'c', 2.0], r.bzpopmax('{szap}foo', '{szap}bar', timeout: 1)
     end
 
-    def test_bzpopmax
-      assert_equal ['{szap}foo', 'c', 2.0], r.bzpopmax('{szap}foo', '{szap}bar', 1)
-      assert_nil r.bzpopmax('{szap}aaa', '{szap}bbb', 1)
+    def test_bzpopmax_float_timeout
+      target_version "6.0" do
+        assert_nil r.bzpopmax('{szap}aaa', '{szap}bbb', timeout: LOW_TIMEOUT)
+      end
     end
 
     def test_blmove_socket_timeout