add Redis::OPT_READ_TIMEOUT option for issue #70

[kotas]

This is a patch to fix issue #70 debug read error on connection.

Redis#connect() uses its third argument timeout not only for the connection timeout but also for the read timeout on an opened stream. (by calling php_stream_set_option on the stream)

This behavior causes "read error" on any reading methods (get, hget, blpop, etc.) if you give very short timeout value to connect().

This patch adds Redis::OPT_READ_TIMEOUT for Redis#setOption() so that you can specify another timeout value for php_stream_set_option.

You can test this fix by @micharl-grunder's reproduce code.

Before

try {
        $r = new Redis();
        if(!$r->connect('localhost',6379,5)) {
                echo "Couldn't connect\n";
                die();
        }
        $st = microtime(true);
        $val = $r->blpop('mykey',0);
        $et = microtime(true);
} catch(Exception $ex) {
        echo "ex: " . $ex->getMessage() . "\n";
        $et = microtime(true);
}

echo "Took: " . ($et-$st) . " to get the key\n";

output:

ex: read error on connection
Took: 5.0015189647675 to get the key

After

try {
        $r = new Redis();
        if(!$r->connect('localhost',6379,5)) {
                echo "Couldn't connect\n";
                die();
        }
        $r->setOption(Redis::OPT_READ_TIMEOUT, 10);   // by patch
        $st = microtime(true);
        $val = $r->blpop('mykey',0);
        $et = microtime(true);
} catch(Exception $ex) {
        echo "ex: " . $ex->getMessage() . "\n";
        $et = microtime(true);
}

echo "Took: " . ($et-$st) . " to get the key\n";

output:

ex: read error on connection
Took: 10.001543998718 to get the key

[kotas]

Note: You cannot setOption(Redis::OPT_READ_TIMEOUT, 0) to NOT time out. php_stream_set_option doesn't accept 0 for that purpose.

[colinmollenhour]

Any word on this? It seems like a good feature to have while having no drawbacks..

[xiaoyjy]

why not just seconds? microseconds should be checked.

[twogood]

+1 on this pull request!

[njam]

+1 please
So there is no way of setting no timeout, should we just set a very high number?

[michael-grunder]

You should be able to set no timeout (overall). You also have to check the value of default_socket_timeout in php.ini. I believe -1 is what you want for unlimited.

However, I think this is a reasonable feature to add (separate read timeout), so I'll get it in later tonight.

[njam]

Ok I see, but my understanding is setting default_socket_timeout to -1 can have unwanted side effects, since this is a global PHP option? Will this also apply to the connection timeout?
I would like to have a reasonable connection timeout of a few seconds, but subscribe should never timeout.
Maybe some hint on the docu for "pub/sub" would be good when this is merged, since it seems many people stumble upon this (#230, #70, #4).

[michael-grunder]

Yes, I see your issue. I will get this merged tonight for you guys. :)

Cheers!
Mike

[njam]

Hm I thought I posted this comment before, but can't see it anymore:

Thank you very much for merging this quickly!
Will there be a new tag or should I use master?

[michael-grunder]

You can go ahead and use master. I think it's a safe enough fix. Just let me know if there are any issues and I can fix them.

[njam]

I hate to nag, but is there any chance you could create a tag of the current version?
(I don't like to use master in production since I want to stick to a specific version for better repeatability.)

[michael-grunder]

Sure, seems fine to me.

@nicolasff Any issue with me tagging the current master 2.2.3? I'm planning on tackling some of the session issues this week, so it could be a decent time. There have been many things incorporated since 2.2.2

[nicolasff]

I think it's a good idea, it's been a while a lots of features have made it to trunk since the last release.

[michael-grunder]

Pushed and tagged! :)

Please let us know if anything was broken. There are quite a few features added in this release.

Cheers!
Mike

[njam]

Thanks 😘, will report any problems