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

Bugfix #41302 - Add an explicit reconnect for failed Redis connections #41502

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/Illuminate/Redis/Connections/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ abstract class Connection
*/
protected $events;

/**
* Indicates if the connection is invalid and needs to reconnect.
*
* @var bool
*/
public $shouldReconnect = false;

/**
* Subscribe to a set of given channels for messages.
*
Expand Down Expand Up @@ -108,12 +115,31 @@ public function psubscribe($channels, Closure $callback)
* @param string $method
* @param array $parameters
* @return mixed
*
* @throws \Exception Throws an exception if this connection should not be used
*/
public function command($method, array $parameters = [])
{
if ($this->shouldReconnect) {
// shouldReconnect is checked by RedisManager before returning this connection.
// If this exception is triggered, it means some other codepath is using this
// which should also be made aware of the public bool shouldReconnect.

// Basically - if you see this, this is a bug in whatever is NOT using RedisManager
// to get Redis objects.
throw new \Exception('The Redis connection '.$this->name.' has been marked as failed and needs to be recreated.');
}
$start = microtime(true);

$result = $this->client->{$method}(...$parameters);
try {
$result = $this->client->{$method}(...$parameters);
} catch (\RedisException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you import all classes?

Copy link
Contributor Author

@xrobau xrobau Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Ah, you were asking for this to be used at the top, rather than explicitly quoted as a root namespace in the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Redis/Connections/PhpRedisConnection.php#L534

// If there is a RedisException, this connection is no longer usable.
// Mark the connection as shouldReconnect, and re-throw the Exception
$this->shouldReconnect = true;

throw $e;
}

$time = round((microtime(true) - $start) * 1000, 2);

Expand Down
12 changes: 10 additions & 2 deletions src/Illuminate/Redis/RedisManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,19 @@ public function connection($name = null)
$name = $name ?: 'default';

if (isset($this->connections[$name])) {
return $this->connections[$name];
// If the connection has had a fatal socket error, the \Redis object
// in this connection is no longer valid, and needs to be destroyed
// and recreated.
if (! $this->connections[$name]->shouldReconnect) {
return $this->connections[$name];
}
// Unset and destroy the Connection object to ensure it's cleaned up.
unset($this->connections[$name]);
Copy link
Contributor Author

@xrobau xrobau Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the unset is here to make sure that the isset always returns false from now on, even if the configure below fails.

}

return $this->connections[$name] = $this->configure(
$this->resolve($name), $name
$this->resolve($name),
$name
);
}

Expand Down