Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Cache] Fix Redis6Proxy #51754

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

Merged
merged 1 commit into from
Sep 27, 2023
Merged

[Cache] Fix Redis6Proxy #51754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
[Cache] Fix Redis6Proxy
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Sep 26, 2023
commit 65540779e9cef7f66e38378083e31dade08a58d6
4 changes: 1 addition & 3 deletions 4 src/Symfony/Component/Cache/Tests/Traits/RedisProxiesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ public function testRelayProxy()
public function testRedis6Proxy($class, $stub)
{
if (version_compare(phpversion('redis'), '6.0.0', '<')) {
$this->markTestIncomplete('To be re-enabled when phpredis v6 becomes stable');

$stub = file_get_contents("https://raw.githubusercontent.com/phpredis/phpredis/develop/{$stub}.stub.php");
$stub = preg_replace('/^class /m', 'return; \0', $stub);
$stub = preg_replace('/^return; class ([a-zA-Z]++)/m', 'interface \1StubInterface', $stub, 1);
Expand All @@ -108,7 +106,7 @@ public function testRedis6Proxy($class, $stub)
continue;
}
$return = $method->getReturnType() instanceof \ReflectionNamedType && 'void' === (string) $method->getReturnType() ? '' : 'return ';
$methods[] = "\n ".ProxyHelper::exportSignature($method, false, $args)."\n".<<<EOPHP
$methods[] = "\n ".str_replace('timeout = 0.0', 'timeout = 0', ProxyHelper::exportSignature($method, false, $args))."\n".<<<EOPHP
{
{$return}(\$this->lazyObjectState->realInstance ??= (\$this->lazyObjectState->initializer)())->{$method->name}({$args});
}
Expand Down
8 changes: 4 additions & 4 deletions 8 src/Symfony/Component/Cache/Traits/Redis6Proxy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public function config($operation, $key_or_settings = null, $value = null): mixe
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->config(...\func_get_args());
}

public function connect($host, $port = 6379, $timeout = 0.0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0.0, $context = null): bool
public function connect($host, $port = 6379, $timeout = 0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0, $context = null): bool
{
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->connect(...\func_get_args());
}
Expand Down Expand Up @@ -686,12 +686,12 @@ public function object($subcommand, $key): \Redis|false|int|string
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->object(...\func_get_args());
}

public function open($host, $port = 6379, $timeout = 0.0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0.0, $context = null): bool
public function open($host, $port = 6379, $timeout = 0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0, $context = null): bool
{
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->open(...\func_get_args());
}

public function pconnect($host, $port = 6379, $timeout = 0.0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0.0, $context = null): bool
public function pconnect($host, $port = 6379, $timeout = 0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0, $context = null): bool
{
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->pconnect(...\func_get_args());
}
Expand Down Expand Up @@ -736,7 +736,7 @@ public function pipeline(): \Redis|bool
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->pipeline(...\func_get_args());
}

public function popen($host, $port = 6379, $timeout = 0.0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0.0, $context = null): bool
public function popen($host, $port = 6379, $timeout = 0, $persistent_id = null, $retry_interval = 0, $read_timeout = 0, $context = null): bool
{
return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->popen(...\func_get_args());
}
Expand Down
14 changes: 7 additions & 7 deletions 14 src/Symfony/Component/Messenger/Bridge/Redis/Tests/Transport/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public function testKeepGettingPendingMessages()
$redis = $this->createMock(\Redis::class);

$redis->expects($this->exactly(3))->method('xreadgroup')
->with('symfony', 'consumer', ['queue' => 0], 1, null)
->with('symfony', 'consumer', ['queue' => 0], 1, 1)
->willReturn(['queue' => [['message' => json_encode(['body' => 'Test', 'headers' => []])]]]);

$connection = Connection::fromDsn('redis://localhost/queue', [], $redis);
Expand Down Expand Up @@ -212,7 +212,7 @@ public function testGetPendingMessageFirst()
$redis = $this->createMock(\Redis::class);

$redis->expects($this->exactly(1))->method('xreadgroup')
->with('symfony', 'consumer', ['queue' => '0'], 1, null)
->with('symfony', 'consumer', ['queue' => '0'], 1, 1)
->willReturn(['queue' => [['message' => '{"body":"1","headers":[]}']]]);

$connection = Connection::fromDsn('redis://localhost/queue', [], $redis);
Expand All @@ -237,11 +237,11 @@ public function testClaimAbandonedMessageWithRaceCondition()
->willReturnCallback(function (...$args) {
static $series = [
// first call for pending messages
[['symfony', 'consumer', ['queue' => '0'], 1, null], []],
[['symfony', 'consumer', ['queue' => '0'], 1, 1], []],
// second call because of claimed message (redisid-123)
[['symfony', 'consumer', ['queue' => '0'], 1, null], []],
[['symfony', 'consumer', ['queue' => '0'], 1, 1], []],
// third call because of no result (other consumer claimed message redisid-123)
[['symfony', 'consumer', ['queue' => '>'], 1, null], []],
[['symfony', 'consumer', ['queue' => '>'], 1, 1], []],
];

[$expectedArgs, $return] = array_shift($series);
Expand Down Expand Up @@ -273,9 +273,9 @@ public function testClaimAbandonedMessage()
->willReturnCallback(function (...$args) {
static $series = [
// first call for pending messages
[['symfony', 'consumer', ['queue' => '0'], 1, null], []],
[['symfony', 'consumer', ['queue' => '0'], 1, 1], []],
// second call because of claimed message (redisid-123)
[['symfony', 'consumer', ['queue' => '0'], 1, null], ['queue' => [['message' => '{"body":"1","headers":[]}']]]],
[['symfony', 'consumer', ['queue' => '0'], 1, 1], ['queue' => [['message' => '{"body":"1","headers":[]}']]]],
];

[$expectedArgs, $return] = array_shift($series);
Expand Down
1 change: 1 addition & 0 deletions 1 src/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,7 @@ public function get(): ?array
$this->group,
$this->consumer,
[$this->stream => $messageId],
1,
1
Copy link
Member Author

@nicolas-grekas nicolas-grekas Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@alexander-schranz can you please confirm that this change makes sense to you? It adds explicit BLOCK 1 to the xreadgroup command. (ext-redis 5 & 6 don't have the same default for this argument apparently, I'm trying to figure out how we should write the code to deal with both versions.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the new default, what was the previous one? I try to understand / find the change inside the phpredis extension, did you already find that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The C code looks a but blurry to me. It looks like the behavior depends on the number of arguments, not on the actual values :'(
Is there a value of BLOCK that works for the need here? eg this 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

In generally I don't see a problem of a Block for 1ms to wait for any messages. The PHP Extension part for me between 5.3.7 and 6.0.1 did look the same so maybe its inside Redis itself? Or was just the redis extension updated? Another possibility would be that we check for the version via phpversion('redis'); but that just returns the extension version not the server version. I try to find out when the added noblock feature as I maybe think its related to that changes 🤔 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me merge this to make tests green but please let me know if you discover 1 isn't what we need.

);
} catch (\RedisException|\Relay\Exception $e) {
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.