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

[dkuzmenchuk]

Hi there,
This PR solves possible out of memory issue when deleting values by reference key from cache in Redis driver.

Example:
$values = array_unique($this->store->connection()->smembers($referenceKey));

Issue:
If there are too many values referenced by key, we get out of memory error. In this case deleting values using chunks does not make sense because memory overflows earlier.

Solution:
I suggest to use sscan to retrieve values from Redis by chunks using cursor. Then remove all unique values in chunk and get new chunk. Repeat before all values have been processed.

[gdebrauwer]

I'm having a similar issue with the flush-method on the RedisTaggedCache-class, which uses the deleteValues method. I have so many keys with a certain tag, that deleting all those keys takes too much time and causes a timeout in Laravel Vapor.

I read that it is possible to delete all keys within a set using the del command.

// will delete all members of "myset"
del myset

Would it not be a better idea to let the deleteValues method just use the delcommand? 🤔 That would be a lot faster I think.

[taylorotwell]

@dkuzmenchuk any thoughts on @gdebrauwer's comment?

[taylorotwell]

Marking this as draft for now pending feedback on comments above. Please mark as ready for review again when you're ready for me to look at it.

[dkuzmenchuk]

Hi there,
I needed a time to did a small research about how tagged values are stored by the Redis driver.

There is a method which used to delete cached values

  protected function deleteKeysByReference($reference)
  {
        foreach (explode('|', $this->tags->getNamespace()) as $segment) {
            $this->deleteValues($segment = $this->referenceKey($segment, $reference));

            $this->store->connection()->del($segment);
        }
  }

  protected function deleteValues($referenceKey)
  {
        $values = array_unique($this->store->connection()->smembers($referenceKey));

        if (count($values) > 0) {
            foreach (array_chunk($values, 1000) as $valuesChunk) {
                $this->store->connection()->del(...$valuesChunk);
            }
        }
  }

If I understood @gdebrauwer comment correct, he suggests to simply remove $referenceKey which is passed into smembers. In this case we will remove just a set containing keys which in turn store references to cached values. As you can see, after deleteValues set also removed.
Sure, if we will only delete set, it will be very fast. But cached values are not removed in this case. There is a possible memory leak if values stored without ttl (i.e. forever). So we'll remove set, but plain key-value items will remain. Please see picture below to be clear with my explanation.

Please let me know if you have any comments.

[gdebrauwer]

I think we can do the following:

protected function deleteValues($referenceKey)
{
    $this->store->connection()->del($referenceKey);
}

If I understand it correctly, $referenceKey is the name of a redis set. The smembers (or sscans) redis command, that is currently used, fetches the keys within that set and then deletes the keys and their values use the del redis command.

Apparently you can also do this in one command, by providing the set name to the del command:

[dkuzmenchuk]

Method deleteValues does not delete values from set. It uses values into the set as references (keys) to another values. So if you delete only set, you do not flush cached values. You just delete references to cached values.

As you can see values referenced by foo1 and foo2 are still exist after set deleted.

[gdebrauwer]

Of course, sorry, I was completely wrong 🤦‍♂️ Thanks for the explanation 🙂

[taylorotwell]

Thanks for contributing to Laravel! ❤️

[robertmarney]

@dkuzmenchuk - What mock tool are you using for testing Redis? The m6web/redis-mock does not implement sscan.

[dkuzmenchuk]

@robertmarney As I see, Laravel does not use any Redis-specific mock tool for testing Redis. If you check a test case, you'll see that redis connection is just a mock on stdClass.

...
$store->shouldReceive('connection')->andReturn($conn = m::mock(stdClass::class));

$conn->shouldReceive('sscan')->once()->with('prefix:foo:forever_ref', '0', ['match' => '*', 'count' => 1000])->andReturn(['0', ['key1', 'key2']]);
...

I used the same way to write test for my feature.

[akalongman]

I think we should add test to cover this case #40149 after fixing