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

[ActionCable] Add sentinel support to ActionCable Redis subscription adapter #32744

Closed
aisrael opened this issue Apr 28, 2018 · 2 comments
Closed

Comments

@aisrael
Copy link

aisrael commented Apr 28, 2018

When attempting to use ActionCable with Redis Sentinel, it fails to establish a connection. Even if config/cable.yml is configured similarly to the Rails cache and session stores, the Redis connector in the subscription adapter doesn't recognize the sentinel configuration.

Steps to reproduce

  1. Deploy Redis for HA using Redis Sentinel (see https://redis.io/topics/sentinel).
  2. Configure Rails cache and session store with Redis Sentinel (https://github.com/redis-store/redis-rails#usage-with-redis-sentinel). Works.
  3. Configure ActionCable Redis with sentinels by supplying the following to config/cable.yml:
development:
  adapter: redis
  url: redis://mymaster
  sentinels:
  - host: redis-sentinel
    port: 26379

Expected behavior

ActionCable will be able to connect to the Redis instance(s) through the Redis sentinel(s).

Actual behavior

SocketError in TweetsController#create
getaddrinfo: nodename nor servname provided, or not known

...

actioncable (5.1.6) lib/action_cable/subscription_adapter/redis.rb:22:in `broadcast'
actioncable (5.1.6) lib/action_cable/subscription_adapter/channel_prefix.rb:6:in `broadcast'
actioncable (5.1.6) lib/action_cable/server/broadcasting.rb:46:in `block in broadcast'
activesupport (5.1.6) lib/active_support/notifications.rb:168:in `instrument'
actioncable (5.1.6) lib/action_cable/server/broadcasting.rb:44:in `broadcast'
actioncable (5.1.6) lib/action_cable/server/broadcasting.rb:23:in `broadcast'

Workaround

Add config/initializers/action_cable.rb as follows:

require 'action_cable/subscription_adapter/redis'

ActionCable::SubscriptionAdapter::Redis.redis_connector = lambda do |config|
  redis_opts = { url: config[:url] }
  redis_opts[:sentinels] = config[:sentinels] if config.key?(:sentinels)
  ::Redis.new(redis_opts)
end

While this is a suitable workaround, it should be easy enough to modify the Redis connector such that it recognizes Redis sentinel configuration if present. For example:

      # Overwrite this factory method for redis connections if you want to use a different Redis library than Redis.
      # This is needed, for example, when using Makara proxies for distributed Redis.
      cattr_accessor(:redis_connector) do ->(config) {
        redis_opts = { url: config[:url] }
        redis_opts[:sentinels] = config[:sentinels] if config.key?(:sentinels)
        ::Redis.new(redis_opts)
      end

If desired, I can gladly create a PR.

System configuration

Rails version: Rails 5.1.6

Ruby version: ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin17]

Redis version: Redis server v=4.0.9 sha=00000000:0 malloc=libc bits=64 build=e0c8d37381c486c6

@rails-bot rails-bot bot added the stale label Jul 28, 2018
@rails-bot
Copy link

rails-bot bot commented Jul 28, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot closed this as completed Aug 4, 2018
@rails-bot rails-bot bot removed the stale label Apr 19, 2019
@georgeclaghorn
Copy link
Contributor

This was fixed in #36053.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants