Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-899

Ruby read timeouts can sometimes be 2x what they should be

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.1, 0.2, 0.3, 0.4
    • 0.5
    • Ruby - Library
    • None
    • Patch Available

    Description

      Thrift::Socket#read will sometimes fail to enforce a timeout properly and wait approximately 2x @timeout before timing out the request.

      Our testing via thrift_client (http://github.com/fauna/thrift_client) would only fail about 1/20 of the time. My hypothesis is that it only fails when you have a floating-point rounding error (we set the timeout to 0.8 seconds, but the math gave us 0.7999, so we wait another 0.8 seconds).

      The patch doesn't have any tests, because this problem is non-deterministic. (also, I couldn't figure out how to run the tests)

      Attachments

        1. THRIFT-899.patch
          2 kB
          Ryan King

        Activity

          kingryan Ryan King added a comment - - edited

          I can't get jira to upload a the patch as an attachment, so here it is pasted:

          diff --git a/lib/rb/lib/thrift/transport/socket.rb b/lib/rb/lib/thrift/transport/socket.rb
          index 06c937e..dd76036 100644
          --- a/lib/rb/lib/thrift/transport/socket.rb
          +++ b/lib/rb/lib/thrift/transport/socket.rb
          @@ -97,12 +97,12 @@ module Thrift
                     data = @handle.readpartial(sz)
                   else
                     # it's possible to interrupt select for something other than the timeout
          -          # so we need to ensure we've waited long enough
          +          # so we need to ensure we've waited long enough, but not too long
                     start = Time.now
          -          rd = nil # scoping
          -          loop do
          -            rd, = IO.select([@handle], nil, nil, @timeout)
          -            break if (rd and not rd.empty?) or Time.now - start >= @timeout
          +          deadline = start + @timeout
          +          rd = loop do
          +            rd, = IO.select([@handle], nil, nil, deadline - Time.now)
          +            break rd if (rd and not rd.empty?) or Time.now >= deadline
                     end
                     if rd.nil? or rd.empty?
                       raise TransportException.new(TransportException::TIMED_OUT, "Socket: Timed out reading #{sz} bytes from #{@desc}")
          
          kingryan Ryan King added a comment - - edited I can't get jira to upload a the patch as an attachment, so here it is pasted: diff --git a/lib/rb/lib/thrift/transport/socket.rb b/lib/rb/lib/thrift/transport/socket.rb index 06c937e..dd76036 100644 --- a/lib/rb/lib/thrift/transport/socket.rb +++ b/lib/rb/lib/thrift/transport/socket.rb @@ -97,12 +97,12 @@ module Thrift data = @handle.readpartial(sz) else # it's possible to interrupt select for something other than the timeout - # so we need to ensure we've waited long enough + # so we need to ensure we've waited long enough, but not too long start = Time.now - rd = nil # scoping - loop do - rd, = IO.select([@handle], nil, nil, @timeout) - break if (rd and not rd.empty?) or Time.now - start >= @timeout + deadline = start + @timeout + rd = loop do + rd, = IO.select([@handle], nil, nil, deadline - Time.now) + break rd if (rd and not rd.empty?) or Time.now >= deadline end if rd.nil? or rd.empty? raise TransportException.new(TransportException::TIMED_OUT, "Socket: Timed out reading #{sz} bytes from #{@desc}")
          kingryan Ryan King added a comment -

          Also, we may want to consider doing this for #open and #write.

          kingryan Ryan King added a comment - Also, we may want to consider doing this for #open and #write.
          kingryan Ryan King added a comment -

          updated diff with tests passing

          kingryan Ryan King added a comment - updated diff with tests passing
          bryanduxbury Bryan Duxbury added a comment -

          I just committed this. Thanks for the patch!

          bryanduxbury Bryan Duxbury added a comment - I just committed this. Thanks for the patch!

          People

            kingryan Ryan King
            kingryan Ryan King
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: