Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix raising command error for first command in pipeline #788

Merged

Conversation

dylanahsmith
Copy link
Collaborator

Problem

When the first command in a redis pipeline has invalid arguments, then the Redis::CommandError object is passed to the transform block which can cause the exception to be transformed and ignored. For example,

set_result, * = redis.pipelined do
  redis.set('mykey', 0, nx: true, px: 900.0) # px value is invalid if a float is given
end
puts set_result.inspect # => false

This problem was mentioned before for zrange in #733 but the fix was specific to the FloatifyPairs transform block so didn't also fix the problem for the set command that uses BoolifySet for the transform block when using the nx: true option.

Solution

Remove the special case in Redis::Client#call_pipelined to read the first command reply which was there only to set @reconnect = false, since it is fine to call that in the loop after each call to read.

I've removed the workaround that #733 added for this problem which isn't needed now that the exception gets raised before the transform block gets called. That way we can also rely on the test in that PR to make sure this works for any transform block.

Also, remove a workaround that was added for a subset of commands.
Copy link

@pushrax pushrax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is written suggests this was somehow intentional, but I can't think of why it should be. The reconnect behaviour should be identical with this change. LGTM, interested to see the maintainer response.

This seems straightforward to test and would benefit from having one added.

@dylanahsmith
Copy link
Collaborator Author

dylanahsmith commented Oct 17, 2018

I think the intention was that @reconnect = false gets set after the first read succeeds so that it doesn't reconnect and try to read replies to commands set on the previous connection. With the way it was written, it looks like missing the exception = reply if exception.nil? && reply.is_a?(CommandError) line after the first read was accidental.

interested to see the maintainer response.

Looks like Shopify took over maintaining the gem (see #752). Of course, I would appreciate another set of 👀 on this before merging.

Copy link

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch is good.

You might want to backport it to 3.x

@dylanahsmith dylanahsmith merged commit dda594a into redis:master Oct 18, 2018
@dylanahsmith dylanahsmith deleted the fix-pipelined-first-command-error branch October 18, 2018 13:38
byroot pushed a commit that referenced this pull request Oct 31, 2018
Also, remove a workaround that was added for a subset of commands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants