[Messenger] Avoid reconnecting an already-connected Redis instance. #52811
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| $connect = isset($params['persistent_id']) ? 'pconnect' : 'connect'; | ||
| $redis->{$connect}($host, $port, $params['timeout'], $params['persistent_id'], $params['retry_interval'], $params['read_timeout'], ...(\defined('Redis::SCAN_PREFIX') || \extension_loaded('relay')) ? [['stream' => $params['ssl'] ?? null]] : []); | ||
|
|
||
| if (!$redis->isConnected()) { |
There was a problem hiding this comment.
What about auth, serializer option and dbindex? should we consider them already initialized?
| if (!$redis->isConnected()) { | |
| if ($redis->isConnected()) { | |
| return $redis; | |
| } |
|
@nicolas-grekas Thank you for the review! I've submitted an updated version against the 5.4 branch in PR #52896. |
…BusterNeece) This PR was merged into the 5.4 branch. Discussion ---------- [Messenger] Avoid reconnecting active Redis connections. | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #45057 | License | MIT With the Messenger component's internal Redis Connection class, if you pass it a \Redis or \Relay instance that's already connected, it will still try to reconnect that instance using the host/port/etc. that's passed to the connection object's "options" array in the constructor. Not only does this cause problems when the Redis instance is already connected, but it also prevents you from, say, using socket connections instead (as the only connection method supported by the class itself is IP-based connections) and just passing in a ready-to-go Redis instance. This PR is a resubmission of #52811 targeting 5.4 as it's a bug fix. Commits ------- 650f153 bug #45057 [Messenger] Avoid reconnecting active Redis connections.
With the Messenger component's internal Redis
Connectionclass, if you pass it a\Redisor\Relayinstance that's already connected, it will still try to reconnect that instance using the host/port/etc. that's passed to the connection object's "options" array in the constructor.Not only does this cause problems when the Redis instance is already connected, but it also prevents you from, say, using socket connections instead (as the only connection method supported by the class itself is IP-based connections) and just passing in a ready-to-go Redis instance.
While I personally use this class heavily in my own applications, I'm not aware of every possible implication of making a change like this, i.e. if there are other parts of the application that are counting on this working as-is (even though I can't imagine that unnecessarily reconnecting is a desired feature in this case). While it's intended to fix a bug in my specific use-case, I'd consider it also arguably "new" functionality that the class can now handle pre-connected instances being passed to it.
Basically, I'm new to contributing to Symfony so I'm a little shaky on my legs here, but the reported issue above mentioned needing a PR, and we're running into the issue ourselves, so here's the PR!