HGETALL in a pipeline results in an exception

[ducharmemp]

call in a pipeline context returns nil always because the pipeline is gathering up all the results. When calling HGETALL, the protocol-redis gem wants to immediately turn the result of that call into a hash by calling each_slice(2) which explodes. Minimal repro:

REDIS.pipeline do |context|
  context.hgetall("foobar") # 💥 
end

REDIS.hgetall("foobar") # {}

[ioquatix]

Are you able to propose a fix?

[ducharmemp]

Hey @ioquatix just wanted to let you know that I'm giving some brainpower to this but it might be a bit slow due to $DAYJOB and other obligations. That said here's a dump of my current context and hacking around to spark conversations:

• If the user uses the non-pipelined client, any transformations can be safely done inside the methods that protocol-redis exposes, since we're guaranteed to read_response immediately after the call
• If the user uses the pipelined client, we instead would like the methods in hashes.rb as an example to be applied later, when read_response has completed. We can guarantee an order of the responses to match the order of the requests if I'm understanding the redis protocols correctly.

One possible solution that might allow for some backwards compat come to mind:

Make #call and/or #write_request accept a block argument and store the proc in a queue. When read_response is called, we pop the proc off of the queue and call it. For the sync client, this will push/pop will be a tiny bit wasteful but for the pipelined approach it allows for a decent amount of flexibility. One method that would not seem to work in this approach is the hscan_each method, but I'm not sure if it makes sense for that to be pipeline-able anyways.

WIP patch: https://github.com/socketry/protocol-redis/compare/main...ducharmemp:protocol-redis:add-callback-stack-for-lazy-evaluation.patch
Branch link: https://github.com/ducharmemp/protocol-redis/tree/add-callback-stack-for-lazy-evaluation

This might be too simple or naiive a solution or might be plain and simple not what you'd like here, I'd love your feedback whenever you get a chance!

[ducharmemp]

On further consideration, I'm not sure that I like the interface of threading blocks around, but I'm not sure of a better approach. This also seems to be the way that redis-rb works: https://github.com/redis/redis-rb/blob/master/lib/redis/commands/hashes.rb

[ioquatix]

I've released protocol-redis v0.13.0 which ignores nil return values.

[ioquatix]

I like your proposal but I'm also slightly concerned about the performance cost and additional interface complexity. I don't think the performance cost is significant though.

[ducharmemp]

Apologies for the delay, I got pulled in a slightly different direction at work and hope to get back to this track.

I'm not sure that ignoring nil values is strictly correct here? In a pipeline, we're strictly saying "please give me this value later when I execute" which means that, given the current implementation of the pipeline interface, I'd want the functions in protocol-redis to apply later when I get the value. If nil is ignored here, then wouldn't that change the behavior of some of the protocol functions in pipelines vs direct calls?

I'll have to do some testing later today or tomorrow to see. For context, I was hoping to use this library (with some patching) as the redis lib for flipper which uses pipelining internally.