Skip to content

Commit 81e4190

Browse files
Merge #733
733: Do not transform (parse) commands that fail inside of a pipeline r=badboy Hii all!! First of all, thanks a bunch for this gem and happy new year 😄 . I recently observed a exception triggered from another gem which extensively uses `redis-rb`[1]: ```ruby undefined method `each_slice' for #<Redis::CommandError:0x007fc46d9ca958> (NoMethodError) ``` <details><summary>Full stacktrace</summary> ``` /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis.rb:2809:in `block in <class:Redis>': undefined method `each_slice' for #<Redis::CommandError:0x007fc46d9ca958> (NoMethodError) from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:114:in `_set' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:61:in `block in finish' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:60:in `each' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:60:in `each_with_index' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:60:in `each' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:60:in `map' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:60:in `finish' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/client.rb:155:in `block in call_pipeline' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/client.rb:291:in `with_reconnect' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/client.rb:153:in `call_pipeline' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis.rb:2307:in `block in pipelined' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis.rb:45:in `block in synchronize' from /Users/javierhonduco/.rubies/ruby-2.4.0/lib/ruby/2.4.0/monitor.rb:214:in `mon_synchronize' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis.rb:45:in `synchronize' from /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis.rb:2303:in `pipelined' [...] ``` </details> I attempted getting a minimal reproduction and ended up with: ```ruby require 'redis' r = Redis.new r.pipelined do r.zrange('a', 'b', 5, :with_scores => true) end ``` This same code but with `with_scores` set to `false` raised this error instead: ```ruby /Users/javierhonduco/.gem/ruby/2.4.0/gems/redis-4.0.1/lib/redis/pipeline.rb:123:in `value': ERR value is not an integer or out of range (Redis::CommandError) ``` ### The problem After some more combinations tested, the key part (no pun intended), was executing a failing command inside a pipeline that requires some parsing (aka transformations) being applied to them. When the transformation is called passing the result, as it's a `Redis::CommandError`[2], rather than an `Array`, [this](https://github.com/redis/redis-rb/blob/master/lib/redis.rb#L2810) raises the original exception. `FloatifyPairs` just checks that `array` is not `false`-y (`nil` or `false`). ### Proposed solution Given that the problem is that we call `FloatifyPairs` passing it something that may, or may not be an `Enumerable`-like object, in this case, an `Array`, (and have `#each_slice` defined), this PR adds a check for that. In case `#each_slice ` is defined, it executes the current flow, otherwise, it just returns the argument verbatim, as this is going to be handled in https://github.com/redis/redis-rb/blob/master/lib/redis/pipeline.rb#L123, and the exception is going to be raised there. I chose this way of fixing the issue as its semantics are the same as the ones without the extra parsing required by `with_scores=true`. Let me know if you would prefer to fix this somewhere else, happy to change this patch 😄 #### Extra info [1]: We are using the one of the latest version of this gem (v4.0.1) and the testing has been done with master :) Don't think that for this case is necessary, but for the sake of completeness: - Ruby version: 2.4.0 (MRI) - Mac OS: 10.12.6 [2]: ```ruby [2805, 2814] in /Users/javierhonduco/.gem/ruby/2.3.3/gems/redis-4.0.1/lib/redis.rb 2805: } 2806: 2807: FloatifyPairs = 2808: lambda { |array| 2809: debugger => 2810: if array 2811: array.each_slice(2).map do |member, score| 2812: [member, Floatify.call(score)] 2813: end 2814: end (byebug) array #<Redis::CommandError: ERR value is not an integer or out of range> (byebug) array.class Redis::CommandError ```
2 parents 0a3f633 + 330fb77 commit 81e4190

File tree

2 files changed

+13
-3
lines changed

2 files changed

+13
-3
lines changed

lib/redis.rb

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2885,11 +2885,13 @@ def method_missing(command, *args)
28852885
}
28862886

28872887
FloatifyPairs =
2888-
lambda { |array|
2889-
if array
2890-
array.each_slice(2).map do |member, score|
2888+
lambda { |result|
2889+
if result.respond_to?(:each_slice)
2890+
result.each_slice(2).map do |member, score|
28912891
[member, Floatify.call(score)]
28922892
end
2893+
else
2894+
result
28932895
end
28942896
}
28952897

test/pipelining_commands_test.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,14 @@ def test_futures_raise_when_trying_to_access_their_values_too_early
128128
end
129129
end
130130

131+
def test_futures_raise_when_command_errors_and_needs_transformation
132+
assert_raise(Redis::CommandError) do
133+
r.pipelined do
134+
@result = r.zrange("a", "b", 5, :with_scores => true)
135+
end
136+
end
137+
end
138+
131139
def test_futures_can_be_identified
132140
r.pipelined do
133141
@result = r.sadd("foo", 1)

0 commit comments

Comments
 (0)