Skip to content

[Lock] Lock::acquire will always return false when using RedisStore #59387

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
Closed
PatNowak opened this issue Jan 7, 2025 · 10 comments · Fixed by #59488 or #59521
Closed

[Lock] Lock::acquire will always return false when using RedisStore #59387

PatNowak opened this issue Jan 7, 2025 · 10 comments · Fixed by #59488 or #59521
Labels
Bug Lock Status: Needs Review

Comments

@PatNowak
Copy link

PatNowak commented Jan 7, 2025

Symfony version(s) affected

7.2, also tested 7.3.x-dev 9169931

Description

We're using Valkey 7.2.4 (more less Redis fork) and it works perfectly fine on Sf 6.4.
On 7.2 (tested dev branches and main) we got issue with Lock->acquire. It uses under the hood RedisStore->save and the $this->evaluate (https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Lock/Store/RedisStore.php#L86) .

The redis tab output:
Zrzut ekranu z 2025-01-07 09-23-57

How to reproduce

I believe the difference is between eval and evalSha, so if you please try to create lock and acquire it using Redis.
We're actually using Valkey (valkey/valkey:8 docker image, https://hub.docker.com/r/valkey/valkey/).

Possible Solution

No response

Additional Context

Might be similar to #58660
@PatNowak PatNowak added the Bug label Jan 7, 2025
@xabbuh
Copy link
Member

xabbuh commented Jan 7, 2025

Can you test if #59348 fixes this too?

@PatNowak
Copy link
Author

PatNowak commented Jan 7, 2025

Can you test if #59348 fixes this too?

I don't see a way how I can get this fix using composer and packagist. When trying to get 7.2.x-dev I get 7.3.x-dev instead without it. 7.2 doesn't have it as well :/

@ro0NL
Copy link
Contributor

ro0NL commented Jan 7, 2025 •
edited
Loading

it's merged into 7.2 branch, which has not been merged into 7.3 yet. It will be available in next patch release.

@xabbuh
Copy link
Member

xabbuh commented Jan 7, 2025

The changes have been merged up into the 7.3 class.

@PatNowak
Copy link
Author

PatNowak commented Jan 8, 2025 •
edited
Loading

I fetched latest fix. Previously it was fine on Redis and Valkey had timeout 15 seconds and then it got error. Now the error on Valkey is immediately, redis is still working great.

I tested docker images:

  • redis:6.2.12
  • redis:7.2 (redis_version:7.2.7)
  • redis:7.4 (redis_version:7.4.2)
  • redis:8.0-M02-bookworm (redis_version:7.9.225)
  • valkey:8 (redis_version:7.2.4)
  • valkey/valkey:latest (redis_version:7.2.4)

and only valkey had issues. So... most likely there are changes under the hood in in that engine and RedisStore doesn't work 100% with that using evalSha. The eval worked fine in Sf 6.4...

The error when using lock::acquire:
Zrzut ekranu z 2025-01-08 08-25-16

@ro0NL
Copy link
Contributor

ro0NL commented Jan 8, 2025

well it's a RedisStore, not a ValkeyStore 😅 perhaps it needs a dedicated store instead.

@ro0NL
Copy link
Contributor

ro0NL commented Jan 8, 2025

i can confirm #59348 fixed our issue (#59087) on redis:6.2.16-alpine using predis/predis:v2.3.0

@PatNowak
Copy link
Author

PatNowak commented Jan 8, 2025

well it's a RedisStore, not a ValkeyStore 😅 perhaps it needs a dedicated store instead.

Yeah, that might be the case ;) I hoped the fork will work the same. Thanks for your time and efforts. I'll understand if the issue will be closed now. I just hope you might have ValkeyStore on your mind then :) Most likely more and more people will start to use it after Redis changed their licensing.

@PatNowak
Copy link
Author

PatNowak commented Jan 8, 2025 •
edited
Loading

I've made another test. Our app is using phpredis, so I removed it and used predis.
It was exactly the same: fine on 6.4 (with valkey), same errors on 7.2 and 7.3.x-dev.

@stof
Copy link
Member

stof commented Jan 8, 2025

the PHP Redis extension has updated its description to say it is an extension providing support for the REdis Serialization Protocol (RESP for short) rather than only for Redis. As Valkey implements the RESP protocol, I would suggest you to report this to Valkey as well if their EVALSHA command does not work the same.

This was referenced Jan 9, 2025
Open
Merged
fabpot added a commit that referenced this issue Jan 13, 2025
@fabpot
bug #59488 [Lock] Make sure RedisStore will also support Valkey (PatN…
a8a9e0b 
…owak)

This PR was submitted for the 7.3 branch but it was squashed and merged into the 7.2 branch instead.

Discussion
----------

[Lock] Make sure RedisStore will also support Valkey

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #59387
| License       | MIT

As the change happened on Valkey (https://github.com/valkey-io/valkey/pull/617/files#diff-1abc5651133d108c0c420d9411925373c711133e7748d9e4f4c97d5fb543fdd9L1819-R1819), ~it's hard to consider this a bug when `RedisStore` was meant only to support Redis ;)~

Commits
-------

15d6b5e [Lock] Make sure RedisStore will also support Valkey
@fabpot fabpot closed this as completed Jan 13, 2025
@PatNowak PatNowak mentioned this issue Jan 16, 2025
Merged
fabpot added a commit that referenced this issue Jan 17, 2025
@fabpot
minor #59521 Issue 59387-2: make check with prefix more robust (PatNo…
561f1fb 
…wak)

This PR was squashed before being merged into the 7.2 branch.

Discussion
----------

Issue 59387-2: make check with prefix more robust

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? |no
| Issues        | Fix #59387
| License       | MIT

Making the #59387 more robust and future proof.

Commits
-------

e1af127 Issue 59387-2: make check with prefix more robust
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Lock Status: Needs Review
Projects
None yet
Milestone
No milestone
6 participants
@fabpot @stof @ro0NL @xabbuh @PatNowak @carsonbot