shogo82148 / Redis-Fast

fast perl binding for Redis database
https://metacpan.org/release/Redis-Fast
Other
25 stars 21 forks source link

BLPOP can timeout after successfully receive data #13

Closed vsespb closed 10 years ago

vsespb commented 10 years ago

Redis::Fast 0.09

use strict;
use warnings;
use Redis::Fast;
my $redis = Redis::Fast->new(server => 'localhost:6379', reconnect=>1, cnx_timeout   => 0.2, read_timeout  => 1);
if (fork()) {
  sleep 2;
  $redis->rpush("somekey", 4);
  sleep 1;
  my $data = $redis->lpop("somekey");
  print "DATA $data\n";
}
else {
  $redis->blpop("somekey", 3);
  exit;
}
wait;
Use of uninitialized value $data in concatenation (.) or string at 1.pl line 10.
DATA 
Error while reading from Redis server: Connection timed out at /usr/local/lib/perl/5.14.2/Redis/Fast.pm line 178.

This means DATA is undef and that can happen only if child process already received data with BLPOP.

Not reproducible with Redis module:

use strict;
use warnings;
use Redis;
my $redis = Redis->new(server => 'localhost:6379', reconnect=>1, cnx_timeout   => 0.2, read_timeout  => 1);
if (fork()) {
  sleep 2;
  $redis->rpush("somekey", 4);
  sleep 1;
  my $data = $redis->lpop("somekey");
  print "DATA $data\n";
}
else {
  $redis->blpop("somekey", 3);
  exit;
}
wait;
Error while reading from Redis server: Resource temporarily unavailable at /usr/local/share/perl/5.14.2/Redis.pm line 267.
DATA 4

p.s. also note different error message. I think users will have to parse error message cause if they wish to continue to work with redis after timeout happened, so would be good to have same error message for Redis and Redis::Fast

vsespb commented 10 years ago

UPD: apparently I see loss of BLPOP message even without read_timeout and even with BLPOP timeout 0. (I use BLPOP with multiple keys). Cannot create testcase for this. But issue gone after rolling back to 0.05

vsespb commented 10 years ago

apparently I see loss of BLPOP message even without read_timeout and even with BLPOP timeout 0.

here is PoC:

use strict;
use warnings;
use Redis::Fast;
my $redis = Redis::Fast->new(server => 'localhost:6379', reconnect=>1, write_timeout  => 1);
if (fork()) {
  sleep 2;
  $redis->rpush("somekey", 4);
  sleep 1;
  my $data = $redis->lpop("somekey");
  print "DATA $data\n";
}
else {
  print $redis->blpop("somekey", 0);
  exit;
}
wait;
Use of uninitialized value $data in concatenation (.) or string at 3.pl line 10.
DATA 
Error while reading from Redis server: Connection timed out at /usr/local/lib/perl/5.14.2/Redis/Fast.pm line 178.

problem with write_timeout - seems it affects BLPOP just like read timeout, and this looks like a bug, cause Redis behaves different way:

use strict;
use warnings;
use Redis;
my $redis = Redis->new(server => 'localhost:6379', reconnect=>1, write_timeout  => 1);
print $redis->blpop("notakey", 5);
__END__
real    0m5.534s
user    0m0.044s
sys 0m0.004s
use strict;
use warnings;
use Redis::Fast;
my $redis = Redis::Fast->new(server => 'localhost:6379', reconnect=>1, write_timeout  => 1);
print $redis->blpop("notakey", 5);
__END__
real    0m2.032s
user    0m0.020s
sys 0m0.008s

Redis::Fast time is 2s, not 5s, and not even 1s (and it should be 5s)

shogo82148 commented 10 years ago

15 will fix it. try it please.

vsespb commented 10 years ago

Issue with wrong timeout fixed, but it does not fix main issue - data loss when using BLPOP!

shogo82148 commented 10 years ago

:( i will try another method.

shogo82148 commented 10 years ago

i have updated #15. try it please.

vsespb commented 10 years ago

Hello. Looks good. I did not find bugs. However we can do real, production testing, only after CPAN release. Also, problem with test - it uses real Redis. I would like to help and try fix SpawnRedisTimeoutServer/SpawnRedisServer to implement real BLPOP for this test. Ok?

shogo82148 commented 10 years ago

yes, of course.