Skip to content

Fix dealing with ext-redis' multi/exec returning a bool #47979

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

Merged

Conversation

joaopbnogueira
Copy link

@joaopbnogueira joaopbnogueira commented Oct 24, 2022

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #46384
License MIT

When using phpredis client, the execution of multi/exec might return a boolean instead of an array, leading to an error when yielding the result.

@OskarStark
Copy link
Contributor

Could you please add a testcase to avoid further regressions? Thanks

@joaopbnogueira
Copy link
Author

joaopbnogueira commented Oct 24, 2022

Hey @OskarStark, now I'm puzzled. This was triggering an error (failure to set the key/value with TTL) and I was able to set a breakpoint (on my project) and observe the $results as being true, exactly as described by another developer on issue #46384.

Was trying to reproduce it using a test but it seems to be working fine, so I'm unsure about the conditions that trigger the exec call to return true instead of an array.

The documentation does seem to hint that exec might return a bool: https://github.com/phpredis/phpredis/blob/develop/redis.stub.php

I was able to verify that even if the cache set fails (due to this issue), the value is correctly written to Redis.

For further context, I'm running this on a long-lived PHP process and the issue only seems to manifest itself on subsequent requests, but not on the first one.

I've run tests using phpredis 5.3.7 as well as 5.3.4 (the one used on symfony's integration tests) and the results are the same.

Switching to Predis\Client solves the issue, but this is less than ideal from a performance standpoint.

@fabpot fabpot modified the milestones: 4.4, 5.4 Nov 23, 2022
@nicolas-grekas nicolas-grekas changed the title fix symfony/symfony#46384 [Cache] Fix dealing with ext-redis' multi/exec returning a bool Dec 13, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to 5.4 December 13, 2022 17:43
@nicolas-grekas nicolas-grekas force-pushed the fix/4.4-phpredis-cache-trait branch from c729ba4 to 813f241 Compare December 13, 2022 17:43
@nicolas-grekas nicolas-grekas force-pushed the fix/4.4-phpredis-cache-trait branch from 813f241 to 4d2b176 Compare December 13, 2022 17:55
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Let's give this a try. Please report back if you see any unexpected behavior after this change.

@nicolas-grekas
Copy link
Member

Thank you @joaopbnogueira.

@carsonbot carsonbot changed the title [Cache] Fix dealing with ext-redis' multi/exec returning a bool Fix dealing with ext-redis' multi/exec returning a bool Dec 13, 2022
@nicolas-grekas nicolas-grekas merged commit 73e03fb into symfony:5.4 Dec 13, 2022
@fabpot fabpot mentioned this pull request Dec 16, 2022
This was referenced Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants