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

[Cache]: setSentinelTimeout fails when only a single Redis Sentinel server is specified #33796

Closed
Rohaq opened this issue Oct 1, 2019 · 0 comments · Fixed by #33797
Closed

Comments

@Rohaq
Copy link
Contributor

Rohaq commented Oct 1, 2019

Symfony version(s) affected: 4.4.x-dev [660b7b4bb4ae391ef2c61d73d258c8ef3c563b66]

Description
In a Redis Sentinel setup, if only a single Redis Sentinel is specified (as we're currently doing in a test environment), then the call to setSentinelTimeout in RedisTrait.php throws the exception:

Call to undefined method Predis\Connection\StreamConnection::setSentinelTimeout()

Looking briefly through the code, it appears that if only a single host has been specified, it never initialises properly as a SentinelReplication interface, instead defaulting as a StreamConnection. The setSentinelTimeout method isn't present, and so it throws an exception trying to call it.

How to reproduce

// WORKING: Two different hosts
$redisSentinels = ['192.168.0.1:26379', '192.168.0.2:26379'];

// NOT WORKING: Single host, or the same host multiple times. i.e. You can't "trick" it.
$redisSentinels = ['192.168.0.1:26379'];
$redisSentinels = ['192.168.0.1:26379', '192.168.0.1:26379'];

$redisAuthPassword = 'mypass';
$dsnHostsString = sprintf('?host[%s]', implode(']&host[', $redisSentinels));
$redisDsnString = sprintf('redis:%s@%s', $redisAuthPassword, $dsnHostsString);

$redisOptions = array(
    'compression' => true,
    'redis_sentinel' => 'name-of-redis-service'
);

$redisClient = RedisAdapter::createConnection($redisDsnString, $redisOptions);

Possible Solution
In RedisTrait.php, line 274, change:

if (1 === \count($hosts) && !$params['redis_cluster']) {

to

if (1 === \count($hosts) && !($params['redis_cluster'] || $params['redis_sentinel'])) {

This will prevent the line below from turning the list of host configs into a single host config when the 'redis_sentinel' parameter is present, and sets up the Connection class properly, preventing the error.

Additional context
Again, this is an unusual bug, in that when using Redis Sentinel you should generally have more than one Sentinel available, but it does cause issues in test platform infrastructure where this may not be the case.

fabpot added a commit that referenced this issue Oct 2, 2019
…specified (Rohaq)

This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] Fixed Redis Sentinel usage when only one Sentinel specified

Added check for $params['redis_sentinel'] to line 274, as by converting the array of hosts to a single host configuration (as you might in a test environment), this causes the class to initialise incorrectly.

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #33796
| License       | MIT
| Doc PR        |

As from Issue #33796
In a Redis Sentinel setup, if only a single Redis Sentinel, or multiple Sentinels with the same host/port are specified (as we're currently doing in a test environment), then the call to `setSentinelTimeout` in `RedisTrait.php` throws an exception, as the wrong interface appears to be initialised as a result.

Adding a check for the `redis_sentinel` parameter, as was done with a check for the `redis_cluster` parameter, avoids this, and the Redis Client is initialised correctly.

Commits
-------

13233fc Fixed Redis Sentinel usage when only one Sentinel specified
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants