fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Redis bug with transactions #785

Closed Djuki closed 12 years ago

Djuki commented 12 years ago

When using transactions in FuelPHP it takes too long to respond. I found where is the problem.

        $redis = \Redis::instance();
        $redis->multi();
        $redis->sadd('list', 4);
        $redis->sadd('list', 1);
        $redis->sadd('list', 55);
        $redis->exec();

Solution : In class Fuel\Core\Redis method __call line 137 is

if ($size == '-1')

i change it to this to fix problem.

if ($size == '-1' or $size == 0)

Problem is that it trying to read from $this->connection when there is nothing to read. It's quite possible that authors main class has the same problem. https://github.com/jdp/redisent/blob/master/redisent.php

There is difference in implementation at the case '$': block with size checking (compare this block in authors main class and FuelPHP Redis class).

komita1981 commented 12 years ago

In both case block there is code

$response .= fread($this->connection, $block_size);

If $block_size is empty then we will get warning. To prevent this we can add

if (! empty($block_size))
{
    $block .= fread($this->connection, $block_size);
}
komita1981 commented 12 years ago

Still not working with transactions given in example.

WanWizard commented 12 years ago

@Djuki @komita1981 As far as I can see the implemented fix does what was originally suggested. Are you saying this wasn't the problem, and the transaction issue is caused elsewhere?

komita1981 commented 12 years ago

Yes. The problem is in binary safe file read after Redis server response to transaction request.

Redis class in Fuel is based on Redisent and original class has problem with transactions. We tested original class, class from Laravel (also based on Redisent) and there are always the same problem with binary safe file read. We are not quite familiar with binary safe file reading and we couldn't find fix.

We also tried predis as third party for Fuel and everything worked fine.

WanWizard commented 12 years ago

Ok, I'll leave this open as a bug.

komita1981 commented 12 years ago

Original Redisent class https://github.com/jdp/redisent/blob/master/src/redisent/Redis.php does not have this bug anymore. I tried this new class with FuelPHP (modifying core Redis class) and everything worked fine. Original Redisent class has some new methods to work with transactions and example that @Djuki gave now should be

$redis = \Redis::instance();
$pipe = $redis->pipeline();
$pipe->sadd('list', 4);
$pipe->sadd('list', 1);
$pipe->sadd('list', 55);        
$result = $pipe->uncork();
WanWizard commented 12 years ago

I'll have a look.