Fixed #2908: better handling of errors in HTTP::Server

spec/std/http/server/server_spec.cr

@@ -1,6 +1,24 @@
 require "spec"
 require "http/server"
 
+module IO
+  class RaiseErrno
+    def initialize(@value : Int32)
+    end
+
+    include IO
+
+    def read(slice : Slice(UInt8))
+      Errno.value = @value
+      raise Errno.new "..."
+    end
+
+    def write(slice : Slice(UInt8)) : Nil
+      raise "not implemented"
+    end
+  end
+end
+
 module HTTP
   class Server
     class ReverseResponseOutput
@@ -207,6 +225,46 @@ module HTTP
     end
   end
 
+  describe HTTP::Server::RequestProcessor do
+    it "works" do
+      processor = HTTP::Server::RequestProcessor.new do |context|
+        context.response.content_type = "text/plain"
+        context.response.print "Hello world"
+      end
+
+      input = MemoryIO.new("GET / HTTP/1.1\r\n\r\n")
+      output = MemoryIO.new
+      processor.process(input, output)
+      output.rewind
+      output.gets_to_end.should eq(requestize(<<-RESPONSE
+        HTTP/1.1 200 OK
+        Connection: keep-alive
+        Content-Type: text/plain
+        Content-Length: 11
+
+        Hello world
+        RESPONSE
+      ))
+    end
+
+    it "handles Errno" do
+      processor = HTTP::Server::RequestProcessor.new { }
+      input = IO::RaiseErrno.new(Errno::ECONNRESET)
+      output = MemoryIO.new
+      processor.process(input, output)
+      output.rewind.gets_to_end.empty?.should be_true
+    end
+
+    it "catches raised error on handler" do
+      processor = HTTP::Server::RequestProcessor.new { raise "OH NO" }
+      input = MemoryIO.new("GET / HTTP/1.1\r\n\r\n")
+      output = MemoryIO.new
+      error = MemoryIO.new
+      processor.process(input, output, error)
+      output.rewind.gets_to_end.should match(/Internal Server Error/)
+    end
+  end
+
   typeof(begin
     # Initialize with custom host
     server = Server.new("0.0.0.0", 0) { |ctx| }
@@ -247,3 +305,7 @@ module HTTP
     server.close
   end)
 end
+
+private def requestize(string)
+  string.gsub("\n", "\r\n")
+end

src/http/request.cr

@@ -52,19 +52,27 @@ class HTTP::Request
     HTTP.serialize_headers_and_body(io, headers, @body, nil, @version)
   end
 
+  # :nodoc:
+  record BadRequest
+
+  # Returns:
+  # * nil: EOF
+  # * BadRequest: bad request
+  # * HTTP::Request: successfully parsed
   def self.from_io(io)
     request_line = io.gets
     return unless request_line
+
     parts = request_line.split
-    return unless parts.size == 3
+    return BadRequest.new unless parts.size == 3
 
     method, resource, http_version = parts
     HTTP.parse_headers_and_body(io) do |headers, body|
       return new method, resource, headers, body.try &.gets_to_end, http_version
     end
 
     # Unexpected end of http request
-    nil
+    BadRequest.new
   end
 
   # Lazily parses and return the request's path component.

src/http/server.cr

@@ -110,18 +110,22 @@ class HTTP::Server
     new("127.0.0.1", port, handler)
   end
 
-  def initialize(@host : String, @port : Int32, &@handler : Context ->)
+  def initialize(@host : String, @port : Int32, &handler : Context ->)
+    @processor = RequestProcessor.new(handler)
   end
 
   def initialize(@host : String, @port : Int32, handlers : Array(HTTP::Handler), &handler : Context ->)
-    @handler = HTTP::Server.build_middleware handlers, handler
+    handler = HTTP::Server.build_middleware handlers, handler
+    @processor = RequestProcessor.new(handler)
   end
 
   def initialize(@host : String, @port : Int32, handlers : Array(HTTP::Handler))
-    @handler = HTTP::Server.build_middleware handlers
+    handler = HTTP::Server.build_middleware handlers
+    @processor = RequestProcessor.new(handler)
   end
 
-  def initialize(@host : String, @port : Int32, @handler : HTTP::Handler | HTTP::Handler::Proc)
+  def initialize(@host : String, @port : Int32, handler : HTTP::Handler | HTTP::Handler::Proc)
+    @processor = RequestProcessor.new(handler)
   end
 
   def port
@@ -145,6 +149,7 @@ class HTTP::Server
 
   def close
     @wants_close = true
+    @processor.close
     if server = @server
       server.close
       @server = nil
@@ -163,49 +168,7 @@ class HTTP::Server
       end
     end
 
-    must_close = true
-    response = Response.new(io)
-
-    begin
-      until @wants_close
-        begin
-          request = HTTP::Request.from_io(io)
-        rescue e
-          STDERR.puts "Bug: Unhandled exception while parsing request"
-          e.inspect_with_backtrace(STDERR)
-        end
-
-        unless request
-          response.respond_with_error("Bad Request", 400)
-          response.close
-          return
-        end
-
-        response.version = request.version
-        response.reset
-        response.headers["Connection"] = "keep-alive" if request.keep_alive?
-        context = Context.new(request, response)
-
-        @handler.call(context)
-
-        if response.upgraded?
-          must_close = false
-          return
-        end
-
-        response.output.close
-        io.flush
-
-        break unless request.keep_alive?
-      end
-    rescue e
-      response.respond_with_error
-      response.close
-      STDERR.puts "Unhandled exception while computing response, make sure to catch all your exceptions"
-      e.inspect_with_backtrace(STDERR)
-    ensure
-      io.close if must_close
-    end
+    @processor.process(io, io)
   end
 
   # Builds all handlers as the middleware for HTTP::Server.

src/http/server/request_processor.cr

@@ -0,0 +1,64 @@
+require "../server"
+
+class HTTP::Server::RequestProcessor
+  def initialize(&@handler : HTTP::Handler::Proc)
+    @wants_close = false
+  end
+
+  def initialize(@handler : HTTP::Handler | HTTP::Handler::Proc)
+    @wants_close = false
+  end
+
+  def close
+    @wants_close = true
+  end
+
+  def process(input, output, error = STDERR)
+    must_close = true
+    response = Response.new(output)
+
+    begin
+      until @wants_close
+        request = HTTP::Request.from_io(input)
+
+        # EOF
+        break unless request
+
+        if request.is_a?(HTTP::Request::BadRequest)
+          response.respond_with_error("Bad Request", 400)
+          response.close
+          return
+        end
+
+        response.version = request.version
+        response.reset
+        response.headers["Connection"] = "keep-alive" if request.keep_alive?
+        context = Context.new(request, response)
+
+        begin
+          @handler.call(context)
+        rescue ex
+          response.respond_with_error
+          response.close
+          error.puts "Unhandled exception on HTTP::Handler"
+          ex.inspect_with_backtrace(error)
+          return
+        end
+
+        if response.upgraded?
+          must_close = false
+          return
+        end
+
+        response.output.close
+        output.flush
+
+        break unless request.keep_alive?
+      end
+    rescue ex : Errno
+      # IO-related error, nothing to do
+    ensure
+      input.close if must_close
+    end
+  end
+end