[8.x] phpredis lock serialization and compression support

[TheLevti]

#36337: Respect serialization and compression settings of the phpredis extension when using it for locking via the cache service.

• Support phpredis serialization when locking (none, php, json, igbinary, msgpack).
• Support phpredis compression when locking (none, lzf, zstd, lz4).

This is a bug that will cause locking to not release the lock if the redis connection is configured to use serialization and/or compression of data. LZ4 code support was added, but is not working yet as phpredis implemented it not consistent to the lz4 php extension. Waiting for this issue on phpredis to be resolved: phpredis/phpredis#1939. All other compression options resolve this bug.

Also once the phpredis issue phpredis/phpredis#1938 is resolved, we might have a consistent way to call the same compression logic that the extension is using without the need to check for each implementation.

Tested locally with all extensions enabled/configured (lzf, zstd, lz4, igbinary, msgpack).

Solves #36337

[it-can]

Would also be nice to have laravel also support compression and serializer functions for phpredis… like this #31262

[TheLevti]

Would also be nice to have laravel also support compression and serializer functions for phpredis… like this #31262

I know, but before we can add those options to the laravel redis config, we should properly support the locking mechanism. So this pull request should be a prerequisite for the options to be added.

[derekmd]

This release() method can be kept simple by moving the "phpredis" extension-specific code into another method:

public function release()
{
    return (bool) $this->redis->eval(LuaScripts::releaseLock(), 1, $this->name, $this->phpRedisOwner() ?? $owner);
}

protected function phpRedisOwner()
{
    if (! $this->isPhpRedisCompressionEnabled()) {
        return;
    }

    $owner = $this->redis->client()->_serialize($this->owner);

    // etc.
}

Here's a Gist with an updated RedisLock.php class: https://gist.github.com/derekmd/039d1b7b83326fc69790552fb30c9cc8 Then a 220 line class is shortened to 181 lines.

Another option is to move these additions into a completely new class with a constructor that accepts dependencies $this->owner and $this->redis->client().

[TheLevti]

I have moved this whole code into a different class.

[TheLevti]

Also I added a new test class for this new one. Seems to be a lot cleaner now as this code is very specific to the extension and should not be mixed with the general redis code.

[derekmd]

Can the calls to constant() be avoided since you don't need to worry about classes \Redis and \RedisCluster not existing? Updating isPhpRedis() to use instanceof:

use Redis;
use RedisCluster;

// ...

protected function isPhpRedis()
{
    $client = $this->redis->client();

    return $client instanceof Redis || $client instanceof RedisCluster;
}

This short-circuits the other is*() methods from being called. The instanceof keyword can accept any symbol, even for classes that don't exist. e.g., $client instanceof ThisClassDoesNotExist won't throw an exception - it just returns false.

Then the other predicate methods can be simplified to a single expression. e.g.,

protected function isPhpRedisLz4Enabled()
{
    return defined('Redis::COMPRESSION_LZ4') &&
        $this->redis->client()->getOption(Redis::OPT_COMPRESSION) === Redis::COMPRESSION_LZ4;
}

[TheLevti]

Sure, I was just not sure how static analyzers will react if classes/constants do not exist as they are added bases on compile config on phpredis

[TheLevti]

For the tests that need the specific extensions I am waiting for shivammathur/setup-php#419.

Currently in the pipeline only the serialization (php/json/none) are run. The igbinary and msgpack more rare use case of also enabled compression works fine locally (see screenshot).

Once it is possible, I will submit another pull request to install those extension in the pipeline so those are also run. Currently they are skipped.

[taylorotwell]

@TheLevti is this ready for review now?

[TheLevti]

@TheLevti is this ready for review now?

Yes, LZ4 support is added and should work once the phpredis extension fixes/adjusts their code. All other compression/serialization methods are working.

[taylorotwell]

@TheLevti - So adding this call will not break any existing locks that were created before this change? I could imagine a scenario where locks exist - application is deployed with this changed - will those locks still be able to be regathered and released?

[TheLevti]

Yes, existing locks will work on deployment (if the lock owner is restored of course). If phpredis has serialization disabled, this will return the same value as passed down (See: https://github.com/phpredis/phpredis/blob/develop/library.c#L3029). I wrote a test for this case, which passes. (See: https://github.com/laravel/framework/pull/36412/files#diff-07d7fee8b1e505b0e2816e7b5dd26b90abc81614820689b5a070912f495c8279R31)

[TheLevti]

Nothing changes in regards of the redlock algorithm. The way the lock is created has not changed here. The same eval call is done with the same lua script executed on release as before. This change is all about modifying the passed arguments to the eval call when releasing the lock.

[it-can]

Would also be nice to have laravel also support compression and serializer functions for phpredis… like this #31262

I know, but before we can add those options to the laravel redis config, we should properly support the locking mechanism. So this pull request should be a prerequisite for the options to be added.

Cool this pr is merged!