From bf112a2cbefacc86f315d5ea98be7bd201d421e6 Mon Sep 17 00:00:00 2001 From: Zach Anker Date: Mon, 30 Mar 2015 09:23:16 -0700 Subject: [PATCH] Use Timeout.timeout for TCP connection due to impossibility of handling timeouts in pure Ruby --- lib/http/timeout/global.rb | 45 +++++----------- lib/http/timeout/per_operation.rb | 80 ++++------------------------ spec/lib/http/client_spec.rb | 2 +- spec/support/http_handling_shared.rb | 44 ++------------- 4 files changed, 28 insertions(+), 143 deletions(-) diff --git a/lib/http/timeout/global.rb b/lib/http/timeout/global.rb index b49cdabb..71dd40d4 100644 --- a/lib/http/timeout/global.rb +++ b/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 diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index dcde7ee9..2bd9658d 100644 --- a/lib/http/timeout/per_operation.rb +++ b/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 diff --git a/spec/lib/http/client_spec.rb b/spec/lib/http/client_spec.rb index 8315469d..5862f533 100644 --- a/spec/lib/http/client_spec.rb +++ b/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 diff --git a/spec/support/http_handling_shared.rb b/spec/support/http_handling_shared.rb index 26c7d903..2710bf40 100644 --- a/spec/support/http_handling_shared.rb +++ b/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