add Redis::OPT_READ_TIMEOUT option for issue #70 #260

Merged
merged 1 commit into from Feb 11, 2013

Conversation

Projects
None yet
7 participants
@kotas
Contributor

kotas commented Oct 4, 2012

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

This comment has been minimized.

Show comment Hide comment
@kotas

kotas Oct 4, 2012

Contributor

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

Contributor

kotas commented Oct 4, 2012

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

@kotas kotas referenced this pull request Oct 4, 2012

Closed

debug read error on connection #70

@colinmollenhour

This comment has been minimized.

Show comment Hide comment
@colinmollenhour

colinmollenhour Nov 19, 2012

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

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

@colinmollenhour colinmollenhour referenced this pull request in colinmollenhour/Cm_Cache_Backend_Redis Nov 19, 2012

Closed

read error on connection #18

@@ -915,9 +919,9 @@ PHPAPI int redis_sock_connect(RedisSock *redis_sock TSRMLS_DC)
php_stream_auto_cleanup(redis_sock->stream);
- if(tv.tv_sec != 0) {
+ if(read_tv.tv_sec != 0) {

This comment has been minimized.

Show comment Hide comment
@xiaoyjy

xiaoyjy Jan 14, 2013

why not just seconds? microseconds should be checked.

@xiaoyjy

xiaoyjy Jan 14, 2013

why not just seconds? microseconds should be checked.

@twogood

This comment has been minimized.

Show comment Hide comment
@twogood

twogood Jan 16, 2013

+1 on this pull request!

twogood commented Jan 16, 2013

+1 on this pull request!

@njam

This comment has been minimized.

Show comment Hide comment
@njam

njam Feb 10, 2013

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

njam commented Feb 10, 2013

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

@michael-grunder

This comment has been minimized.

Show comment Hide comment
@michael-grunder

michael-grunder Feb 10, 2013

Owner

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.

Owner

michael-grunder commented Feb 10, 2013

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

This comment has been minimized.

Show comment Hide comment
@njam

njam Feb 10, 2013

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).

njam commented Feb 10, 2013

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

This comment has been minimized.

Show comment Hide comment
@michael-grunder

michael-grunder Feb 10, 2013

Owner

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

Cheers!
Mike

Owner

michael-grunder commented Feb 10, 2013

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

Cheers!
Mike

@michael-grunder michael-grunder merged commit 3764a6c into phpredis:master Feb 11, 2013

@njam

This comment has been minimized.

Show comment Hide comment
@njam

njam Feb 12, 2013

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?

njam commented Feb 12, 2013

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

This comment has been minimized.

Show comment Hide comment
@michael-grunder

michael-grunder Feb 12, 2013

Owner

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.

Owner

michael-grunder commented Feb 12, 2013

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 njam referenced this pull request in cargomedia/cm Mar 29, 2013

Merged

Set redis read timeout for subscribe() #129

@njam

This comment has been minimized.

Show comment Hide comment
@njam

njam Apr 25, 2013

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.)

njam commented Apr 25, 2013

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

This comment has been minimized.

Show comment Hide comment
@michael-grunder

michael-grunder Apr 25, 2013

Owner

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

Owner

michael-grunder commented Apr 25, 2013

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

This comment has been minimized.

Show comment Hide comment
@nicolasff

nicolasff Apr 25, 2013

Contributor

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.

Contributor

nicolasff commented Apr 25, 2013

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

This comment has been minimized.

Show comment Hide comment
@michael-grunder

michael-grunder Apr 28, 2013

Owner

Pushed and tagged! :)

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

Cheers!
Mike

Owner

michael-grunder commented Apr 28, 2013

Pushed and tagged! :)

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

Cheers!
Mike

@njam

This comment has been minimized.

Show comment Hide comment
@njam

njam Apr 29, 2013

Thanks 😘, will report any problems

njam commented Apr 29, 2013

Thanks 😘, will report any problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment