redis / redis-rb

A Ruby client library for Redis
MIT License
3.96k stars 1.03k forks source link

fallback to Timeout::timeout in Ruby 1.8 #51

Closed jordan-brough closed 14 years ago

jordan-brough commented 14 years ago

Thanks for the update in http://github.com/ezmobius/redis-rb/issues/issue/50. A related issue: even Ruby 1.8's default timeout can handle some important open hangs (ours, for example). Why not fallback to Timeout::timeout rather than a no-op? I can see still having some sort of warning.

jordan-brough commented 14 years ago

Submitted a patch via pull request for http://github.com/jordan-brough/redis-rb/tree/ruby_1_8_timeout (commit 0972e29b0607d62fc4408dcc3af0f5f677d3887a)

djanowski commented 14 years ago

I'm not sure about this one. It looks like in 1.8 the timeout will never work because we're only using it for a blocking system call and that's exactly the one scenario that is broken. http://davidvollbracht.com/2008/6/2/30-days-of-teach-day-1-systemtimer http://ph7spot.com/musings/system-timer

jordan-brough commented 14 years ago

It depends on which part of the connection open fails and whether the particular system call is a blocking system call. If there's some sort of DNS issue then 1.8 may fail to timeout. On the other hand, if the machine is simply down (as in our case) then 1.8's timer works fine (and TCP waits for a long time before bombing out).

Example on my box and on one of our servers:

>> RUBY_VERSION
=> "1.8.7"
>> `uname -spr`
=> "Darwin 10.4.0 i386\n"
>> require 'socket'
>> require 'benchmark'
>> Benchmark.measure { begin; TCPSocket.new('127.0.0.2', 80); rescue Exception => e; puts e.message; end }.real
Operation timed out - connect(2)
=> 74.5902619361877
>> require 'timeout'
>> Benchmark.measure { begin; Timeout.timeout(3) { TCPSocket.new('127.0.0.2', 80) }; rescue Exception => e; puts e.message; end }.real
execution expired
=> 3.00085616111755

>> RUBY_VERSION
=> "1.8.7"
>> `uname -spr`
=> "Linux 2.6.21.7-2.fc8xen x86_64\n"
>> require 'socket'
>> require 'benchmark'
>> Benchmark.measure { begin; TCPSocket.new('10.73.48.91', 80); rescue Exception => e; puts e.message; end }.real
Connection timed out - connect(2)
=> 188.967981815338
>> require 'timeout'
>> Benchmark.measure { begin; Timeout.timeout(3) { TCPSocket.new('10.73.48.91', 80) }; rescue Exception => e; puts e.message; end }.real
execution expired
=> 3.00121092796326
inspire22 commented 14 years ago

SystemTimer is a good gem to help with this on ruby 1.8

djanowski commented 14 years ago

@jordan-brough: OK, thanks a lot for your valuable feedback. How does this look now? 2cdd8a0b733686dbd8f85a5789b504a57d2a923a What do you think about the warning message?

djanowski commented 14 years ago

@inspire22: Yes, that was included in the original patch. jordan-brough was suggesting that we still use the built-in Timeout class if SystemTimer is not installed.

jordan-brough commented 14 years ago

Perfect, thanks. The warning message looks great to me.

djanowski commented 14 years ago

Cool, thank you again.