laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.49k stars 11.01k forks source link

[5.1] [5.2] Cache tags in Redis does not flush() any items cached via remember() or put() #9967

Closed swayok closed 8 years ago

swayok commented 9 years ago

Laravel v5.1.10 Driver: Redis with predis

This does not remove 'test' item: \Cache::tags(['user'])->put('test', 'value', 25); \Cache::tags(['user'])->flush();

while next code removes 'test' item as expected: \Cache::tags(['user'])->forever('test', 'value'); \Cache::tags(['user'])->flush();

GrahamCampbell commented 9 years ago

Forever just calls the put method.

GrahamCampbell commented 9 years ago

Oh wait, I see. Not in redis.

GrahamCampbell commented 9 years ago

Ping @taylorotwell.

swayok commented 9 years ago

It looks like there are only methods for forever keys. For put() there is even no special tagging key created in Redis and no flush() for such keys.

GrahamCampbell commented 9 years ago

Ping @taylorotwell.

pekhota commented 9 years ago

Made a test on the last version of homestead. Works correct.

swayok commented 9 years ago

@aslikeyou https://github.com/laravel/framework/blob/master/src/Illuminate/Cache/RedisTaggedCache.php stil has no put() method overriding - it just cannot work properly without it with redis. Are you sure you used put()? In latest stable version I still have no caching for put(), only forever() works.

pekhota commented 9 years ago

There is method put() in this file: https://github.com/laravel/framework/blob/master/src/Illuminate/Cache/TaggedCache.php which RedisTaggedCache is extending. I updated laravel/framework via composer and I have cbf68c02542e3d668086283c1928347d1e6f605f in my composer.lock now. Then I open put() method in TaggedCache.php and add a few lines of code:

 public function put($key, $value, $minutes)
    {
        $minutes = $this->getMinutes($minutes);
        (new Dumper())->dump("Call from tagged cache!");
        (new Dumper())->dump(__CLASS__);
        (new Dumper())->dump(get_class($this));
        if (! is_null($minutes)) {
            $this->store->put($this->taggedItemKey($key), $value, $minutes);
        }
    }

Then in route.php I created test method and wrote the next:

use Illuminate\Support\Debug\Dumper;
Route::get('/test', function() {
    (new Dumper())->dump(\Cache::getStore());

    \Cache::tags(['user'])->put('test', rand(10, 100), 25);
    (new Dumper())->dump(\Cache::tags(['user'])->get('test'));
    \Cache::tags(['user'])->flush();
    (new Dumper())->dump(\Cache::tags(['user'])->get('test'));
});

And this is what I have in browser: https://www.dropbox.com/s/gpv5ssgvhkmkbcb/Screenshot%202015-10-04%2018.49.29.png

If I don't understand something - please tell me.

swayok commented 9 years ago

@aslikeyou This is quite strange. I've made some tests earlier to make sure it isn't working and in my project it still caches nothing =( I should try again.

mstephens commented 8 years ago

This is still present in 5.2, any ideas when this may be released?

Cache::tags(['hello'])->put('world', 'hello', 10);
Cache::tags(['hello'])->flush();
taylorotwell commented 8 years ago

What is still present? You're saying that syntax doesn't work in 5.2?

mstephens commented 8 years ago

The fact it doesn't flush when associated with tags.

mstephens commented 8 years ago

Hello guys.

The issue @swayok and I mentioned with flushing cache tags (in this example Redis) is still present.

The following does not work:

Cache::tags(['hello'])->put('world', 'hello', 10);
Cache::tags(['hello'])->flush();

However, when used with forever cache tags it does flush correctly:

Cache::tags(['hello'])->forever('world', 'hello', 10);
Cache::tags(['hello'])->flush();

It is associated with this line where only forever tags are flushed via the "deleteForeverKeys" method. As the logic is already there to extract cache tags from storage, could this be modified to also remove those that aren't forever cache items? I see no reason why this would become a problem?

Thanks

mstephens commented 8 years ago

Hey guys, any news if this will make the next release? I know testing is dull but should this have been picked up?

Tickthokk commented 8 years ago

+1

Laravel 5.1.29, Cache using Redis - Update: Upgraded from 5.1.20 to 5.1.29 just to make sure I had the latest, issue is still there

Any example I could post is exactly what @swayok or @mstephens has already posted.

Tags and ->forever/->rememberForever would ->flush() properly. Tags and ->put/->remember would not flush.

mstephens commented 8 years ago

@Tickthokk If I had time I'd try to look at this myself :(. Seems a significant big to exist for so long. @taylorotwell appreciate you're busy but do you know if this is on any internal tracker to be fixed currently?

yansern commented 8 years ago

+1 This is not working for me either!

arun-chaudhary commented 8 years ago

+1 How long this is fixed?

mstephens commented 8 years ago

Guys, see fix above that I've just committed. Had to separate out much of the logic there, whilst keeping backwards functionality.

Let me know your thoughts and if this works for you.

mkwsra commented 8 years ago

+1 Same issue here,

version 5.2.22

mstephens commented 8 years ago

@mkwsra Do you have any further details on the exact problem to replicate? Looks like none of the cache classes have changed since 5.2.15? Also to confirm it's Redis? Do you see the "standard" Redis cache key in the store - it'll be prefixed with a hash.

swayok commented 8 years ago

@mstephens In my situation:

  1. Redis
  2. Only tagged cache
  3. Only cache that has expiration (forever caching works fine)
  4. Only flush() called for tagged cache
mstephens commented 8 years ago

@swayok Are you definitely using Laravel 5.2.15 or greater (check composer.lock)? That sounds like the original problem reported. Originally only forever items were flushed.

Also if you had any cache items in your application before updating to 5.2.15 they won't be removed either (as it has to track which keys belong to which tag and this wasn't implemented until then).

mkwsra commented 8 years ago

@mstephens Simply my situation is exactly like @swayok , thanks btw!

So I guess it's not only me!

mstephens commented 8 years ago

@mkwsra When did you update to 5.2.22? Were the items in cache before the update?

mkwsra commented 8 years ago

@mstephens And I don't see that "standard" one

mkwsra commented 8 years ago

@mstephens this is an example of my keys in redis

screen shot 2016-03-03 at 16 45 58

I wasn't using cache at all before update, and I updated into 5.2.22 in one of these days

screen shot 2016-03-03 at 17 02 28
mstephens commented 8 years ago

@mkwsra Can you check vendor\laravel\framework\src\Illuminate\Cache\RedisTaggedCache.php line 30 you should see $this->pushStandardKeys($this->tags->getNamespace(), $key);.

In your example you should also have a key called laravel:*****:standard

mkwsra commented 8 years ago

Bizarre! I don't see standard key, this is another screenshot:

screen shot 2016-03-03 at 17 03 44
mstephens commented 8 years ago

Did you check the PHP file mentioned, should see references to pushStandardKeys?

mstephens commented 8 years ago

PS. Fix went live on the 11th February, so if it was in the first Composer update, it won't be there.

swayok commented 8 years ago

@mstephens I've tested my 1st example on Laravel Framework version 5.2.15. Now it works. But there is another issue with it now:

\Cache::tags(['user', 'test'])->put('test', 'value', 25);
echo \Cache::get('test'); // null
echo \Cache::tags(['user'])->get('test'); //< null
echo \Cache::tags(['test'])->get('test'); //< null
echo \Cache::tags(['test', 'user'])->get('test'); //< null
echo \Cache::tags(['user', 'test'])->get('test'); //< value

This way tags usage are very narrow because i cannot access cache entry even by its id without knowing what tags were used there. I intended to use tags for automatic caching for db querues where there are quite many situations when I need to drop all cache entries by main tag name or by main+helper tag pair. But this way i cannot even access single entry by its cache id

mkwsra commented 8 years ago

@mstephens Yes I did check that mentioned file and it was exactly like yours!

And with those composer updates altogether the problem should be fixed right?

mstephens commented 8 years ago

@mkwsra How are you setting the cache items, via put?

Cache::tags(['test'])->put('hello', 'put', 25);

mkwsra commented 8 years ago

Oops! I'm using remember() not put, is this my problem?? if so I'm so sorry!

mstephens commented 8 years ago

Yep, that's the problem. Remember are forever keys, you need to use standard (put) :+1:

mkwsra commented 8 years ago

@mstephens I'm so sorry! it's totally my fault! thanks a lot for your awesome help :+1:

mstephens commented 8 years ago

@chrisloftus It seems like there could be a bug with the Memcached flush if that's what you're reporting?

Having looked into the code (I don't use Memcached), the flush call on tags, runs the resetTag function which actually just stores a new forever tag.

ctf0 commented 8 years ago

@mstephens i know am kinda late to the party but could u plz tell me what exactly are the use for those keys

"laravel:tag:something:key"
"laravel:56edeb5824fa2247638553:standard"
huglester commented 8 years ago

@ctf0 did you figure it out?

If I understand correct, those :standard etc sufixes are needed when using tags() in the cache

mstephens commented 8 years ago

@ctf0 the first item you mentions stores the cache ID of the array cache item (your second item)

The second item is a Redis list which stores all cache IDs related to that tag.

You will then have each individual cache item (series of numbers and letters).

The first cache tag reference is needed so Laravel knows it always has a common name and can reference the tag.

juanf commented 5 years ago

Hi guys, sorry to post on such an old issue, was this fixed on a newer version or is this behavior by design? I'm on 5.6.39. I saw @swayok package addressing this on github but no further discussion. https://github.com/swayok/alternative-laravel-cache I think tags are not very useful this way.

Thanks.

@mstephens I've tested my 1st example on Laravel Framework version 5.2.15. Now it works. But there is another issue with it now:

\Cache::tags(['user', 'test'])->put('test', 'value', 25);
echo \Cache::get('test'); // null
echo \Cache::tags(['user'])->get('test'); //< null
echo \Cache::tags(['test'])->get('test'); //< null
echo \Cache::tags(['test', 'user'])->get('test'); //< null
echo \Cache::tags(['user', 'test'])->get('test'); //< value

This way tags usage are very narrow because i cannot access cache entry even by its id without knowing what tags were used there. I intended to use tags for automatic caching for db querues where there are quite many situations when I need to drop all cache entries by main tag name or by main+helper tag pair. But this way i cannot even access single entry by its cache id

brazenvoid commented 5 years ago

Now with phpredis, the above package also doesn't work, making it even more of a reason to fix this. Its obvious that Laravel team is uninterested in fixing this and is waiting for someone to fix it for them. In the meanwhile their users get frustrated profiling their systems, finally stumbling on this closed issue and only by reading it thoroughly will they get to know the known issues.

There should have been a known issues section mentioning this as the documentation clearly says otherwise. My team would not have used this forsaken tagged cache, profiled our caching system to discover how broken it is, and now they even have to remake everything to be compatible with regular key value pairs...

swayok commented 5 years ago

@brazenvoid welcome to https://github.com/swayok/alternative-laravel-cache Unfortunately compatibility with phpredis is yet to be added but predis works correctly

brazenvoid commented 5 years ago

@swayok Yes, I understand but all our production servers experienced a 4x redis performance boost at the minimum when we switched to phpredis so a revert is not at all on the cards.

swayok commented 5 years ago

@brazenvoid got it. I think I need to switch my projects to phpredis too. Check a package in a week - I'l try to upgrade it until then

swayok commented 5 years ago

https://github.com/swayok/alternative-laravel-cache - now supports php_redis (since version 5.4.10)