Handling closed clients

* Handling closed clients
@ 2013-11-05 14:46 Andrew Hobson
  2013-11-05 17:20 ` Eric Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Hobson @ 2013-11-05 14:46 UTC (permalink / raw)
  To: mongrel-unicorn

We have a service that clients use to upload files. We have a couple of clients that are on slow links and so their upload times out. We get errors in the logs that I'd like to get rid of.

I was hoping that the recent commit 24b9f66dcdda44378b4053645333ce9ce336b413 would help us, but it does not. After digging in a bit, I have some ideas about why and a patch I'd like comments on.

The commit above attempts to ignore exceptions of types EOFError, Errno::ECONNRESET, Errno::EPIPE, and Errno::ENOTCONN. However, that doesn't address our issue because our exception comes when trying to write the response. It is a generic IOError and thus is not handled.

In addition, I believe that change is unsafe because I believe client code could raise those exceptions. Think about a client with a database connection: that could also raise e.g. EOFError. I believe unicorn needs to check the status of the client connection instead of the type of exception that is raised.

The patch below has two new test cases: one where the client code raises an EOFError and one that replicates the condition we see where the client socket is closed before the response can be written.

It changes the behavior of the 'test_client_shutdown_write_truncates' test, but I believe the change makes sense. If the client socket is still open for writing, unicorn should write the response.

Thoughts?

--drew

diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index 402f133..3620427 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -547,8 +547,6 @@ class Unicorn::HttpServer
# the socket is closed at the end of this function
def handle_error(client, e)
code = case e
- when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::ENOTCONN
- # client disconnected on us and there's nothing we can do
when Unicorn::RequestURITooLongError
414
when Unicorn::RequestEntityTooLargeError
@@ -556,8 +554,12 @@ class Unicorn::HttpServer
when Unicorn::HttpParserError # try to tell the client they're bad
400
else
- Unicorn.log_error(@logger, "app error", e)
- 500
+ if client.closed?
+ # client disconnected on us and there's nothing we can do
+ else
+ Unicorn.log_error(@logger, "app error", e)
+ 500
+ end
end
if code
client.kgio_trywrite(err_response(code, @request.response_start_sent))
diff --git a/test/unit/test_server.rb b/test/unit/test_server.rb
index e5b335f..2fca1b4 100644
--- a/test/unit/test_server.rb
+++ b/test/unit/test_server.rb
@@ -145,8 +145,7 @@ class WebServerTest < Test::Unit::TestCase
# processing on us even during app dispatch
sock.shutdown(Socket::SHUT_WR)
IO.select([sock], nil, nil, 60) or raise "Timed out"
- buf = sock.read
- assert_equal "", buf
+ assert_match %r{\AHTTP/1.[01] 500\b}, sock.sysread(4096)
next_client = Net::HTTP.get(URI.parse("http://127.0.0.1:#@port/"))
assert_equal 'hello!\n', next_client
lines = File.readlines("test_stderr.#$$.log")
@@ -265,4 +264,55 @@ class WebServerTest < Test::Unit::TestCase
def test_listener_names
assert_equal [ "127.0.0.1:#@port" ], Unicorn.listener_names
end
+
+ # ensure that EOFError from client code is not ignored
+ def test_eof_app
+ teardown
+ app = lambda { |env| raise EOFError }
+ # [200, {}, []] }
+ redirect_test_io do
+ @server = HttpServer.new(app, :listeners => [ "127.0.0.1:#@port"] )
+ @server.start
+ end
+ sock = TCPSocket.new('127.0.0.1', @port)
+ sock.syswrite("GET / HTTP/1.0\r\n\r\n")
+ assert_match %r{\AHTTP/1.[01] 500\b}, sock.sysread(4096)
+ assert_nil sock.close
+ lines = File.readlines("test_stderr.#$$.log")
+ assert lines.grep(/app error:/)
+ end
+
+ def test_file_streamed_request_close
+ teardown
+ # do a funky dance so that the socket is closed partway though the request
+ tmp = Tempfile.new('test_file_streamed_request_close')
+ app = lambda { |env|
+ while File.zero?(tmp.path)
+ sleep(0.2)
+ end
+ tmp.unlink
+ o = env['rack.input'].instance_variable_get(:@socket)
+ o.close
+ hdrs = { 'Content-Type' => 'text/plain' }
+ [ 200, hdrs, [ "#$$\n" ] ]
+ }
+ # [200, {}, []] }
+ redirect_test_io do
+ @server = HttpServer.new(app, :listeners => [ "127.0.0.1:#@port"] )
+ @server.start
+ end
+ sock = TCPSocket.new('127.0.0.1', @port)
+ body = "a" * 10
+ long = "PUT /test HTTP/1.1\r\nContent-length: #{body.length + 10}\r\n\r\n" + body
+ sock.syswrite(long)
+ sock.close
+ tmp.puts "ok"
+ tmp.flush
+ while File.exists?(tmp.path)
+ sleep(0.2)
+ end
+ lines = File.readlines("test_stderr.#$$.log")
+ assert lines.grep(/app error:/).empty?
+ end
+
end






_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

^ permalink raw reply related	[flat|nested] 5+ messages in thread