XCLAIM exception with nil result

[eadz]

XCLAIM can return a nil array result. ( [nil] )
It's passed to HashifyStreamEntries which then calls each_slice on nil causing an exception.

In REDIS, the following command can return an array with a nil element.

"xclaim" "mystream" "default" "3734fd61-ec3f-4bc1-b59d-3719d8ae40be" "1" "1589669825174-0"`
1) (nil)

[eadz]

Closing until I can reproduce reliably.

[eadz]

So I haven't managed to write a failing test, and my PR fix is no good, but I'm pretty sure there is a bug in HashifyStreamEntries

( for reference: )

HashifyStreamEntries = lambda { |reply|
    reply.map do |entry_id, values|
      [entry_id, values.each_slice(2).to_h]
    end
  }

Arrays can have nil elements. The reply coming into the HashifyStreamEntries that caused the error is [nil].

# current HashifyStreamEntries
reply = [nil]
HashifyStreamEntries.call(reply) # => NoMethodError (undefined method `each_slice' for nil:NilClass)

Personally, I don't care about nil results, so I am using a fix of compact following fix:

HashifyStreamEntries = lambda { |reply|
    reply.compact.map do |entry_id, values|
      [entry_id, values.each_slice(2).to_h]
    end
  }

[eadz]

would a patch using compact be acceptable?

[byroot]

Not without a reproduction.

[eadz]

I thought I explained clearly how HashifyStreamEntries cannot handle an array with a nil value.
It's self evident from the code.

Is a reproduction necessary because you do not think Redis will return an array with a nil value? Even though that's the documented behavior?

[byroot]

Even though that's the documented behavior?

That's not the documented behavior. What the documentation says is that it returns an Array, then points to the RESPv3 (which this library doesn't use) generic description of an array, and say that an array can contain a nil.

The documentation doesn't say that XCLAIM can return nil.

You have to understand that I can't do blind fixes like this. What is a fix for you might very well introduce a bug for others, hence why I need to be able to reproduce and understand the bug.

By the way, what driver do you use?

[eadz]

Fair enough, ok it took some time, but I've managed to write a script which reproduces the bug every time I run it. Running redis 5.0.7 btw.

require "bundler/inline"

gemfile do
  source "https://rubygems.org/"
  gem "redis"
end

def rs; (rand * 10**10).to_i.to_s(32); end

id = rs

puts "using stream '#{id}'"
redis = Redis.current
redis.xgroup(:create, id, 'a', "$", mkstream: true)

threads = []

jids = []

clients = [:a,:b,:c]

sizes = [1,4,8]

100.times do
  jids << redis.xadd(id, test: rs)
end

threads << Thread.new do
  redis = Redis.new
  10000.times do
    sleep rand / 10000
    redis.xreadgroup(:a, sizes.sample, id, ">", count: sizes.sample)
  end
end

threads << Thread.new do
  redis = Redis.new
  10000.times do
    sleep rand / 10000
    jid = jids.sample
    redis.multi do
      redis.xack(id, clients.sample, jid)
      redis.xdel(id, jid)
    end
  end
end

threads << Thread.new do
  redis = Redis.new
  10000.times do
    sleep rand / 10000
    redis.xclaim(id, :a, clients.sample, sizes.sample, jids.sample)
  end
end

threads.each(&:join)

[eadz]

Sorry it's a random script, but you can see that as I mentioned, XCLAIM can return a result like [nil].

[byroot]

Thank you. I'll dig into that script.

[byroot]

Thank again for the script. I merge a slightly different patch: #929

[eadz]

Hi,
Can we reopen this issue:

The reason I used compact is that it's an array, where some of the replies can be empty.

XCLAIM can take multiple IDs, so can return an array like [something, nil, nil, something]

Here is a repoduction with your fix, same problem

require "bundler/inline"

gemfile do
  source "https://rubygems.org/"
  gem "redis"
end

class Redis
  EMPTY_STREAM_RESPONSE = [nil].freeze
  private_constant :EMPTY_STREAM_RESPONSE

  HashifyStreamEntries = lambda { |reply|
    return []  if reply == EMPTY_STREAM_RESPONSE
    reply.map do |entry_id, values|
      [entry_id, values.each_slice(2).to_h]
    end
  }
end

def rs; (rand * 10**10).to_i.to_s(32); end

id = rs

puts "using stream '#{id}'"
redis = Redis.current
redis.xgroup(:create, id, 'a', "$", mkstream: true)

threads = []

jids = []

clients = [:a,:b,:c]

sizes = [10,20,30]

10000.times do
  jids << redis.xadd(id, test: rs)
end

threads << Thread.new do
  redis = Redis.new
  10000.times do
    sleep rand / 10000
    redis.xreadgroup(:a, sizes.sample, id, ">", count: sizes.sample)
  end
end

threads << Thread.new do
  redis = Redis.new
  10000.times do
    sleep rand / 10000
    jid = jids.sample
    redis.multi do
      redis.xack(id, clients.sample, jid)
      redis.xdel(id, jid)
    end
  end
end

threads << Thread.new do
  redis = Redis.new
  10000.times do
    sleep rand / 10000
    redis.xclaim(id, :a, clients.sample, sizes.sample, jids.sample, jids.sample, jids.sample, jids.sample)
  end
end

threads.each(&:join)

[byroot]

@eadz I see. Let's go with compact then, I pushed 9fd381b

[eadz]

Great, thank you! And thanks for all your work on redis-rb too!