Merge pull request #195 from zanker/timeout-timeout-dns

Use Timeout.timeout for TCP connection

lib/http/timeout/global.rb

@@ -1,4 +1,3 @@
-# rubocop:disable Lint/HandleExceptions
 module HTTP
   module Timeout
     class Global < PerOperation
@@ -11,24 +10,24 @@ def initialize(*args)
         @total_timeout = time_left
       end
 
-      # Abstracted out from the normal connect for SSL connections
-      def connect_with_timeout(*args)
+      def connect(socket_class, host, port)
         reset_timer
+        ::Timeout.timeout(time_left, TimeoutError) do
+          @socket = socket_class.open(host, port)
+        end
 
-        begin
-          socket.connect_nonblock(*args)
+        log_time
+      end
+
+      def connect_ssl
+        reset_timer
 
+        begin
+          socket.connect_nonblock
         rescue IO::WaitReadable
           IO.select([socket], nil, nil, time_left)
           log_time
           retry
-
-        rescue Errno::EINPROGRESS
-          IO.select(nil, [socket], nil, time_left)
-          log_time
-          retry
-
-        rescue Errno::EISCONN
         end
       end
 
@@ -58,26 +57,9 @@ def write(data)
         end
       end
 
-      private
-
-      # Create a DNS resolver
-      def resolve_address(host)
-        addr = HostResolver.getaddress(host)
-        return addr if addr
-
-        reset_timer
-
-        addr = Resolv::DNS.open(:timeout => time_left) do |dns|
-          dns.getaddress
-        end
+      alias_method :<<, :write
 
-        log_time
-
-        addr
-
-      rescue Resolv::ResolvTimeout
-        raise TimeoutError, "DNS timed out after #{total_timeout} seconds"
-      end
+      private
 
       # Due to the run/retry nature of nonblocking I/O, it's easier to keep track of time
       # via method calls instead of a block to monitor.
@@ -96,4 +78,3 @@ def log_time
     end
   end
 end
-# rubocop:enable Lint/HandleExceptions

lib/http/timeout/per_operation.rb

@@ -1,6 +1,3 @@
-# rubocop:disable Lint/HandleExceptions
-require "resolv"
-
 module HTTP
   module Timeout
     class PerOperation < Null
@@ -20,40 +17,22 @@ def initialize(*args)
         @connect_timeout = options.fetch(:connect_timeout, CONNECT_TIMEOUT)
       end
 
-      def connect(_, host, port)
-        # https://github.com/celluloid/celluloid-io/blob/master/lib/celluloid/io/tcp_socket.rb
-        begin
-          addr = Resolv::IPv4.create(host)
-        rescue ArgumentError
-        end
-
-        # Guess it's not IPv4! Is it IPv6?
-        begin
-          addr ||= Resolv::IPv6.create(host)
-        rescue ArgumentError
-        end
-
-        unless addr
-          addr = resolve_address(host)
-          fail Resolv::ResolvError, "DNS result has no information for #{host}" unless addr
+      def connect(socket_class, host, port)
+        ::Timeout.timeout(connect_timeout, TimeoutError) do
+          @socket = socket_class.open(host, port)
         end
+      end
 
-        case addr
-        when Resolv::IPv4
-          family = Socket::AF_INET
-        when Resolv::IPv6
-          family = Socket::AF_INET6
-        else fail ArgumentError, "unsupported address class: #{addr.class}"
+      def connect_ssl
+        socket.connect_nonblock
+      rescue IO::WaitReadable
+        if IO.select([socket], nil, nil, connect_timeout)
+          retry
+        else
+          raise TimeoutError, "Connection timed out after #{connect_timeout} seconds"
         end
-
-        @socket = Socket.new(family, Socket::SOCK_STREAM, 0)
-
-        connect_with_timeout(Socket.sockaddr_in(port, addr.to_s))
       end
 
-      # No changes need to be made for the SSL connection
-      alias_method :connect_with_timeout, :connect_ssl
-
       # Read data from the socket
       def readpartial(size)
         socket.read_nonblock(size)
@@ -75,43 +54,6 @@ def write(data)
           raise TimeoutError, "Read timed out after #{write_timeout} seconds"
         end
       end
-
-      private
-
-      # Actually do the connect after we're setup
-      def connect_with_timeout(*args)
-        socket.connect_nonblock(*args)
-
-      rescue IO::WaitReadable
-        if IO.select([socket], nil, nil, connect_timeout)
-          retry
-        else
-          raise TimeoutError, "Connection timed out after #{connect_timeout} seconds"
-        end
-
-      rescue Errno::EINPROGRESS
-        if IO.select(nil, [socket], nil, connect_timeout)
-          retry
-        else
-          raise TimeoutError, "Connection timed out after #{connect_timeout} seconds"
-        end
-
-      rescue Errno::EISCONN
-      end
-
-      # Create a DNS resolver
-      def resolve_address(host)
-        addr = HostResolver.getaddress(host)
-        return addr if addr
-
-        Resolv::DNS.open(:timeout => connect_timeout) do |dns|
-          dns.getaddress
-        end
-
-      rescue Resolv::ResolvTimeout
-        raise TimeoutError, "DNS timed out after #{connect_timeout} seconds"
-      end
     end
   end
 end
-# rubocop:enable Lint/HandleExceptions

spec/lib/http/client_spec.rb

@@ -181,7 +181,7 @@ def simple_response(body, status = 200)
       described_class.new options.merge :ssl_context => SSLHelper.client_context
     end
 
-    include_context "HTTP handling", true do
+    include_context "HTTP handling" do
       let(:server) { dummy_ssl }
     end
 

spec/support/http_handling_shared.rb

@@ -1,4 +1,4 @@
-RSpec.shared_context "HTTP handling" do |ssl = false|
+RSpec.shared_context "HTTP handling" do
   describe "timeouts" do
     let(:conn_timeout) { 1 }
     let(:read_timeout) { 1 }
@@ -73,56 +73,18 @@
 
       let(:response) { client.get(server.endpoint).body.to_s }
 
-      context "with localhost" do
-        let(:endpoint) { server.endpoint.sub("127.0.0.1", "localhost") }
-
-        it "errors if DNS takes too long" do
-          # Block the localhost lookup
-          expect(timeout_class::HostResolver)
-            .to receive(:getaddress).with("localhost").and_return(nil)
-
-          # Request
-          expect(Resolv::DNS).to receive(:open).with(:timeout => 1) do |_|
-            sleep 1.25
-            "127.0.0.1"
-          end
-
-          expect { client.get(server.endpoint.sub("127.0.0.1", "localhost")) }
-            .to raise_error(HTTP::TimeoutError, /Timed out/)
-        end
-      end
-
       it "errors if connecting takes too long" do
-        socket = Socket.new(Socket::AF_INET, Socket::SOCK_STREAM, 0)
-
-        fake_socket_id = double(:to_io => socket)
-        expect(fake_socket_id).to receive(:connect_nonblock) do |*args|
+        expect(TCPSocket).to receive(:open) do
           sleep 1.25
-          socket.connect_nonblock(*args)
         end
 
-        allow_any_instance_of(timeout_class).to receive(:socket).and_return(fake_socket_id)
-
-        expect { response }.to raise_error(HTTP::TimeoutError, /Timed out/)
+        expect { response }.to raise_error(HTTP::TimeoutError, /execution/)
       end
 
       it "errors if reading takes too long" do
         expect { client.get("#{server.endpoint}/sleep").body.to_s }
           .to raise_error(HTTP::TimeoutError, /Timed out/)
       end
-
-      unless ssl
-        it "errors if writing takes too long" do
-          socket = Socket.new(Socket::AF_INET, Socket::SOCK_STREAM, 0)
-          allow_any_instance_of(timeout_class).to receive(:socket).and_return(socket)
-
-          expect(socket).to receive(:<<) do |*|
-            sleep 1.25
-          end
-
-          expect { response }.to raise_error(HTTP::TimeoutError, /Timed out/)
-        end
-      end
     end
   end