Merge pull request #194 from zanker/state-tracking

Add an extra set of state tracking for Timeout.timeout edge cases

Author: zanker

.rubocop.yml

@@ -5,6 +5,9 @@ Metrics/ClassLength:
   CountComments: false
   Max: 110
 
+Metrics/PerceivedComplexity:
+  Max: 8
+
 Metrics/CyclomaticComplexity:
   Max: 8 # TODO: Lower to 6
 

lib/http/client.rb

@@ -19,6 +19,7 @@ class Client
     def initialize(default_options = {})
       @default_options = HTTP::Options.new(default_options)
       @connection = nil
+      @state = :clean
     end
 
     # Make an HTTP request
@@ -56,6 +57,8 @@ def perform(req, options)
     def make_request(req, options)
       verify_connection!(req.uri)
 
+      @state = :dirty
+
       @connection ||= HTTP::Connection.new(req, options)
       @connection.send_request(req)
       @connection.read_headers!
@@ -69,6 +72,7 @@ def make_request(req, options)
       )
 
       @connection.finish_response if req.verb == :head
+      @state = :clean
 
       res
 
@@ -83,6 +87,7 @@ def make_request(req, options)
     def close
       @connection.close if @connection
       @connection = nil
+      @state = :clean
     end
 
     private
@@ -91,11 +96,14 @@ def close
     def verify_connection!(uri)
       if default_options.persistent? && base_host(uri) != default_options.persistent
         fail StateError, "Persistence is enabled for #{default_options.persistent}, but we got #{base_host(uri)}"
-
       # We re-create the connection object because we want to let prior requests
       # lazily load the body as long as possible, and this mimics prior functionality.
       elsif @connection && (!@connection.keep_alive? || @connection.expired?)
         close
+      # If we get into a bad state (eg, Timeout.timeout ensure being killed)
+      # close the connection to prevent potential for mixed responses.
+      elsif @state == :dirty
+        close
       end
     end
 

spec/support/http_handling_shared.rb

@@ -95,13 +95,13 @@
       it "errors if connecting takes too long" do
         socket = Socket.new(Socket::AF_INET, Socket::SOCK_STREAM, 0)
 
-        fake_socket = double(:to_io => socket)
-        expect(fake_socket).to receive(:connect_nonblock) do |*args|
+        fake_socket_id = double(:to_io => socket)
+        expect(fake_socket_id).to receive(:connect_nonblock) do |*args|
           sleep 1.25
           socket.connect_nonblock(*args)
         end
 
-        allow_any_instance_of(timeout_class).to receive(:socket).and_return(fake_socket)
+        allow_any_instance_of(timeout_class).to receive(:socket).and_return(fake_socket_id)
 
         expect { response }.to raise_error(HTTP::TimeoutError, /Timed out/)
       end
@@ -148,6 +148,18 @@
         expect(sockets_used.uniq.length).to eq(1)
       end
 
+      context "on a mixed state" do
+        it "re-opens the connection" do
+          first_socket_id = client.get("#{server.endpoint}/socket/1").body.to_s
+
+          client.instance_variable_set(:@state, :dirty)
+
+          second_socket_id = client.get("#{server.endpoint}/socket/2").body.to_s
+
+          expect(first_socket_id).to_not eq(second_socket_id)
+        end
+      end
+
       context "when trying to read a stale body" do
         it "errors" do
           client.get("#{server.endpoint}/not-found")
@@ -169,8 +181,8 @@
 
       context "with a socket issue" do
         it "transparently reopens" do
-          first_socket = client.get("#{server.endpoint}/socket").body.to_s
-          expect(first_socket).to_not eq("")
+          first_socket_id = client.get("#{server.endpoint}/socket").body.to_s
+          expect(first_socket_id).to_not eq("")
           # Kill off the sockets we used
           # rubocop:disable Style/RescueModifier
           DummyServer::Servlet.sockets.each do |socket|
@@ -183,8 +195,8 @@
           expect { client.get("#{server.endpoint}/socket").body.to_s }.to raise_error(IOError)
 
           # Should succeed since we create a new socket
-          second_socket = client.get("#{server.endpoint}/socket").body.to_s
-          expect(second_socket).to_not eq(first_socket)
+          second_socket_id = client.get("#{server.endpoint}/socket").body.to_s
+          expect(second_socket_id).to_not eq(first_socket_id)
         end
       end