Merge pull request #166 from tarcieri/fix/eof-handling

Fix/eof handling

lib/http/client.rb

@@ -55,11 +55,7 @@ def perform(req, options)
 
       req.stream @socket
 
-      begin
-        read_more BUFFER_SIZE until @parser.headers
-      rescue IOError, Errno::ECONNRESET, Errno::EPIPE => ex
-        raise IOError, "problem making HTTP request: #{ex}"
-      end
+      read_headers!
 
       body = Response::Body.new(self)
       res  = Response.new(@parser.status_code, @parser.http_version, @parser.headers, body, uri)
@@ -70,13 +66,22 @@ def perform(req, options)
     end
 
     # Read a chunk of the body
+    #
+    # @return [String] data chunk
+    # @return [Nil] when no more data left
     def readpartial(size = BUFFER_SIZE)
       return unless @socket
 
-      read_more size
+      begin
+        read_more size
+        finished = @parser.finished?
+      rescue EOFError
+        finished = true
+      end
+
       chunk = @parser.chunk
 
-      finish_response if @parser.finished?
+      finish_response if finished
 
       chunk.to_s
     end
@@ -95,7 +100,7 @@ def start_tls(socket, options)
 
     # Merges query params if needed
     def make_request_uri(uri, options)
-      uri = URI uri.to_s unless uri.is_a? URI
+      uri = normalize_uri uri
 
       if options.params && !options.params.empty?
         params    = CGI.parse(uri.query.to_s).merge(options.params || {})
@@ -105,6 +110,21 @@ def make_request_uri(uri, options)
       uri
     end
 
+    # Normalize URI
+    #
+    # @param [#to_s] uri
+    # @return [URI]
+    def normalize_uri(uri)
+      uri = URI uri.to_s
+
+      # Some proxies (seen on WEBRick) fail if URL has
+      # empty path (e.g. `http://example.com`) while it's RFC-complaint:
+      # http://tools.ietf.org/html/rfc1738#section-3.1
+      uri.path = '/' if uri.path.empty?
+
+      uri
+    end
+
     # Create the request body object to send
     def make_request_body(opts, headers)
       if opts.body
@@ -118,6 +138,14 @@ def make_request_body(opts, headers)
       end
     end
 
+    # Reads data from socket up until headers
+    def read_headers!
+      read_more BUFFER_SIZE until @parser.headers
+    rescue IOError, Errno::ECONNRESET, Errno::EPIPE => ex
+      return if ex.is_a?(EOFError) && @parser.headers
+      raise IOError, "problem making HTTP request: #{ex}"
+    end
+
     # Callback for when we've reached the end of a response
     def finish_response
       @socket.close if @socket && !@socket.closed?
@@ -129,9 +157,6 @@ def finish_response
     # Feeds some more data into parser
     def read_more(size)
       @parser << @socket.readpartial(size) unless @parser.finished?
-      true
-    rescue EOFError
-      false
     end
   end
 end

spec/http/client_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 RSpec.describe HTTP::Client do
+  let(:test_endpoint)  { "http://#{ExampleServer::ADDR}" }
+
   StubbedClient = Class.new(HTTP::Client) do
     def perform(request, options)
       stubs.fetch(request.uri.to_s) { super(request, options) }
@@ -148,23 +150,80 @@ def simple_response(body, status = 200)
     it 'calls finish_response before actual performance' do
       allow(TCPSocket).to receive(:open) { throw :halt }
       expect(client).to receive(:finish_response)
-      catch(:halt) { client.head "http://127.0.0.1:#{ExampleService::PORT}/" }
+      catch(:halt) { client.head test_endpoint }
     end
 
     it 'calls finish_response once body was fully flushed' do
       expect(client).to receive(:finish_response).twice.and_call_original
-      client.get("http://127.0.0.1:#{ExampleService::PORT}/").to_s
+      client.get(test_endpoint).to_s
     end
 
     context 'with HEAD request' do
       it 'does not iterates through body' do
         expect(client).to_not receive(:readpartial)
-        client.head("http://127.0.0.1:#{ExampleService::PORT}/")
+        client.head(test_endpoint)
       end
 
       it 'finishes response after headers were received' do
         expect(client).to receive(:finish_response).twice.and_call_original
-        client.head("http://127.0.0.1:#{ExampleService::PORT}/")
+        client.head(test_endpoint)
+      end
+    end
+
+    context 'when server closes connection unexpectedly' do
+      before do
+        socket_spy = double
+
+        allow(socket_spy).to receive(:close) { nil }
+        allow(socket_spy).to receive(:closed?) { true }
+        allow(socket_spy).to receive(:readpartial) { chunks.shift.call }
+        allow(socket_spy).to receive(:<<) { nil }
+
+        allow(TCPSocket).to receive(:open) { socket_spy }
+      end
+
+      context 'during headers reading' do
+        let :chunks do
+          [
+            proc { "HTTP/1.1 200 OK\r\n" },
+            proc { "Content-Type: text/html\r" },
+            proc { fail EOFError }
+          ]
+        end
+
+        it 'raises IOError' do
+          expect { client.get test_endpoint }.to raise_error IOError
+        end
+      end
+
+      context 'after headers were flushed' do
+        let :chunks do
+          [
+            proc { "HTTP/1.1 200 OK\r\n" },
+            proc { "Content-Type: text/html\r\n\r\n" },
+            proc { 'unexpected end of f' },
+            proc { fail EOFError }
+          ]
+        end
+
+        it 'reads partially arrived body' do
+          res = client.get(test_endpoint).to_s
+          expect(res).to eq 'unexpected end of f'
+        end
+      end
+
+      context 'when body and headers were flushed in one chunk' do
+        let :chunks do
+          [
+            proc { "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\nunexpected end of f" },
+            proc { fail EOFError }
+          ]
+        end
+
+        it 'reads partially arrived body' do
+          res = client.get(test_endpoint).to_s
+          expect(res).to eq 'unexpected end of f'
+        end
       end
     end
 
@@ -194,7 +253,7 @@ def simple_response(body, status = 200)
       end
 
       it 'properly reads body' do
-        body = client.get("http://127.0.0.1:#{ExampleService::PORT}/").to_s
+        body = client.get(test_endpoint).to_s
         expect(body).to eq '<!doctype html>'
       end
     end

spec/http_spec.rb

@@ -2,7 +2,7 @@
 require 'json'
 
 RSpec.describe HTTP do
-  let(:test_endpoint)  { "http://127.0.0.1:#{ExampleService::PORT}/" }
+  let(:test_endpoint)  { "http://#{ExampleServer::ADDR}" }
 
   context 'getting resources' do
     it 'is easy' do
@@ -12,21 +12,21 @@
 
     context 'with URI instance' do
       it 'is easy' do
-        response = HTTP.get URI(test_endpoint)
+        response = HTTP.get URI test_endpoint
         expect(response.to_s).to match(/<!doctype html>/)
       end
     end
 
     context 'with query string parameters' do
       it 'is easy' do
-        response = HTTP.get "#{test_endpoint}params", :params => {:foo => 'bar'}
+        response = HTTP.get "#{test_endpoint}/params", :params => {:foo => 'bar'}
         expect(response.to_s).to match(/Params!/)
       end
     end
 
     context 'with query string parameters in the URI and opts hash' do
       it 'includes both' do
-        response = HTTP.get "#{test_endpoint}multiple-params?foo=bar", :params => {:baz => 'quux'}
+        response = HTTP.get "#{test_endpoint}/multiple-params?foo=bar", :params => {:baz => 'quux'}
         expect(response.to_s).to match(/More Params!/)
       end
     end
@@ -73,26 +73,26 @@
 
   context 'posting forms to resources' do
     it 'is easy' do
-      response = HTTP.post "#{test_endpoint}form", :form => {:example => 'testing-form'}
+      response = HTTP.post "#{test_endpoint}/form", :form => {:example => 'testing-form'}
       expect(response.to_s).to eq('passed :)')
     end
   end
 
   context 'posting with an explicit body' do
     it 'is easy' do
-      response = HTTP.post "#{test_endpoint}body", :body => 'testing-body'
+      response = HTTP.post "#{test_endpoint}/body", :body => 'testing-body'
       expect(response.to_s).to eq('passed :)')
     end
   end
 
   context 'with redirects' do
     it 'is easy for 301' do
-      response = HTTP.with_follow(true).get("#{test_endpoint}redirect-301")
+      response = HTTP.with_follow(true).get("#{test_endpoint}/redirect-301")
       expect(response.to_s).to match(/<!doctype html>/)
     end
 
     it 'is easy for 302' do
-      response = HTTP.with_follow(true).get("#{test_endpoint}redirect-302")
+      response = HTTP.with_follow(true).get("#{test_endpoint}/redirect-302")
       expect(response.to_s).to match(/<!doctype html>/)
     end
 

spec/support/example_server.rb

@@ -1,101 +1,29 @@
 require 'webrick'
+require 'singleton'
+require 'forwardable'
 
-class ExampleService < WEBrick::HTTPServlet::AbstractServlet
-  PORT = 65432 # rubocop:disable NumericLiterals
+require 'support/example_server/servlet'
 
-  def do_GET(request, response) # rubocop:disable MethodName
-    case request.path
-    when '/'
-      handle_root(request, response)
-    when '/params'
-      handle_params(request, response)
-    when '/multiple-params'
-      handle_multiple_params(request, response)
-    when '/proxy'
-      response.status = 200
-      response.body     = 'Proxy!'
-    when '/not-found'
-      response.body   = 'not found'
-      response.status = 404
-    when '/redirect-301'
-      response.status = 301
-      response['Location'] = "http://127.0.0.1:#{PORT}/"
-    when '/redirect-302'
-      response.status = 302
-      response['Location'] = "http://127.0.0.1:#{PORT}/"
-    else
-      response.status = 404
-    end
-  end
+class ExampleServer
+  extend  Forwardable
 
-  def handle_root(request, response)
-    response.status = 200
-    case request['Accept']
-    when 'application/json'
-      response['Content-Type'] = 'application/json'
-      response.body = '{"json": true}'
-    else
-      response['Content-Type'] = 'text/html'
-      response.body   = '<!doctype html>'
-    end
-  end
+  include Singleton
 
-  def handle_params(request, response)
-    return unless request.query_string == 'foo=bar'
+  PORT = 65_432
+  ADDR = "127.0.0.1:#{PORT}"
 
-    response.status = 200
-    response.body   = 'Params!'
+  def initialize
+    @server = WEBrick::HTTPServer.new :Port => PORT, :AccessLog => []
+    @server.mount '/', Servlet
   end
 
-  def handle_multiple_params(request, response)
-    params = CGI.parse(request.query_string)
-
-    return unless params == {'foo' => ['bar'], 'baz' => ['quux']}
-
-    response.status = 200
-    response.body   = 'More Params!'
-  end
-
-  def do_POST(request, response) # rubocop:disable MethodName
-    case request.path
-    when '/form'
-      if request.query['example'] == 'testing-form'
-        response.status = 200
-        response.body   = 'passed :)'
-      else
-        response.status = 400
-        response.body   = 'invalid! >:E'
-      end
-    when '/body'
-      if request.body == 'testing-body'
-        response.status = 200
-        response.body   = 'passed :)'
-      else
-        response.status = 400
-        response.body   = 'invalid! >:E'
-      end
-    else
-      response.status = 404
-    end
-  end
-
-  def do_HEAD(request, response) # rubocop:disable MethodName
-    case request.path
-    when '/'
-      response.status = 200
-      response['Content-Type'] = 'text/html'
-    else
-      response.status = 404
-    end
-  end
+  delegate [:start, :shutdown] => :@server
 end
 
-ExampleServer = WEBrick::HTTPServer.new(:Port => ExampleService::PORT, :AccessLog => [])
-ExampleServer.mount '/', ExampleService
+t = Thread.new { ExampleServer.instance.start }
 
-t = Thread.new { ExampleServer.start }
 trap('INT') do
-  ExampleServer.shutdown
+  ExampleServer.instance.shutdown
   exit
 end