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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Add exponential backoff when reading from Redis #3344

Open
wants to merge 2 commits into
base: 8.3
Choose a base branch
from
Open

BUGFIX: Add exponential backoff when reading from Redis #3344

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db497e5
BUGFIX: Add exponential backoff when reading from Redis
kdambekalns Apr 18, 2024
1f63a30
Use ** instead of ^ for exponentiation
kdambekalns Apr 29, 2024
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
13 changes: 12 additions & 1 deletion Neos.Cache/Classes/Backend/RedisBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,18 @@ private function calculateExpires(string $tag, int $lifetime): int
*/
public function get(string $entryIdentifier): string|bool
{
return $this->uncompress($this->redis->get($this->getPrefixedIdentifier('entry:' . $entryIdentifier)));
$tries = 1;
do {
try {
return $this->uncompress($this->redis->get($this->getPrefixedIdentifier('entry:' . $entryIdentifier)));
} catch (\RedisException $exception) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems dangerous to me.. Isn't there at least a more specific exception that is thrown when "Redis is not ready (yet)"?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, you can only inspect the error message.

But then again, even if it's something else, why not try again? What could be dangerous?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure but I've seen so many errors in the past due to silently caught exceptions that those make me suspicious.
Also we have seen some performance issues with exponential back offs in the past (and are replacing those in Neos 9). For example: Some error in Redis will now lead to a 20s+ delay (if I counted correctly) until it is displayed. A wrong configuration could easily build up to kill the server upon deployments.

What about moving this logic to the getStatus() implementation and make sure that it is called upon deployment?
And/or implement the WithSetupInterface and put it there

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe logging the exceptions would be good – after all, at that point nothing should go wrong.

About killing something on a deployment: The use case for me here is exactly a deployment failure. 🙃

Adding something else to explicitly call won't happen (in "my" current project), as we have that workaround already: Call doctrine:compileproxies after deployment until no error appears… 🙈

Copy link
Member

Choose a reason for hiding this comment

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

About killing something on a deployment: The use case for me here is exactly a deployment failure.

Right, it fixes the error you described in the issue, but it might create a new one for misconfigured backends.
I won't block this, but I'm not a big fan as you might be able to tell :)
What about turning this into a composition, i.e. introduce some RetryBackend that can be wrapped around any other backend (similar to the MultiBackend)?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is also an option…

Copy link
Member

Choose a reason for hiding this comment

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

I think that the PDO and Redis backend should both have a automatic retry implementation like this by default. In modern hosting environments, there will be movement and it can happen that a Redis is gone for a second or two once in a while. Therefore, it would be nice if a Flow dev wouldn't have to think about this and her application would just work.

I see the point of @bwaidelich that sometimes retries can worsen things. You need to apply the "circuit braker" design pattern for those cases. However, it depends …

I wouldn't like to introduce another wrapper backend since it makes the setup complicated and I think that a reasonable retry is what most people want.

How about making this configurable (on / off and number of retries) using the same option names for RedisBackend, PDOBackend and any other backend which could profit from this?

if ($tries > 8) {
throw $exception;
}
usleep($tries**2 * 100000);
$tries++;
}
} while (true);
}

/**
Expand Down
Loading