Redis cache store issues on numeric values

[halaei]

• Laravel Version: 6.13.1
• PHP Version: 7.2.27
• Database Driver & Version: Redis

Description:

RedisStore::serialize() and unserialize() don't do right when it comes to caching numbers. I think the code has some considerations that Redis stores strings that are integers more efficient than normal integers. But the implementation is wrong.
From https://redis.io/commands/INCR:

Redis stores integers in their integer representation, so for string values that actually hold an integer, there is no overhead for storing the string representation of the integer.

Steps To Reproduce:

Set cache driver to redis and try the followings:

Cache::put('test', '1');
Cache::get('test');; // expected = actual = "1"
Cache::put('test', 1);
Cache::get('test'); // expected: 1, actual "1"
Cache::put('test', 1.0);
Cache::get('test'); // expected: 1.0, actual "1"
Cache::put('test', INF)
Cache::get('test'); // expected INF, actual: PHP Notice:  unserialize(): Error at offset 0 of 3 bytes
Cache::put('test', -INF)
Cache::get('test'); // expected -INF, actual: PHP Notice:  unserialize(): Error at offset 0 of 4 bytes
Cache::put('test', NAN);
Cache::get('test');// expected: NAN, actual: PHP Notice:  unserialize(): Error at offset 0 of 3 bytes 

I don't know if other drivers have similar issues.

[driesvints]

Managed to reproduce this. Sending in a fix. Also not really sure about other stores.

[halaei]

Should the complete fix be for the next major release, as considering returning 1 instead of "1" be a possible breaking change?

[driesvints]

@halaei I guess this is now completely broken atm and never worked? I sent in a fix here: #31348

Let me know what you think.

[carestad]

Came across this today when we tried switching to redis. Some of the stuff in our cache is type sensitive, so when some of the cache keys went from 1 to "1" the === checks failed.

It seems this is sort of broken for all numeric strings, since is_numeric() is being used, and then skips serializing it. Why not just consistently serialize everything before storing it, like in the file and memcached stores? A quick test shows me that the outcome of Cache::put('test', 123) with the cache drivers file, array, database, apc and memcached all return an integer, but for redis I get a string.

[halaei]

@carestad I agree. These are some reasons for the inconsistency, although I don't think they are good enough:

1. Storing integers without serialization makes it possible to use incr, decr, incrby, and decrby Redis commands to implement Cache::increment() and Cache::decrement() functions.
2. Storing float without serialization makes use of incrbyfloat command possible, altough it is not used in Laravel. So floating point increment is not yet support.
3. (Less important) Storing integers as integer in Redis causes some considerable memory reduction when storing a lot of integers, something near 10x I guess.

I suggest to update Redis driver to only skip serialization if the data type is int, but not when the data is float or a numeric string. This change probably is a breaking change for some other applications.
Some configuration options may be helpful to make things BC and let users to store floats without serialization as well.

[carestad]

@halaei Tough one. It all boils down to the fact that redis don't have its own numerical type I suppose.

But I was not really aware of this before trying it out. I feel like this should be mentioned in the documentation somewhere, that integers and float types will not be preserved when using redis? Especially since all the other cache providers does this (haven't tried dynamodb and apc yet though so I can't say 100% for that one, but I have tested file, array, database and memcached). So switching from anyone of the other cache providers to redis could easily cause problems in type-aware applications.

What about a force_numeric_serialization config option that defaults to false, but can be overridden? Either always force-run the value through serialize() when setting and unserialize() when getting, or force-cast numeric strings to int/float (only when getting). And when that option is set, the increment/decrement functions will need to compute this in PHP instead of using redis' own INCR/DECR. But I'd much rather have consistency between caching providers than how it is right now.

[robin-vendredi]

Just got burned by this as we switched our cache store from database to redis - suddenly this throws an error on the second call:

function foo(): int
{
    return Cache::remember('foo', 60, function() {
        return 1;
    });
}

=> expected int but got string instead

Is there any fix other than manually casting to int everywhere that's the expected type? The fix that closes this issue (#31348) deals with storing INF values, but that's not the whole problem here.

[alies-dev]

@robin-vendredi

you can use PHP’s serialize/unserialize to store type info

[robin-vendredi]

@lptn Thanks for your reply.

If I understand it correctly, this would look something like return serialize(1);? I would be afraid it might lead to double serialization (esp. if I switch cache driver in future), and I'd hope for a more global solution where I don't have to remember that "integer needs to be manually serialized each time" and my code breaks if I forget.

I could write a custom driver extending the RedisStore to not skip the integer serialization they are doing here:

framework/src/Illuminate/Cache/RedisStore.php

Lines 332 to 335 in 6718ae6

	protected function serialize($value)
	{
	return is_numeric($value) && ! in_array($value, [INF, -INF]) && ! is_nan($value) ? $value : serialize($value);
	}

But that would have an impact on the increment/decrement functions too and I'd like to avoid writing my own store. Are there any other solutions?

[AidasK]

Can this be reopened? This inconsistency bug is still present in laravel 8

[aysark]

Running into this whilst profiling/memory tuning redis, if this is fixed, it would provide a big memory saving:

Sets that contain only integers are extremely efficient memory wise. If your set contains strings, try to use integers by mapping string identifiers to integers.
You can either use enums in your programming language, or you can use a redis hash data structure to map values to integers. Once you switch to integers, Redis will use the IntSet encoding internally.

[Jurigag]

Why this is closed when it's not fixed or resolved anyhow? @taylorotwell your PR closed this, not sure if this was mistake or what.