Add a request timeout to Excon for non-blocking sockets (#835)

* Add a /sloth path that takes 5 milliseconds to return

* Allow defining a timeout parameter and default to 5 minutes

* Set a deadline in the datum at the start of the request

* Add tests for the request timeout

* Only timeout nonblocking connections, same as read and write timeouts

* Default to no timeout.

* Only set the deadline when a timeout is set.

* Add tests for no timeout set.

* Handle explicit nil timeout, such as in the defaults.

* Don't check the deadline unless set in socket data

* match error message to ensure the right type of timeout occurred

* Use single quotes for timeout message.

* Fix indentation

* Fix indentation

* Remove duplicate /sloth path and add read timeout tests together with timeout.

* Use request symbol instead of string.

* Make expectations specific to the type of timeout

* Update readme with timeout option.

* Switch to a hash for mapping timeout keys to reduce the impact to socket use without a deadline.
Also, remove extra raise when checking the deadline.

* Ruby the use of multi-return in #check_deadline!

* Remove unused params and update docs for check_deadline!

* Add a millisecond example and mention that timeout may be an int or float.

* Switch / to /no-timeout in the timeout.ru server definition

* Rename check deadline and update the doc comment.

* Add comments to explain the request timeout in select_with_timeout

* Put more of a distance between read and request timeout to reduce the likelihood of a race condition

* Need to keep the blocking read-timeout small so the test is fast.

README.md

@@ -179,6 +179,10 @@ connection.request(:read_timeout => 360)
 # set longer write_timeout (default is 60 seconds)
 connection.request(:write_timeout => 360)
 
+# set a request timeout in seconds (default is no timeout).
+# the timeout may be an integer or a float to support sub-second granularity (e.g., 1, 60, 0.005 and 3.5).
+connection.request(:timeout => 0.1) # timeout if the entire request takes longer than 100 milliseconds
+
 # Enable the socket option TCP_NODELAY on the underlying socket.
 #
 # This can improve response time when sending frequent short

lib/excon/connection.rb

@@ -230,6 +230,13 @@ def response_call(datum)
     def request(params={}, &block)
       # @data has defaults, merge in new params to override
       datum = @data.merge(params)
+
+      # Set the deadline for the current request in order to determine when we have run out of time.
+      # Only set when a request timeout has been defined.
+      if datum[:timeout]
+        datum[:deadline] = Process.clock_gettime(Process::CLOCK_MONOTONIC) + datum[:timeout]
+      end
+
       datum[:headers] = @data[:headers].merge(datum[:headers] || {})
 
       validate_params(:request, params, datum[:middlewares])

lib/excon/constants.rb

@@ -62,6 +62,7 @@ module Excon
     :resolv_resolver,
     :response_block,
     :stubs,
+    :timeout,
     :user,
     :versions,
     :write_timeout
@@ -171,6 +172,7 @@ module WaitWritable; end
     :stubs               => :global,
     :tcp_nodelay         => false,
     :thread_safe_sockets => true,
+    :timeout             => nil,
     :uri_parser          => URI,
     :versions            => VERSIONS,
     :write_timeout       => 60

lib/excon/socket.rb

@@ -25,6 +25,13 @@ class Socket
     else # Ruby <= 2.0
       [Errno::EAGAIN, Errno::EWOULDBLOCK, IO::WaitWritable]
     end
+    # Maps a socket operation to a timeout property.
+    OPERATION_TO_TIMEOUT = {
+      :connect_read => :connect_timeout,
+      :connect_write => :connect_timeout,
+      :read => :read_timeout,
+      :write => :write_timeout
+    }.freeze
 
     def params
       Excon.display_warning('Excon::Socket#params is deprecated use Excon::Socket#data instead.')
@@ -304,17 +311,33 @@ def write_block(data)
     end
 
     def select_with_timeout(socket, type)
+      timeout_kind = type
+      timeout = @data[OPERATION_TO_TIMEOUT[type]]
+
+      # Check whether the request has a timeout configured.
+      if @data.include?(:deadline)
+        request_timeout = request_time_remaining
+
+        # If the time remaining until the request times out is less than the timeout for the type of select,
+        # use the time remaining as the timeout instead.
+        if request_timeout < timeout
+          timeout_kind = :request
+          timeout = request_timeout
+        end
+      end
+
       select = case type
       when :connect_read
-        IO.select([socket], nil, nil, @data[:connect_timeout])
+        IO.select([socket], nil, nil, timeout)
       when :connect_write
-        IO.select(nil, [socket], nil, @data[:connect_timeout])
+        IO.select(nil, [socket], nil, timeout)
       when :read
-        IO.select([socket], nil, nil, @data[:read_timeout])
+        IO.select([socket], nil, nil, timeout)
       when :write
-        IO.select(nil, [socket], nil, @data[:write_timeout])
+        IO.select(nil, [socket], nil, timeout)
       end
-      select || raise(Excon::Errors::Timeout.new("#{type} timeout reached"))
+
+      select || raise(Excon::Errors::Timeout.new("#{timeout_kind} timeout reached"))
     end
 
     def unpacked_sockaddr
@@ -324,5 +347,16 @@ def unpacked_sockaddr
         raise
       end
     end
+
+    # Returns the remaining time in seconds until we reach the deadline for the request timeout.
+    # Raises an exception if we have exceeded the request timeout's deadline.
+    def request_time_remaining
+      now = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+      deadline = @data[:deadline]
+
+      raise(Excon::Errors::Timeout.new('request timeout reached')) if now >= deadline
+
+      deadline - now
+    end
   end
 end

spec/requests/timeout_spec.rb

@@ -0,0 +1,83 @@
+require 'spec_helper'
+
+describe Excon::Connection do
+  include_context('test server', :webrick, 'timeout.ru', before: :start, after: :stop)
+
+  let(:read_timeout) { 60 }
+  let(:conn) do
+    Excon::Connection.new(host: '127.0.0.1',
+                          hostname: '127.0.0.1',
+                          nonblock: nonblock,
+                          port: 9292,
+                          scheme: 'http',
+                          timeout: timeout,
+                          read_timeout: read_timeout)
+  end
+
+  context "blocking connection" do
+    let (:nonblock) { false }
+
+    context 'when timeout is not set' do
+      let(:timeout) { nil }
+
+      it 'does not error' do
+        expect(conn.request(:path => '/no-timeout').status).to eq(200)
+      end
+    end
+
+    context 'when timeout is not triggered' do
+      let(:timeout) { 1 }
+
+      it 'does not error' do
+        expect(conn.request(:path => '/no-timeout').status).to eq(200)        
+      end
+    end
+
+    context 'when timeout is triggered' do
+      let(:read_timeout) { 0.005 }
+      let(:timeout) { 0.001 }
+
+      it 'does not raise' do
+        # raising a read timeout to keep tests fast
+        expect { conn.request(:path => '/timeout') }.to raise_error(Excon::Error::Timeout, 'read timeout reached')
+      end
+    end
+  end
+
+  context "non-blocking connection" do
+    let (:nonblock) { true }
+
+    context 'when timeout is not set' do
+      let(:timeout) { nil }
+
+      it 'does not error' do
+        expect(conn.request(:path => '/no-timeout').status).to eq(200)        
+      end
+    end
+
+    context 'when timeout is not triggered' do
+      let(:timeout) { 1 }
+
+      it 'does not error' do
+        expect(conn.request(:path => '/no-timeout').status).to eq(200)        
+      end
+    end
+
+    context 'when timeout is triggered' do
+      let(:timeout) { 0.001 }
+
+      it 'returns a request Excon::Error::Timeout' do
+        expect { conn.request(:path => '/timeout') }.to raise_error(Excon::Error::Timeout, 'request timeout reached')
+      end
+    end
+
+    context 'when read timeout is triggered' do
+      let(:read_timeout) { 0.001 }
+      let(:timeout) { 5 }
+
+      it 'returns a read Excon::Error::Timeout' do
+        expect { conn.request(:path => '/timeout') }.to raise_error(Excon::Error::Timeout, 'read timeout reached')
+      end
+    end
+  end
+end

tests/rackups/timeout.ru

@@ -4,6 +4,10 @@ class App < Sinatra::Base
   set :environment, :production
   enable :dump_errors
 
+  get('/no-timeout') do
+    ''
+  end
+
   get('/timeout') do
     sleep(2)
     ''