dealing with client disconnects with TeeInput

dealing with client disconnects with TeeInput

[Eric Wong]

Foreword: this probably doesn't affect nginx+Unicorn users, which is the
recommended configuration for the vast majority of sites.  It probably
affects Rainbows! users using Thread* or Revactor the most, and probably
some Unicorn users serving Intranet clients directly.

When clients are uploading large files, there's always a good
possibility of them disconnecting before the upload ends.  For other web
app servers it's not much of a problem: they read the entire upload
before attempting to process things; so the app never sees a prematurely
disconnected client.

However Rainbows! and Unicorn have the TeeInput class which allows
real-time processing of uploads as they occur.  Now, we _want_ the
exception to be thrown and application to stop processing the dead
client request immediately.  I've made changes in unicorn.git and
rainbows.git to ensure no EOFError exceptions from the socket are
silenced, not just ones from reading trailers.

However, this means (many more) socket errors will be seen within the
application and any global exception trappers they use will see them as
well.  For Rails (and possibly other frameworks), this can mean very
messy log files with large backtraces.

So, would making a Unicorn::Disconnect < EOFError exception class and
raising it with a short/empty backtrace on EOFErrors be the best way to
go?  That way those global exception trappers can distinguish between
EOFError exceptions raised by Unicorn/Rainbows! itself and other code
that Unicorn/Rainbows does not care about, and log appropriately...

The other option we have is catch/throw.  We can avoid worrying about
the stack trace entirely, and middlewares that opt-in can still capture
and log the disconnect if they want to.  More maintenance overhead for
Rainbows! with all its concurrency models, but this is a situation
where I think catch/throw is appropriate for given the current
middleware/application stacks these days.

Thanks for reading.

-- 
Eric Wong

Re: dealing with client disconnects with TeeInput

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> So, would making a Unicorn::Disconnect < EOFError exception class and
> raising it with a short/empty backtrace on EOFErrors be the best way to
> go?  That way those global exception trappers can distinguish between
> EOFError exceptions raised by Unicorn/Rainbows! itself and other code
> that Unicorn/Rainbows does not care about, and log appropriately...

I actually named it Unicorn::ClientShutdown since I figured the
name would be more descriptive.  Here's what I've pushed out
to unicorn.git:

>From e4256da292f9626d7dfca60e08f65651a0a9139a Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Sat, 14 Nov 2009 00:23:19 +0000
Subject: [PATCH] raise Unicorn::ClientShutdown if client aborts in TeeInput

Leaving the EOFError exception as-is bad because most
applications/frameworks run an application-wide exception
handler to pretty-print and/or log the exception with a huge
backtrace.

Since there's absolutely nothing we can do in the server-side
app to deal with clients prematurely shutting down, having a
backtrace does not make sense.  Having a backtrace can even be
harmful since it creates unnecessary noise for application
engineers monitoring or tracking down real bugs.
---
 lib/unicorn.rb           |    6 ++++
 lib/unicorn/tee_input.rb |   11 ++++++++
 test/unit/test_server.rb |   61 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index a696402..c6c311e 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -8,6 +8,12 @@ autoload :Rack, 'rack'
 # a Unicorn web server.  It contains a minimalist HTTP server with just enough
 # functionality to service web application requests fast as possible.
 module Unicorn
+
+  # raise this inside TeeInput when a client disconnects inside the
+  # application dispatch
+  class ClientShutdown < EOFError
+  end
+
   autoload :Const, 'unicorn/const'
   autoload :HttpRequest, 'unicorn/http_request'
   autoload :HttpResponse, 'unicorn/http_response'
diff --git a/lib/unicorn/tee_input.rb b/lib/unicorn/tee_input.rb
index 69397c0..50ddb5b 100644
--- a/lib/unicorn/tee_input.rb
+++ b/lib/unicorn/tee_input.rb
@@ -135,10 +135,21 @@ module Unicorn
         end
       end
       finalize_input
+      rescue EOFError
+        # in case client only did a premature shutdown(SHUT_WR)
+        # we do support clients that shutdown(SHUT_WR) after the
+        # _entire_ request has been sent, and those will not have
+        # raised EOFError on us.
+        socket.close if socket
+        raise ClientShutdown, "bytes_read=#{@tmp.size}", []
     end
 
     def finalize_input
       while parser.trailers(req, buf).nil?
+        # Don't worry about throw-ing :http_499 here on EOFError, tee()
+        # will catch EOFError when app is processing it, otherwise in
+        # initialize we never get any chance to enter the app so the
+        # EOFError will just get trapped by Unicorn and not the Rack app
         buf << socket.readpartial(Const::CHUNK_SIZE)
       end
       self.socket = nil
diff --git a/test/unit/test_server.rb b/test/unit/test_server.rb
index bbb06da..a7f6a35 100644
--- a/test/unit/test_server.rb
+++ b/test/unit/test_server.rb
@@ -12,11 +12,12 @@ include Unicorn
 
 class TestHandler 
 
-  def call(env) 
-  #   response.socket.write("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n\r\nhello!\n")
+  def call(env)
     while env['rack.input'].read(4096)
     end
     [200, { 'Content-Type' => 'text/plain' }, ['hello!\n']]
+    rescue Unicorn::ClientShutdown => e
+      $stderr.syswrite("#{e.class}: #{e.message} #{e.backtrace.empty?}\n")
    end
 end
 
@@ -103,6 +104,62 @@ class WebServerTest < Test::Unit::TestCase
     assert_equal 'hello!\n', results[0], "Handler didn't really run"
   end
 
+  def test_client_shutdown_writes
+    sock = nil
+    buf = nil
+    bs = 15609315 * rand
+    assert_nothing_raised do
+      sock = TCPSocket.new('127.0.0.1', @port)
+      sock.syswrite("PUT /hello HTTP/1.1\r\n")
+      sock.syswrite("Host: example.com\r\n")
+      sock.syswrite("Transfer-Encoding: chunked\r\n")
+      sock.syswrite("Trailer: X-Foo\r\n")
+      sock.syswrite("\r\n")
+      sock.syswrite("%x\r\n" % [ bs ])
+      sock.syswrite("F" * bs)
+      sock.syswrite("\r\n0\r\nX-")
+      "Foo: bar\r\n\r\n".each_byte do |x|
+        sock.syswrite x.chr
+        sleep 0.05
+      end
+      # we wrote the entire request before shutting down, server should
+      # continue to process our request and never hit EOFError on our sock
+      sock.shutdown(Socket::SHUT_WR)
+      buf = sock.read
+    end
+    assert_equal 'hello!\n', buf.split(/\r\n\r\n/).last
+    lines = File.readlines("test_stderr.#$$.log")
+    assert lines.grep(/^Unicorn::ClientShutdown: /).empty?
+    assert_nothing_raised { sock.close }
+  end
+
+  def test_client_shutdown_write_truncates
+    sock = nil
+    buf = nil
+    bs = 15609315 * rand
+    assert_nothing_raised do
+      sock = TCPSocket.new('127.0.0.1', @port)
+      sock.syswrite("PUT /hello HTTP/1.1\r\n")
+      sock.syswrite("Host: example.com\r\n")
+      sock.syswrite("Transfer-Encoding: chunked\r\n")
+      sock.syswrite("Trailer: X-Foo\r\n")
+      sock.syswrite("\r\n")
+      sock.syswrite("%x\r\n" % [ bs ])
+      sock.syswrite("F" * (bs / 2.0))
+
+      # shutdown prematurely, this will force the server to abort
+      # 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
+    end
+    assert_equal "", buf
+    lines = File.readlines("test_stderr.#$$.log")
+    lines = lines.grep(/^Unicorn::ClientShutdown: bytes_read=\d+/)
+    assert_equal 1, lines.size
+    assert_match %r{\AUnicorn::ClientShutdown: bytes_read=\d+ true$}, lines[0]
+    assert_nothing_raised { sock.close }
+  end
 
   def do_test(string, chunk, close_after=nil, shutdown_delay=0)
     # Do not use instance variables here, because it needs to be thread safe
-- 
Eric Wong