feat: support redis v4

[tonytonyjan]

In redis v4, the client inside a Redis instance is stateful and it will replace the client inside the Redis#multi block, which is different from v5.

It causes the issue that the Redis::Future instance is returned instead of the actual value when calling any command inside the Redis#multi block, which is not expected in our case.

redis.multi do
  redis.get('foo').class
  # => Redis::Future in v4
  # => NilClass in v5
end

[choznerol]

LGTM

[choznerol]

nitpick(non-blocking):

"Is Redis less than 5" feels like "Is Redis instance count less than 5" to me at first glance.

Wdyt about "Is Reids below V5"?

Suggested change

	IS_REDIS_LESS_THAN_5 = Gem::Version.new(::Redis::VERSION) < Gem::Version.new('5')
	IS_REDIS_BELOW_V5 = Gem::Version.new(::Redis::VERSION) < Gem::Version.new('5')

[tonytonyjan]

Thanks for the suggestion, I've updated the PR 🙏

[requiemformemories]

It seems that the IS_REDIS_BELOW_V5 change has not yet been applied to this branch. I will approve it after the update 🙇

[tonytonyjan]

Sorry, I thought I force pushed 😓

I've made the change, please review again, thank you 🙏

[choznerol]

question:

Both the before and after versions use @client for v5 transactions.

If I understand this v5.0.0 changelog entry correctly:

Removed the deprecated pipelined and multi signature. Commands now MUST be called on the block argument, not the original redis instance.

Shouldn't we always use Thread.current[:redis_multi] ("the block argument") instead of @redis ("the original redis instance") within a transaction, whether v4 or v5?

	v4	v5
inside transaction	Thread.current[:redis_multi]	Thread.current[:redis_multi] (and @client will be nil)
outside transaction	@redis	@redis

Suggested change

	client = Thread.current[:redis_multi] && IS_REDIS_LESS_THAN_5 ? @redis_for_less_than_v5 : @redis
	client = Thread.current[:redis_multi] || @redis

[tonytonyjan]

A classical read & write transaction in Redis is:

WATCH mykey
val = GET mykey
val = val + 1
MULTI
SET mykey $val
EXEC

With redis gem, it becomes:

redis.watch 'mykey' do
  val = redis.get('mykey') # read operation is outside of the multi block
  redis.multi do |multi|
    multi.write(val + 1) # write operation is insdie of the multi block
  end
end

The issue of lib/cover_rage/recorder.rb is that the read and write both happen] in #transaction block:

cover_rage/lib/cover_rage/recorder.rb

Lines 58 to 61 in d7e911d

	@store.transaction do
	records_to_save = Record.merge(@store.list, records)
	@store.update(records_to_save)
	end

Shouldn't we always use Thread.current[:redis_multi] ("the block argument") instead of @redis ("the original redis instance") within a transaction, whether v4 or v5?

If we always use Thread.current[:redis_multi], the #list will not return the exact value, and return an instance of Redis::Future instead.

I hope it answers your question. Any suggestions for improvement are appreciated. 🙏

[choznerol]

Ah I understand now. It's the Redis::Future that can't be read immediately. Thanks for the explaination 🙏 🙏

[choznerol]

Oh wait, you already use WATCH...MULTI...EXEC to detect race and rety. That's probably safe 🙏.

[requiemformemories]

suggestion(non-blocking):

When I first read this code, it was a bit hard to follow which client is used in different actions and why. I am thinking maybe we can extract read_client and write_client as private methods, making the intent clearer.

like this:

 def transaction(&)                                                                                                                                 
    loop do               
      break if @redis.watch(KEY) do
        @redis.multi do |multi|                                                                                                                      
          Thread.current[:redis_multi] = multi
          yield                                                                                                                                      
        ensure            
          Thread.current[:redis_multi] = nil
        end
      end
    end
  end

  def update(records)                                                                                                                                
    arguments = records.flat_map { |r| [r.path, JSON.dump(r.to_h)] }
    write_client.hset(KEY, *arguments)                                                                                                               
  end                     
                                                                                                                                                     
  def list                
    result = read_client.hgetall(KEY)
    return [] if result.empty?                                                                                                                       
    result.map { |_, value| Record.new(**JSON.parse(value)) }
  end                                                                                                                                                
                          
  private                                                                                                                                            
                          
  def read_client
    # In Redis v4 and below, @redis becomes stateful inside a multi block and returns Redis::Future instead of real 
    # values. Use a separate connection to perform the read outside the transaction.
    return @redis_for_less_than_v5 if Thread.current[:redis_multi] && IS_REDIS_LESS_THAN_5
    @redis                                                                                                                                           
  end
                                                                                                                                                     
  def write_client        
    Thread.current[:redis_multi] || @redis
  end                                                                                                                                                

Or I suggest to at least simply adding a comment above return @redis_for_less_than_v5 if Thread.current[:redis_multi] && IS_REDIS_LESS_THAN_5.

[requiemformemories]

LGTM!