Merge pull request #178 from composerinteralia/raise-after-response-written

Avoid double response on post-response exception

Author: hmcguire-shopify

lib/pitchfork/http_server.rb

@@ -803,7 +803,7 @@ def restart_outdated_workers
     # assuming we haven't closed the socket, but don't get hung up
     # if the socket is already closed or broken.  We'll always ensure
     # the socket is closed at the end of this function
-    def handle_error(client, e)
+    def handle_error(client, e, response_written)
       code = case e
       when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::ENOTCONN
         # client disconnected on us and there's nothing we can do
@@ -817,7 +817,7 @@ def handle_error(client, e)
         Pitchfork.log_error(@logger, "app error", e)
         500
       end
-      if code
+      if code && !response_written
         client.write_nonblock(err_response(code, @request.response_start_sent), exception: false)
       end
       client.close
@@ -842,6 +842,7 @@ def e100_response_write(client, env)
     # once a client is accepted, it is processed in its entirety here
     # in 3 easy steps: read request, call app, write app response
     def process_client(client, worker, timeout_handler)
+      response_written = false
       env = nil
       @request = Pitchfork::HttpParser.new
       env = @request.read(client)
@@ -876,6 +877,7 @@ def process_client(client, worker, timeout_handler)
         end
         @request.headers? or headers = nil
         http_response_write(client, status, headers, body, @request)
+        response_written = true
       ensure
         body.respond_to?(:close) and body.close
       end
@@ -889,7 +891,7 @@ def process_client(client, worker, timeout_handler)
       end
       env
     rescue => application_error
-      handle_error(client, application_error)
+      handle_error(client, application_error, response_written)
       env
     ensure
       if env

test/slow/test_server.rb

@@ -63,6 +63,13 @@ def check_arguments(env, status, headers, application_error)
     end
   end
 
+  class TestRaiseAfterResponseWritten
+    def call(env)
+      body = Rack::BodyProxy.new(['hello!\n']) { raise "response already written" }
+      [200, { 'content-type' => 'text/plain' }, body]
+    end
+  end
+
   def setup
     @valid_request = "GET / HTTP/1.1\r\nHost: www.zedshaw.com\r\nContent-Type: text/plain\r\n\r\n"
     @port = unused_port
@@ -181,6 +188,21 @@ def test_broken_app
     assert_match %r{\AHTTP/1.[01] 500\b}, sock.sysread(4096)
     assert_nil sock.close
   end
+
+  def test_raise_after_response_written
+    redirect_test_io do
+      @server = Pitchfork::HttpServer.new(TestRaiseAfterResponseWritten.new, :listeners => [ "127.0.0.1:#@port"])
+      @server.start
+    end
+
+    sock = tcp_socket('127.0.0.1', @port)
+    sock.syswrite("GET / HTTP/1.0\r\n\r\n")
+
+    responses = sock.read(4096)
+
+    assert_match %r{^HTTP/1.[01] 200\b}, responses
+    refute_match %r{HTTP/1.[01] 500\b}, responses
+  end
 end
 
 class WebServerTest < Pitchfork::Test