Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns <tom.burns@jadedpixel.com>]

On Tue, Oct 30, 2012 at 5:37 PM, Eric Wong <normalperson@yhbt.net> wrote:
>
> Tom Burns <tom.burns@jadedpixel.com> wrote:
> > We'd prefer to not have to fork unicorn for this change.  How do you
> > feel about merging this or a derivative thereof?  I can develop this
> > further if you can send me what you'd want.
>
> Sure thing!

Below is a patch for this functionality.

We're going to be testing this further next week before rolling it out
in production.

It's still pre-writing the entire HTTP/1.1 header start, but as just
two strings.  It could be shortened to just pre-writing HT but I
thought writing that full word looked cleaner.

Please let me know what you think.

The only thing it's missing from your TODO is enforcing
tcp_nodelay/tcp_nopush as I wasn't sure where was the best place to do
the enforcement.

Never done this inline with GMail, sorry in advance if formatting gets
destroyed :)

Cheers,
Tom

---
 examples/unicorn.conf.rb       |    6 ++++++
 lib/unicorn/configurator.rb    |    7 +++++++
 lib/unicorn/const.rb           |    4 ++++
 lib/unicorn/http_request.rb    |   16 ++++++++++++++++
 lib/unicorn/http_response.rb   |    6 +++++-
 lib/unicorn/http_server.rb     |   19 ++++++++++++++++++-
 test/exec/test_exec.rb         |    7 ++++++-
 test/unit/test_configurator.rb |   12 ++++++++++++
 8 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/examples/unicorn.conf.rb b/examples/unicorn.conf.rb
index 0238043..1f4c9c0 100644
--- a/examples/unicorn.conf.rb
+++ b/examples/unicorn.conf.rb
@@ -46,6 +46,12 @@ preload_app true
 GC.respond_to?(:copy_on_write_friendly=) and
   GC.copy_on_write_friendly = true

+# Enable this flag to have unicorn test client connections by writing the
+# beginning of the HTTP headers before calling the application.  This
+# prevents calling the application for connections that have disconnected
+# while queued.
+check_client_connection false
+
 before_fork do |server, worker|
   # the following is highly recomended for Rails + "preload_app true"
   # as there's no need for the master process to hold a connection
diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 89cbf5c..ca84a88 100644
--- a/lib/unicorn/configurator.rb
+++ b/lib/unicorn/configurator.rb
@@ -45,6 +45,7 @@ class Unicorn::Configurator
       },
     :pid => nil,
     :preload_app => false,
+    :check_client_connection => false,
     :rewindable_input => true, # for Rack 2.x: (Rack::VERSION[0] <= 1),
     :client_body_buffer_size => Unicorn::Const::MAX_BODY,
     :trust_x_forwarded => true,
@@ -454,6 +455,12 @@ class Unicorn::Configurator
     set_int(:client_body_buffer_size, bytes, 0)
   end

+  # When enabled, unicorn will check the client connection by writing
+  # the beginning of the HTTP headers before calling the application.
+  def check_client_connection(bool)
+    set_bool(:check_client_connection, bool)
+  end
+
   # Allow redirecting $stderr to a given path.  Unlike doing this from
   # the shell, this allows the unicorn process to know the path its
   # writing to and rotate the file if it is used for logging.  The
diff --git a/lib/unicorn/const.rb b/lib/unicorn/const.rb
index b3d8d71..56598d9 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -33,8 +33,12 @@ module Unicorn::Const
   ERROR_414_RESPONSE = "HTTP/1.1 414 Request-URI Too Long\r\n\r\n"
   ERROR_413_RESPONSE = "HTTP/1.1 413 Request Entity Too Large\r\n\r\n"
   ERROR_500_RESPONSE = "HTTP/1.1 500 Internal Server Error\r\n\r\n"
+
   EXPECT_100_RESPONSE = "HTTP/1.1 100 Continue\r\n\r\n"
+  EXPECT_100_RESPONSE_SUFFIXED = "100 Continue\r\n\r\nHTTP/1.1 "

+  HTTP_RESPONSE_START = ['HTTP', '/1.1 ']
   HTTP_EXPECT = "HTTP_EXPECT"
+
   # :startdoc:
 end
diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index a0435d6..095ca7c 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -27,6 +27,7 @@ class Unicorn::HttpParser
   REMOTE_ADDR = 'REMOTE_ADDR'.freeze
   RACK_INPUT = 'rack.input'.freeze
   @@input_class = Unicorn::TeeInput
+  @@check_client_connection = false

   def self.input_class
     @@input_class
@@ -35,6 +36,15 @@ class Unicorn::HttpParser
   def self.input_class=(klass)
     @@input_class = klass
   end
+
+  def self.check_client_connection
+    @@check_client_connection
+  end
+
+  def self.check_client_connection=(bool)
+    @@check_client_connection = bool
+  end
+
   # :startdoc:

   # Does the majority of the IO processing.  It has been written in
@@ -70,6 +80,12 @@ class Unicorn::HttpParser
       # an Exception thrown from the parser will throw us out of the loop
       false until add_parse(socket.kgio_read!(16384))
     end
+
+    # detect if the socket is valid by writing a partial response:
+    if @@check_client_connection && headers?
+      Unicorn::Const::HTTP_RESPONSE_START.each { |c| socket.write(c) }
+    end
+
     e[RACK_INPUT] = 0 == content_length ?
                     NULL_IO : @@input_class.new(socket, self)
     e.merge!(DEFAULTS)
diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index b781e20..10a92d1 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -17,12 +17,16 @@ module Unicorn::HttpResponse
   }
   CRLF = "\r\n"

+  def http_response_start
+    Unicorn::HttpParser.check_client_connection ? '' : 'HTTP/1.1 '
+  end
+
   # writes the rack_response to socket as an HTTP response
   def http_response_write(socket, status, headers, body)
     status = CODES[status.to_i] || status

     if headers
-      buf = "HTTP/1.1 #{status}\r\n" \
+      buf = "#{http_response_start}#{status}\r\n" \
             "Date: #{httpdate}\r\n" \
             "Status: #{status}\r\n" \
             "Connection: close\r\n"
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index 13df55a..3e061af 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -17,6 +17,7 @@ class Unicorn::HttpServer
                 :listener_opts, :preload_app,
                 :reexec_pid, :orig_app, :init_listeners,
                 :master_pid, :config, :ready_pipe, :user
+
   attr_reader :pid, :logger
   include Unicorn::SocketHelper
   include Unicorn::HttpResponse
@@ -355,6 +356,14 @@ class Unicorn::HttpServer
     Unicorn::HttpParser.trust_x_forwarded = bool
   end

+  def check_client_connection
+    Unicorn::HttpRequest.check_client_connection
+  end
+
+  def check_client_connection=(bool)
+    Unicorn::HttpRequest.check_client_connection = bool
+  end
+
   private

   # wait for a signal hander to wake us up and then consume the pipe
@@ -529,13 +538,21 @@ class Unicorn::HttpServer
     rescue
   end

+  def expect_100_response
+    if Unicorn::HttpRequest.check_client_connection
+      Unicorn::Const::EXPECT_100_RESPONSE_SUFFIXED
+    else
+      Unicorn::Const::EXPECT_100_RESPONSE
+    end
+  end
+
   # 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)
     status, headers, body = @app.call(env = @request.read(client))

     if 100 == status.to_i
-      client.write(Unicorn::Const::EXPECT_100_RESPONSE)
+      client.write(expect_100_response)
       env.delete(Unicorn::Const::HTTP_EXPECT)
       status, headers, body = @app.call(env)
     end
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index b30a3d6..1cee2b7 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -871,13 +871,14 @@ EOF
     wait_for_death(pid)
   end

-  def hup_test_common(preload)
+  def hup_test_common(preload, check_client=false)
     File.open("config.ru", "wb") { |fp| fp.syswrite(HI.gsub("HI", '#$$')) }
     pid_file = Tempfile.new('pid')
     ucfg = Tempfile.new('unicorn_test_config')
     ucfg.syswrite("listen '#@addr:#@port'\n")
     ucfg.syswrite("pid '#{pid_file.path}'\n")
     ucfg.syswrite("preload_app true\n") if preload
+    ucfg.syswrite("check_client_connection true\n") if check_client
     ucfg.syswrite("stderr_path 'test_stderr.#$$.log'\n")
     ucfg.syswrite("stdout_path 'test_stdout.#$$.log'\n")
     pid = xfork {
@@ -942,6 +943,10 @@ EOF
     hup_test_common(false)
   end

+  def test_check_client_hup
+    hup_test_common(false, true)
+  end
+
   def test_default_listen_hup_holds_listener
     default_listen_lock do
       res, pid_path = default_listen_setup
diff --git a/test/unit/test_configurator.rb b/test/unit/test_configurator.rb
index c19c427..fc4170e 100644
--- a/test/unit/test_configurator.rb
+++ b/test/unit/test_configurator.rb
@@ -139,6 +139,18 @@ class TestConfigurator < Test::Unit::TestCase
     end
   end

+  def test_check_client_connection
+    tmp = Tempfile.new('unicorn_config')
+    test_struct = TestStruct.new
+    tmp.syswrite("check_client_connection true\n")
+
+    assert_nothing_raised do
+      Unicorn::Configurator.new(:config_file => tmp.path).commit!(test_struct)
+    end
+
+    assert test_struct.check_client_connection
+  end
+
   def test_after_fork_proc
     test_struct = TestStruct.new
     [ proc { |a,b| }, Proc.new { |a,b| }, lambda { |a,b| } ].each do |my_proc|
-- 
1.7.7.5 (Apple Git-26)
_______________________________________________
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