Not reproducible compression results with LZ4 #1939
Comments
TheLevti
changed the title
|
|
I tested several compression levels with lz4. Let me post the result here, maybe it will help you. So far I was not able to find a level that produces the same value as the phpredis extension. |
So basically this is my test using laravel's redis cache locking mechanism: $client->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_NONE);
$client->setOption(Redis::OPT_COMPRESSION, Redis::COMPRESSION_LZ4);
$client->setOption(Redis::OPT_COMPRESSION_LEVEL, 0);
$lock = $store->lock('foo', 10);
$this->assertTrue($lock->get());
// Do something while holding the lock...
$lock->release();
$this->assertNull($store->lockConnection()->get($store->getPrefix().'foo')); // Make sure that the key is gone after our lock was released.Test result:
Redis monitor result: As you can see there is no lua execution that calls |
|
Level 1: Level 3: Level 12: Interestingly I get an exception when using lvl 20 for example: I saw in the C code that it should overwrite the level to lvl 12 and still execute. I see no error when I use 0 though. |
|
This is the code that tries to mimic the phpredis extension's serialization/compression for the eval call. For serialization its straight forward by just calling the extension's exposed utility method. For compression its a bit cumbersome. But it works with LZF and ZSTD. We had a similar issue with LZF and it was fixed (#1477) so phpredis and lzf both compress in the same way. It would be nice to achieve the same result with the lz4 implementation. /**
* Release the lock.
*
* @return bool
*/
public function release()
{
/* If a serialization mode such as "php" or "igbinary" and/or a
* compression mode such as "lzf" or "zstd" is enabled, the owner
* must be serialized and/or compressed by us, because phpredis does
* not do this for the eval command.
*
* Name must not be modified!
*/
$owner = $this->redis->client()->_serialize($this->owner);
/* Once the phpredis extension exposes a compress function like the
* above `_serialize()` function, we should switch to it to guarantee
* consistency in the way the extension serializes and compresses to
* avoid the need to check each compression option ourselves.
*
* @see https://github.com/phpredis/phpredis/issues/1938
*/
if ($this->compressed()) {
if ($this->lzfCompressed()) {
$owner = \lzf_compress($owner);
} elseif ($this->zstdCompressed()) {
$owner = \zstd_compress(
$owner,
$this->redis->client()->getOption(Redis::OPT_COMPRESSION_LEVEL)
);
} elseif ($this->lz4Compressed()) {
$owner = \lz4_compress(
$owner,
$this->redis->client()->getOption(Redis::OPT_COMPRESSION_LEVEL)
);
} else {
throw new UnexpectedValueException(sprintf(
'Unknown phpredis compression in use (%d). Unable to release lock.',
$this->redis->client()->getOption(Redis::OPT_COMPRESSION)
));
}
}
return (bool) $this->redis->eval(LuaScripts::releaseLock(), 1, $this->name, $owner);
} |
|
Another example I noticed while adding compression/serialization support to laravel phpredis integration:
For some reason there is always some random letter appended when using |
|
Anyone got an idea how to resolve this? This is currently a blocker to add compression support to the laravel framework. At least if someone could give me a hint where to look, I would try myself. |
|
Hi @TheLevti, The binary mismatch in LZ4 data is because I attach a crc8 + length header as a quick sanity check. I honestly can't remember why I wrote it that way, but unfortunately, we can't just change it because it would break backward compatibility. I'm happy to help get this working for you though. One solution would be to add something like |
The last option would be a perfect solution. Or both. For the utility function I even have opened an issue already: #1938 And thank you for the hint with crc. Maybe i can just do 1:1 on php side to achieve the same result. But yes if a helper function can be added, it would solve all our problems ^^ |
This commit splits compression and serialization into two distinct parts and adds some utility functions so the user can compress/uncompress or pack/unpack data explicily. See #1939
|
It's still a WIP but you can check out the changes pushed above. Not sure what the final API should look like but I've added I'm going to see what it would take to do the
Edit: Turns out there was a reason I used a checksum and a header. It's because there's no way to distinguish between insufficient space in the buffer and malformed data with LZ4 (related LZ4 issue) It may be possible to accomplish unchecked decompression by growing the buffer up to some reasonable multiple of the compressed length when hitting an error. |
Thank you very much, I will try out these the coming days and hope can give some helpful feedback. If we have the pack/unpack helpers or even separated by compress/serialize, it in the end really does not matter how the extension modifies the values and whether it matches the other php extensions. Its just important to be able to reproduce the same results as phpredis to be able to pass correct values to the eval call. Pack/unpack would be nice, because then we would not need to care about the order in which it needs to be modified. But its not that important if you say its too much work. I am already super happy to be able to have a similar compression helper as we currently have with _serialize. |
Ok I finally found the time to fork the repo and build your branch. All my tests for the laravel pull request that were waiting for the convenience functions are passing now. I had troubles with LZ4 and ZSTD and with the _pack function all problems are gone and tests pass now. Code with manual packing logic + not working LZ4:
Code after your addition:
Finally uncommented tests:
Test executions without LZ4 and with this big logic block to properly manually pack (last execution is with LZ4 and a single line call):
Also the redlock algorithm is now working with lz4 enabled. |
|
Can we create a pull request out of your branch? For me the utility functions provided there solve all problems. |
|
I'll create a PR this weekend, @yatsukhnenko can find all my bugs, and then we'll get it merged. |
This commit splits compression and serialization into two distinct parts and adds some utility functions so the user can compress/uncompress or pack/unpack data explicily. See #1939
This commit splits compression and serialization into two distinct parts and adds some utility functions so the user can compress/uncompress or pack/unpack data explicily. See #1939
|
Resolved |
I just tested the LZ4 and ZSTD compression options against the REDLOCK algorithm with the laravel cache lock library and it seems like I have the same problem as in #1477. ZSTD worked out of the box with different compression levels.
For LZ4 I am not able to get the same result by calling
lz4_compress($string, $compressionLevel)with any compression level. Most of the time the difference is the first byte, which seems to be off.With compression level 0 (default lz4), I get the following result:
As you can see the first SET and the EVAL ARGV[1] should be the same, they are but just the first view bytes are different. For the redlock algorithm the user needs to serialize/compress the arguments himself for the EVAL to work, because phpredis is not doing that for the user. For LZF, ZSTD it works fine. As a result the key is never deleted and thus the lock never released until it times out.
Expected behaviour
and
should have the same value.
Actual behaviour
phpredis compresses the string differently than the php extension lz4 by calling
lz4_compress. (https://github.com/kjdev/php-ext-lz4).I'm seeing this behaviour on
Steps to reproduce, backtrace or example script
This is the EVAL script that is running to remove the locked key. See #1477 for examples.
The text was updated successfully, but these errors were encountered: