redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
124 stars 60 forks source link

fix bug with sentinels and string arguments #121

Closed moofkit closed 1 year ago

moofkit commented 1 year ago

Description

There are integration bug with actioncable and redis-client with sentinels config. It is possible to reproduce within this repo https://github.com/moofkit/actioncable-redis-sentinel-bug

$ bundle && bundle exec rails test
Running 1 tests in a single process (parallelization threshold is 50)
Run options: --seed 7348

# Running:

E

Error:
ActioncableTest#test_broadcasts_to_test_channel:
ArgumentError: unknown keywords: "host", "port"
    test/integration/actioncable_test.rb:5:in `block in <class:ActioncableTest>'

rails test test/integration/actioncable_test.rb:4

Finished in 0.043971s, 22.7423 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

The problem is that config for the cable is a YAML file and sentinels are deep nested hash in the sentinels array and even deep_symbolize_keys don't convert it to the symbols. So sentinels keys should be converted to symbol explicitly like in this fix

# config/initializers/action_cable.rb
require "action_cable/subscription_adapter/redis"

# https://github.com/rails/rails/blob/da309a582e04183a85340ec0e1ed59b2d69ca9c2/actioncable/lib/action_cable/subscription_adapter/redis.rb#LL18C21-L18C61
ActionCable::SubscriptionAdapter::Redis.redis_connector = lambda do |config|
  # BUG: it needs to symbolize keys for redis gem >= 5.0.0
  config[:sentinels] = config[:sentinels].map(&:symbolize_keys) if config.key?(:sentinels)
  Redis.new(config.except(:adapter, :channel_prefix))
end

I'm not sure should it be fixed in actioncable or here, but this PR is a patch for redis-client to handle such configs without errors. If this is not the right place, I will try to fix it on the rails side.

byroot commented 1 year ago

I'm not sure should it be fixed in actioncable or here

I believe it should be fixed in Action Cable. Please file a PR there and ping me, I'll review and merge it.