opt: memo cache write operations override store values

[KIKOmanasijev]

Hello,

I was just watching @taylorotwell's Keynote at Laracon, and saw him presenting an example about the new memo() cache features. It was this example, to be more precise:

Current implementation

 // roundtrip request #1, since 'name' is not in the memo store.
$name = Cache::memo()->get('name');

 // roundtrip request #2, updates the 'name' in the cache store.
Cache::memo()->put('name', 'Taylor', 10);

// roundtrip request #3, since 'name' was invalidated in the previous command
$name = Cache::memo()->get('name');

So this example got me wondering, why do we need to have 3 roundtrips to the cache store? Why is the Cache::memo()->put() method not memoizing the value?

Unnecessary Cache Roundtrips

The issue I see is that this will make 3 roundtrips to the underlying cache store, because the put() command invalidates the existing memoized value. Instead of storing invalidating the cache, I think overriding the existing value would make more sense, and it could save an extra roundtrip request to the cache store.

After the proposed solution

 // roundtrip request #1, since 'name' is not in the memo store.
$name = Cache::memo()->get('name');

 // roundtrip request #2, updates the 'name' in the cache store AND stores it in the memo store.
Cache::memo()->put('name', 'Taylor', 10);

// no roundtrip request, since the value is already in the memo store.
$name = Cache::memo()->get('name');

Disclaimer:

• This is a breaking change, so I am targeting the master branch.

Note:

• This year's Laracon was one of the best one's I've seen! It's a good time to be a Laravel developer. Thanks to Taylor and the team 🔥🔥🔥

[KIKOmanasijev]

The failing tests are not related to my changes.

[taylorotwell]

Want to get a review from @timacdonald on this one 👍

[timacdonald]

I wanted this for my initial implementation, but I ran into issues and we decided not to include this functionality.

Source

There are inconsistencies between the cache drivers that cause issues with this approach. As an example of the inconsistency, with these changes, this test will never pass for all drivers because the result is not always a stringy number:

public function test_it_is_inconsistent()
{
    foreach (['redis', 'database', 'memcache'] as $name) {
        Cache::store($name)->increment('count');
        $this->assertSame('1', Cache::store($name)->get('count'), $name);
    }
}

Changing '1' to 1 will make some other drivers pass and fail for others.

I feel we would need to address these kinds of inconsistencies if we were to make this happen. I also seem to remember there could be more inconsistencies, not just the increment case.

We'd also need to make sure the write to the cache was successful before we memoized the value, e.g.,

public function put($key, $value, $seconds)
{
    return tap($this->repository->put($key, $value, $seconds), function ($result) use ($key, $value) {
        if ($result) {
            $this->cache[$this->prefix($key)] = $value;
        }
    });
}

Lastly, I would also expect to add the same treatment to putMany and forever.

Re-opening to put it back on Taylor's radar for further consideration.

[crynobone]

@timacdonald do you think this should go to master or 12.x?

[timacdonald]

If the inconsistencies are addressed, it should not be a breaking change, so 12.x is fine.

If the inconsistencies are not addressed, I think we should either not move forward or target master, as it is a breaking change, but I would feel most comfortable if all my concerns were addressed before we considered the feature.

All for the feature in principle.

[KIKOmanasijev]

Thanks for the feedback @timacdonald

1. The check whether the cache write operation was successful, before saving to the memo store, is implemented.
2. Added the same treatment for putMany and forever.

Regarding this part:

public function test_it_is_inconsistent()
{
    foreach (['redis', 'database', 'memcache'] as $name) {
        Cache::store($name)->increment('count');
        $this->assertSame('1', Cache::store($name)->get('count'), $name);
    }
}

I might be missing out something, but IMO this issue isn't introduced by my changes, since it already exists in the framework. The type inconsistency with increment() (and other similar ones) would occur even if we are not using cache memoization. Therefore, we just need to make sure the memoized value types are always matching the types that the underlying cache store would return.

To test whether memoization behaves consistently with the underlying store, a test like this might be more relevant:

public function test_it_is_inconsistent()
{
    foreach (['redis', 'database', 'memcache'] as $name) {
        Cache::store($name)->increment('count');
     
        $memo = Cache::memo($name)->get('count');
        $live = Cache::store($name)->get('count');

        $this->assertSame($memo, $live, $name);
    }
}

This ensures that the memoized value and the value from the underlying cache store are always matching.
The existing data type inconsistencies between cache drivers (for example: Redis returning strings and the database driver returning integers) already exist in the framework and aren’t introduced by this PR. A test like this would simply help ensure that memo() consistently mirrors the return value of the underlying cache store, regardless of the driver.

Let me know what you think :)

[KIKOmanasijev]

@timacdonald I've pushed the test I've mentioned in my previous comment. I think it would be enough to ensure that the memoized value is always the same type as the type of the value fetched from the underlying cache store.

Also a similar test, without using the memo functionality, would fail because not all cache stores return stringy values. This proves that the changes introduced in this PR do not affect this already existing issue.

// ❌ fails
public function test_prove_cache_store_type_inconsistencies()
    {
        foreach (['redis', 'database', 'memcached'] as $name) {
            Cache::store($name)->put('count', 0);
            Cache::store($name)->increment('count');

            $live = Cache::store($name)->get('count');

            $this->assertSame("1", $live, "Failed for cache store: $name");
        }
    }

[timacdonald]

@KIKOmanasijev, I agree my initial test was bad. Sorry about that. It has been a while since I did this so I had forgotten what the exact issue was.

Here is a better example where the memo value differs from the underlying cache store:

<?php

use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Route;

Route::get('/', function () {
    Cache::put('count', 0);

    Cache::memo('database')->increment('count');

    dump(
        Cache::store('database')->get('count'),
        Cache::memo('database')->get('count')
    );
});

This was produced using your current changes.

[KIKOmanasijev]

@timacdonald I spent a lot of time into thinking about a potential solution for this problem, but the problem is more complex than it seems.

All default cache stores (except Redis) return numeric values when using get(), and they all returns numeric values when using increment()/decrement(). So the return types for get/increment all matching for all drivers, and the only outlier is the Redis store. For me, it's confusing that "getting" a value would ALWAYS return a string, and incrementing/decrementing a value would ALWAYS return an integer (although this is a default Redis behaviour).

One solution that came to my mind was to check whether the underlaying store is a RedisStore instance, but something felt off because it introduced driver-specific logic and it didn't seem future-proof.

Instead, I propose updating Redis' implemention of the increment() and decrement() methods to cast their return values to strings, thus the return types will always match when getting a value.

I am aware this is a breaking change, but I think the benefits of matching the return types of the increment and decrement operations with the get operation for Redis-based cache stores, is worth the trade-off.

What do you think?

Cache Driver	get Return Type	increment Return Type	decrement Return Type	Matching Types
Database	int	int	int	yes
Redis	string	string (prev. int)	string (prev. int)	yes (prev. no)
Memcached	int	int	int	yes
DynamoDB	int	int	int	yes

[taylorotwell]

Not sure juice is worth the squeeze here.