Caches persisting issues when flushing with cache manager

[FinBoWa]

Description

We have had similar issues that this pull request tries to prevent #13.

If you flush the cache with the cache manager there are some cache key orphans left in the system.

The culprit seems to be in the class CacheInclude with the function:

    /**
     * @param string $name
     * @param string $key
     * @param \Heyday\CacheInclude\KeyCreators\KeyCreatorInterface $keyCreator
     * @return void
     */
    protected function addStoredKey($name, $key, KeyCreatorInterface $keyCreator)
    {
        $keys = (array) $this->cache->fetch($name);
        if (!array_key_exists($key, $keys)) {
            if ($keyCreator instanceof KeyInformationProviderInterface) {
                $keys[$key] = $keyCreator->getKeyInformation();
            } else {
                $keys[$key] = true;
            }
            $this->cache->save($name, $keys);
        }
    }

That keeps a list of the keys and the urls that they belong to and the problem comes when you have loads of keys and concurrent requests to the server that gets out of sync.

And as the cache-managers delete function deletes only keys that are find in the array you end up in a situation that you need to manually flush redis cache for the site as the frontend just displays the key without checking it from the "list".

That needs to be changed to use Redis´s own list type to keep it in sync correctly.

Hopefully I'll have a tweak ready for testing in the next couple days.

Steps to repro

• flush redis db
• use a crawler to cralw the site with a single user
• check keys from the redis-cli
• use cache-manager to flush all keys
• check keys from the redis-cli and youll see that only the "list" keys exists
• use a crawler to cralw the site with multiple concurrent requests, in our case it just needs 2
• use cache-manager to flush all keys
• check keys from the redis-cli and youll see list keys and orphaned cache keys

@stecman I added you here as you might be interested in this :)

[camspiers]

I'll have a bit of a think about this. It is definitely a locking issue, we need to lock fetches until the transaction is complete. But doctrine cache doesn't provide this ability. I think using redis list type is going to mean that the issue isn't solved for any provider other than redis.

[camspiers]

BTW thanks heaps for tracking down and reporting this issue. It is definitely a major one for high concurrency websites.

[stecman]

Thanks, @FinBoWa. We found out yesterday that #13 didn't fix the problem it was trying to, so I suspect this issue could also be the cause of the behaviour we're seeing occasionally with CacheInclude.

[camspiers]

Sounds likely to me @stecman

[FinBoWa]

@camspiers Ah.. Off course I forgot the other caches totally! I locked my line of thought to redis only..

Of course the fix should be related to other cache providers so I'll have to revise my idea of the fix that I had. At least we know the reason for issues that has been bugging as for ages :).

I tested this on a two concurrent requests on our local vagrant box. Mostly it happens on lists that contain more than +30 keys and we only use Redis. So I'd of course would like to hear that can you repro the issue also before anything else :).

One hotfix that would help in this would have been to modify the cache manager to delete keys with a wildcard but I haven't checked is that commonly supported with the cache providers.

At least that way if you get a too persistent key that goes missing from the list you could flush all the keys for that project.

Also I haven't checked is the lists used in otherwise that could cause the saves not flushing right. I presume that is the case.. e.g. it deletes keys only found in the list?

Heres some thoughts on the matter.. some of them are crazy and not in the order of feasible to not feasible :) :

• hot fix for clearing the cache: use wildcards.. is it possible?
• can we use wildcards in fetching > if so, move the url information to each key
• list management to db > add constraints to the table and just write a line the db in a try statement.. if it fails there was a line. Modify the manager to query the db.
• use a own lock key > add the new keys that couldn't be added to the list to a queue and write them as soon as possible.
• use a own lock key > if lock is found.. Prevent writing all the cache keys totally > this way the previous update operation is solved. And no new list items would have been added untill a request that can be handled is made.

[FinBoWa]

I think the last one would be the most easiest to implement.. The lock file should have a TTL set so it will be flushed really soon so if the request runs to any errors it would remove them automatically also .

Also it shouldn't prevent writing all keys.. Just for the set.

[FinBoWa]

@camspiers Did you have the change to have any thoughts on the matter yet?

If some my suggestions sounds reasonable I'd be able to allocate time to produce a fix as a pull reguest. Thought testing other cache provides might be out of scope for us, but id test it fully with Redis at least.

[FinBoWa]

I have tidied up the commit and it seems to work.

I'v tested with a local vagrant box and redis with 60 concurrent requests and its not causing any keys to be left around.

[camspiers]

@stecman can you have a look at this. I don't have bandwidth for more work at the moment.

[kinglozzer]

2 years late to the party, but we’ve just encountered this issue too on a high(er) traffic website

[FinBoWa]

We did another tweak also on that Sohova@c878669