Bugfix #41302 - Add an explicit reconnect for failed Redis connections #41502
Conversation
…nections When a \Redis connection has a socket/connection error, there's no reliable way to re-establish the connection. This adds a simple public 'shouldRestart' bool (based on the existing Queue\Worker 'stopWorkerIfLostConnection' code) to the Redis Connection, which is checked in RedisManager and recreated if needed there. Signed-Off-By: Rob Thomas <xrobau@gmail.com>
| return $this->connections[$name]; | ||
| } | ||
| // Unset and destroy the Connection object to ensure it's cleaned up. | ||
| unset($this->connections[$name]); |
There was a problem hiding this comment.
Note the unset is here to make sure that the isset always returns false from now on, even if the configure below fails.
|
Thanks @xrobau |
|
I don't understand why we would return an empty array on failure. Wouldn't we just want to rethrow the exception and let it be logged, etc? |
|
Updated PR @taylorotwell - I was returning an empty array because the noise was irritating to me, but that's not going to help anyone else 8) |
| $result = $this->client->{$method}(...$parameters); | ||
| try { | ||
| $result = $this->client->{$method}(...$parameters); | ||
| } catch (\RedisException $e) { |
There was a problem hiding this comment.
Edit: Ah, you were asking for this to be used at the top, rather than explicitly quoted as a root namespace in the exception.
There was a problem hiding this comment.
@driesvints this may be resolved simply by ALWAYS reconnecting redis here, rather than only on one specific edge case. Waiting for feedback below, which would simplify this to simply removing the if.
|
Did you examine the code for Furthermore,
|
|
Screenshot above references https://github.com/laravel/framework/blob/9.x/src/Illuminate/Redis/Connections/PhpRedisConnection.php#L533 for anyone with visual issues. @taylorotwell - What would you prefer? That code is currently ONLY catching 'has gone away', which appears to be triggered by a controlled shutdown of Redis. The error that I'm attempting to fix is when an UNCONTROLLED shutdown happens, or a network issue. Anecdotally, I've never had any exception thrown by Phpredis that is recoverable at all. Was there any reason that only one edge case is handled? |
This is an alternative option to PR laravel#41502, and adds a match for 'socket' ("socket error when (reading|writing)") to trigger a reconnection. I am not sure if this is valid in other languages, too. My personal preference would be to remove the entire if and ALWAYS reconnect on any Exception Signed-Off-By: Rob Thomas xrobau@gmail.com
|
Alternative PR #41546 - as I mention in the PR, I don't think this is suitable, and would prefer to remove the |
* Bugfix #41302 - Alternative This is an alternative option to PR #41502, and adds a match for 'socket' ("socket error when (reading|writing)") to trigger a reconnection. I am not sure if this is valid in other languages, too. My personal preference would be to remove the entire if and ALWAYS reconnect on any Exception Signed-Off-By: Rob Thomas xrobau@gmail.com * Fix typo (used vim instead of my usual IDE)
When a \Redis connection has a socket/connection error, there's no
reliable way to re-establish the connection. This adds a simple
public 'shouldRestart' bool (based on the existing Queue\Worker
'stopWorkerIfLostConnection' code) to the Redis Connection, which
is checked in RedisManager and recreated if needed there.
Signed-Off-By: Rob Thomas xrobau@gmail.com