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

[8.x] Fix possible out of memory error when deleting values by reference key from cache in Redis driver #40039

Conversation

dkuzmenchuk
Copy link
Contributor

@dkuzmenchuk dkuzmenchuk commented Dec 14, 2021

This PR solves possible out of memory issue when deleting values by reference key from cache in Redis driver. PR contains changes from #39939, but also takes into consideration that Laravel supports 2 Redis clients: phpredis and predis (issue #40037).

Changes from the PR #39939:

  1. In case of empty results of sscan command predis client returns default cursor value and empty array. At the same time phpredis just returns false.
    To process this I suggest break the loop if $valuesChunk is null. Because array destruction on false writes null to all array items.

  2. phpredis returns integer values as cursor. So we should cast $cursor to string to make a correct comparison with a default value.

  3. Options for sscan command are in lowercase to work correct with both Redis clients. Otherwise phpredis processes set by 10 items per iteration

@dkuzmenchuk dkuzmenchuk force-pushed the fix-issue-in-delete-values-on-phpredis-client branch 4 times, most recently from b62f802 to 300f3e6 Compare December 14, 2021 20:27
@dkuzmenchuk dkuzmenchuk force-pushed the fix-issue-in-delete-values-on-phpredis-client branch from 300f3e6 to 03ae4df Compare December 14, 2021 20:32
@dkuzmenchuk dkuzmenchuk changed the title [8.x] Fix issue with deleteValues method on phpredis client [8.x] Fix possible out of memory error when deleting values by reference key from cache in Redis driver Dec 14, 2021
@dkuzmenchuk dkuzmenchuk marked this pull request as ready for review December 14, 2021 20:48
@taylorotwell taylorotwell merged commit 74d5464 into laravel:8.x Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants