Combating nginx 499 HTTP responses during flash traffic scenario

Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

Hi,

We're dealing with an issue with our large-scale deployment of unicorn
& nginx.  The problem occurs during "flash" scenarios where we receive
an order magnitude more traffic for up to an hour.  Much of this
traffic cannot be cached and it's typical for some of our rails
responses to take a few seconds to generate.  Our preference is to
queue incoming requests instead of returning 502 if we can respond
within a reasonable amount of time.

On each of our servers the stack is nginx -> unicorn.

The main connection queue in front of Rails is the unicorn connection
queue.  Our problem is that when the traffic hits, the unicorn queue
grows.  When users begin hitting refresh, their abandoned requests in
the unicorn queue are still passed to the rails application and
rendered.  In this case we see a 200 HTTP response in our Rails log
and a 499 in the nginx access log.  Once this starts happening the
problem can compound: app workers are busy rendering pages for clients
who have already detached so response time grows and more users hit
refresh, etc.

Our nginx config:
6 nginx workers, 1024 worker connections.

Our unicorn config:
70 unicorn workers, 2048 connection backlog.

Our goal is to not have our app process requests for clients that have
already disconnected while their connection is still queued in
unicorn.  We also would prefer not to shrink our queue such that we
begin to return 502 when our queue is a few seconds deep.

We're looking at potential solutions to this problem, including:
- modifying unicorn to select() on the client connection after reading
the request, to see if it's been closed upstream, and avoid calling
the app.
- Replacing nginx with haproxy and queuing connections there.  This
goes against the nginx recommendation at
http://unicorn.bogomips.org/PHILOSOPHY.html

Any input would be appreciated.

Thanks,
Tom
Developer @ Shopify
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> We're looking at potential solutions to this problem, including:
> - modifying unicorn to select() on the client connection after reading
> the request, to see if it's been closed upstream, and avoid calling
> the app.

You should be able to get the socket out of
Unicorn::TeeInput/Unicorn::StreamInput object via:

  env["rack.input"].instance_variable_get(:@socket)

Make sure you don't have other middleware which wraps rack.input
such as Rack::Lint.  I highly doubt the ivar name will change in
the future.

If you're accepting uploads, you (or your app framework) will need to
drain the socket of upload data, first (Rails does this for you, I
think).

> - Replacing nginx with haproxy and queuing connections there.  This
> goes against the nginx recommendation at
> http://unicorn.bogomips.org/PHILOSOPHY.html

Haproxy is fine as long as you have nginx /somewhere/ in between unicorn
and clients.  You have some extra overhead in data copying, but it could
save you cycles...

I'm unsure about the ordering, however:

a) nginx -> haproxy -> unicorn
b) haproxy -> nginx -> unicorn

Though, I suspect a) will be better.

> Any input would be appreciated.

I'd love to hear back on how you eventually solve this, too :>
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Hongli Lai]

On Mon, Oct 29, 2012 at 7:45 PM, Eric Wong <normalperson@yhbt.net> wrote:
>> Any input would be appreciated.
>
> I'd love to hear back on how you eventually solve this, too :>

Does haproxy support removing clients from the queue like this?

-- 
Phusion | Ruby & Rails deployment, scaling and tuning solutions

Web: http://www.phusion.nl/
E-mail: info@phusion.nl
Chamber of commerce no: 08173483 (The Netherlands)
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Hongli Lai <hongli@phusion.nl> wrote:
> On Mon, Oct 29, 2012 at 7:45 PM, Eric Wong <normalperson@yhbt.net> wrote:
> >> Any input would be appreciated.
> >
> > I'd love to hear back on how you eventually solve this, too :>
> 
> Does haproxy support removing clients from the queue like this?

I'm not sure, Tom can investigate further.

I do recall haproxy having more intelligent load distribution than
nginx, so it can send requests to less-busy machines, at least.
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Hongli Lai]

On Mon, Oct 29, 2012 at 8:41 PM, Eric Wong <normalperson@yhbt.net> wrote:
> I'm not sure, Tom can investigate further.
>
> I do recall haproxy having more intelligent load distribution than
> nginx, so it can send requests to less-busy machines, at least.

It is correct that HAProxy can do more intelligent load distribution.
The "maxconn 1" feature in HAProxy is a bit like global queuing in
Phusion Passenger, or the kernel socket queuing that Unicorn relies
on. Phusion Passenger sends the request to the first worker that
becomes available, and the kernel passes the connection to the first
Unicorn instance that accept()s the socket. Nginx's proxy_module
passes requests immediately and only does round robin.

So to solve this problem we need to add 2 features to the layer that
manages the queue:
1. It must be able to detect an early disconnect.
2. It must be able to remove a client from the queue.

The kernel obviously can't do this, but I'm very curious as to whether
HAProxy supports these two things. I also think it shouldn't be too
hard to implement this in Phusion Passenger 4's new architecture.

-- 
Phusion | Ruby & Rails deployment, scaling and tuning solutions

Web: http://www.phusion.nl/
E-mail: info@phusion.nl
Chamber of commerce no: 08173483 (The Netherlands)
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Tom Burns <tom.burns@jadedpixel.com> wrote:
> > We're looking at potential solutions to this problem, including:
> > - modifying unicorn to select() on the client connection after reading
> > the request, to see if it's been closed upstream, and avoid calling
> > the app.
> 
> You should be able to get the socket out of
> Unicorn::TeeInput/Unicorn::StreamInput object via:
> 
>   env["rack.input"].instance_variable_get(:@socket)
> 
> Make sure you don't have other middleware which wraps rack.input
> such as Rack::Lint.  I highly doubt the ivar name will change in
> the future.
> 
> If you're accepting uploads, you (or your app framework) will need to
> drain the socket of upload data, first (Rails does this for you, I
> think).

Maybe this gross hack can work for you guys.  It writes the first
chunk of the HTTP response header out immediately after reading
the request headers, and sends the rest once it gets the status...

It probably needs :tcp_nodelay => false for it to be reliable, though.
(I don't think I need each_byte below, two separate write()s should
be enough Nagle)

It screws up error/exception reporting in some cases, though:

diff --git a/lib/unicorn/const.rb b/lib/unicorn/const.rb
index b3d8d71..818fac8 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -33,7 +33,7 @@ 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 = "100 Continue\r\n\r\n HTTP/1.1"
 
   HTTP_EXPECT = "HTTP_EXPECT"
   # :startdoc:
diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index a0435d6..73fbd41 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -70,6 +70,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 headers?
+      "HTTP/1.1 ".each_char { |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..c2e9d1d 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -22,7 +22,7 @@ module Unicorn::HttpResponse
     status = CODES[status.to_i] || status
 
     if headers
-      buf = "HTTP/1.1 #{status}\r\n" \
+      buf = "#{status}\r\n" \
             "Date: #{httpdate}\r\n" \
             "Status: #{status}\r\n" \
             "Connection: close\r\n"
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

On Mon, Oct 29, 2012 at 5:53 PM, Eric Wong <normalperson@yhbt.net> wrote:
> Maybe this gross hack can work for you guys.  It writes the first
> chunk of the HTTP response header out immediately after reading
> the request headers, and sends the rest once it gets the status...

Eric, thank you very much for your replies.  We'd debated this as an
alternate solution along with the two I mentioned in my original
email, and to be honest your tentative patch is cleaner than I'd had
expected this solution to look like :)

I will test this and respond back.

One of our goals in solving this problem would be to get any changes
merged back into unicorn master, and this looks like it would actually
lead to a cleaner result than having to select() on the socket.

Another side effect of the "select() in a middleware" solution was
going to be removing the NULL_IO optimization that sets rack.input to
a StringIO.

Cheers,
Tom
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

On Mon, Oct 29, 2012 at 5:53 PM, Eric Wong <normalperson@yhbt.net> wrote:
> Maybe this gross hack can work for you guys.  It writes the first
> chunk of the HTTP response header out immediately after reading
> the request headers, and sends the rest once it gets the status...

I tested the patch today and it does what we want, dropping
connections before passing them to the rails app when the client has
already disconnected.

I also benchmarked the patch to see if it had a negligible performance
hit and it did not.  The cost was absorbed by the variation in speed
of the other components in the stack (nginx & rails).

I noticed on my computer applying the patch breaks
test_rack_lint_big_put in the unicorn test suite.  This may be just my
issue as the test suite does not run cleanly anyways if I checkout
origin/master.

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.

Cheers,
Tom
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> On Mon, Oct 29, 2012 at 5:53 PM, Eric Wong <normalperson@yhbt.net> wrote:
> > Maybe this gross hack can work for you guys.  It writes the first
> > chunk of the HTTP response header out immediately after reading
> > the request headers, and sends the rest once it gets the status...
> 
> I tested the patch today and it does what we want, dropping
> connections before passing them to the rails app when the client has
> already disconnected.
> 
> I also benchmarked the patch to see if it had a negligible performance
> hit and it did not.  The cost was absorbed by the variation in speed
> of the other components in the stack (nginx & rails).

Good to know.  Thanks for reporting back.

> I noticed on my computer applying the patch breaks
> test_rack_lint_big_put in the unicorn test suite.  This may be just my
> issue as the test suite does not run cleanly anyways if I checkout
> origin/master.

The test suite in master should be passing cleanly, at least on a
GNU/Linux machine...

Yes, this hacky patch breaks some tests/internals and screws up
exception error/reporting badly.

> 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!

I strongly prefer this to be optional behavior and off-by-default.

Also, I'm nearly certain two write()s is all that's needed and the
each_char is unnecessary syscall/packet overhead.

This will trigger bugs in badly-written HTTP clients/parsers (probably
some test suites :x) which assume a header comes in a single read().

For TCP users, I believe this requires both tcp_nodelay:false and
tcp_nopush:false to be completely reliable, so we need to enforce that
those if this option is in effect.  The current each_char usage is
probably masking the tcp_nodelay:false requirement.

Thanks again.
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> 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.

Thank you for the patch and I'm looking forward to hearing about how it
works for you guys.  I'd love to hear feedback from anybody else who is
trying this, too.

> 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.

Everything looks good for stock configurations.  The ERROR_XXX_RESPONSE
constants should have "HTTP/1.1 " removed if used with this option, too.
That requires keeping track of whether or not we've written out
"HTTP/1.1 ", yet.

(I don't think there's much benefit in using constants for rarely-used
 error responses, either.  Most of those were inherited from Mongrel
 which had many client-triggered errors to deal with)

> 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.

It should probably be done inside Unicorn::Configurator#commit!

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

"git apply" runs cleanly on this message :)

You'll probably find it more reliable and easier to use "git send-email".
The git-send-email manpage has an example for using it with
"git format-patch" + GMail SMTP.  Using "git format-patch" also lets
you preserve your original commit message/authorship information.
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

On Fri, Nov 2, 2012 at 3:38 PM, Eric Wong <normalperson@yhbt.net> wrote:
> Everything looks good for stock configurations.  The ERROR_XXX_RESPONSE
> constants should have "HTTP/1.1 " removed if used with this option, too.
> That requires keeping track of whether or not we've written out
> "HTTP/1.1 ", yet.

The patch below tracks if we've sent the start of the response.  It
also passes that to http_response_write as a parameter with default
value false.  By doing this, the test suite passes if you change the
default value for check_client_connection to true, which makes me feel
a lot better about the patch.

> > 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.
>
> It should probably be done inside Unicorn::Configurator#commit!

So, I still didn't so this, because in commit! we don't know if it's a
TCP or UNIX socket, and I don't think it'd be nice to force tcp
configs if the user's using a UNIX socket.  If you disagree I can add
the check there, it just seemed weird when I looked at it.

>
> You'll probably find it more reliable and easier to use "git send-email".

Sadly with 2-factor authentication I think this is a no go.

Let me know what you think!

Cheers,
Tom

>From d7e249202b0fd5c24f2b98c700e02bf992c6576b Mon Sep 17 00:00:00 2001
From: Tom Burns <tom.burns@jadedpixel.com>
Date: Tue, 30 Oct 2012 16:22:21 -0400
Subject: [PATCH] Begin writing HTTP request headers early to detect
 disconnected clients

---
 examples/unicorn.conf.rb       |    6 ++++++
 lib/unicorn/configurator.rb    |    7 +++++++
 lib/unicorn/const.rb           |   12 ++++++++----
 lib/unicorn/http_request.rb    |   21 +++++++++++++++++++++
 lib/unicorn/http_response.rb   |    5 +++--
 lib/unicorn/http_server.rb     |   23 +++++++++++++++++++++--
 test/exec/test_exec.rb         |    7 ++++++-
 test/unit/test_configurator.rb |   12 ++++++++++++
 8 files changed, 84 insertions(+), 9 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..f0c4c12 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -29,12 +29,16 @@ module Unicorn::Const

   # :stopdoc:
   # common errors we'll send back
-  ERROR_400_RESPONSE = "HTTP/1.1 400 Bad Request\r\n\r\n"
-  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"
+  ERROR_400_RESPONSE = "400 Bad Request\r\n\r\n"
+  ERROR_414_RESPONSE = "414 Request-URI Too Long\r\n\r\n"
+  ERROR_413_RESPONSE = "413 Request Entity Too Large\r\n\r\n"
+  ERROR_500_RESPONSE = "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..9aceb0e 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -22,11 +22,14 @@ class Unicorn::HttpParser

   NULL_IO = StringIO.new("")

+  attr_accessor :response_start_sent
+
   # :stopdoc:
   # A frozen format for this is about 15% faster
   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 +38,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 +82,15 @@ 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?
+      @response_start_sent = true
+      Unicorn::Const::HTTP_RESPONSE_START.each { |c| socket.write(c) }
+    else
+      @response_start_sent = false
+    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..9c2bacc 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -18,11 +18,12 @@ module Unicorn::HttpResponse
   CRLF = "\r\n"

   # writes the rack_response to socket as an HTTP response
-  def http_response_write(socket, status, headers, body)
+  def http_response_write(socket, status, headers, body,
response_start_sent=false)
     status = CODES[status.to_i] || status

+    http_response_start = response_start_sent ? '' : 'HTTP/1.1 '
     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..abe27db 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
@@ -524,23 +533,33 @@ class Unicorn::HttpServer
       Unicorn.log_error(@logger, "app error", e)
       Unicorn::Const::ERROR_500_RESPONSE
     end
+    msg = 'HTTP/1.1 ' + msg unless @request.response_start_sent
     client.kgio_trywrite(msg)
     client.close
     rescue
   end

+  def expect_100_response
+    if @request.response_start_sent
+      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)
+    @request.response_start_sent = false
     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
     @request.headers? or headers = nil
-    http_response_write(client, status, headers, body)
+    http_response_write(client, status, headers, body,
@request.response_start_sent)
     client.shutdown # in case of fork() in Rack app
     client.close # flush and uncork socket immediately, no keepalive
   rescue => e
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> On Fri, Nov 2, 2012 at 3:38 PM, Eric Wong <normalperson@yhbt.net> wrote:
> > Everything looks good for stock configurations.  The ERROR_XXX_RESPONSE
> > constants should have "HTTP/1.1 " removed if used with this option, too.
> > That requires keeping track of whether or not we've written out
> > "HTTP/1.1 ", yet.
> 
> The patch below tracks if we've sent the start of the response.  It
> also passes that to http_response_write as a parameter with default
> value false.  By doing this, the test suite passes if you change the
> default value for check_client_connection to true, which makes me feel
> a lot better about the patch.

Cool.

> > > 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.
> >
> > It should probably be done inside Unicorn::Configurator#commit!
> 
> So, I still didn't so this, because in commit! we don't know if it's a
> TCP or UNIX socket, and I don't think it'd be nice to force tcp
> configs if the user's using a UNIX socket.  If you disagree I can add
> the check there, it just seemed weird when I looked at it.

Erm, just check if :tcp_* is set in the :listener_opts hash and raise if
there's incompatible settings.  We shouldn't be changing settings behind
users' backs.

> >From d7e249202b0fd5c24f2b98c700e02bf992c6576b Mon Sep 17 00:00:00 2001
> From: Tom Burns <tom.burns@jadedpixel.com>
> Date: Tue, 30 Oct 2012 16:22:21 -0400
> Subject: [PATCH] Begin writing HTTP request headers early to detect
>  disconnected clients

This definitely needs a thorough commit message which explains why this
is a necessary addition, how it works, when it works, and what benefits
it brings.

We can wait until you can report on how this change improves your
situation before fleshing this out.  Real-world results are always
good to have :>

> --- a/lib/unicorn/http_request.rb
> +++ b/lib/unicorn/http_request.rb
> @@ -70,6 +82,15 @@ 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?
> +      @response_start_sent = true
> +      Unicorn::Const::HTTP_RESPONSE_START.each { |c| socket.write(c) }
> +    else
> +      @response_start_sent = false

This else portion is redundant given it's set to false when you reset
the parser (see my final comment below)

> --- a/lib/unicorn/http_server.rb
> +++ b/lib/unicorn/http_server.rb
> @@ -524,23 +533,33 @@ class Unicorn::HttpServer
>        Unicorn.log_error(@logger, "app error", e)
>        Unicorn::Const::ERROR_500_RESPONSE
>      end
> +    msg = 'HTTP/1.1 ' + msg unless @request.response_start_sent

String interpolation would be easier to read, here (and slightly
smaller bytecode if it matters :>).

>      client.kgio_trywrite(msg)
>      client.close
>      rescue
>    end
> 
> +  def expect_100_response
> +    if @request.response_start_sent
> +      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)
> +    @request.response_start_sent = false

Better to reset this ivar in the HttpParser#reset method
(it's in the C extension, I can change it later).

Random thought:  HttpResponse generation gains even more coupling
with the current Http{Request,Parser} state.  Organizing code is hard :x
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson@yhbt.net> wrote:
> We can wait until you can report on how this change improves your
> situation before fleshing this out.  Real-world results are always
> good to have :>
Agreed.

We're going to be testing this this week so I should have some results
to share by the weekend.  We only experience the problem during major
sales so it may or may not manifest without our traffic generation
tool.

I'd like to be testing a patch close to the finished product so I've
addressed all of your comments from the previous email (including the
modification to the C extension) in the patch below.

>
> Random thought:  HttpResponse generation gains even more coupling
> with the current Http{Request,Parser} state.  Organizing code is hard :x
Agreed.  Without a class wrapping the raw socket to hold state I
didn't see a better way to accomplish this, but I've only been looking
at this code for the past week or so.

Cheers,
Tom

---
>From c696bc0cb4df223a096142b6f314c8c2d83255be Mon Sep 17 00:00:00 2001
From: Tom Burns <tom.burns@jadedpixel.com>
Date: Tue, 30 Oct 2012 16:22:21 -0400
Subject: [PATCH] Begin writing HTTP request headers early to detect
 disconnected clients

This patch checks incoming connections and avoids calling the application
if the connection has been closed.

It works by sending the beginning of the HTTP response before calling
the application to see if the socket can successfully be written to.

By enabling this feature users can avoid wasting application rendering
time only to find the connection is closed when attempting to write, and
throwing out the result.

When a client disconnects while being queued or processed, Nginx will log
HTTP response 499 but the application will log a 200.

Enabling this feature will minimize the time window during which the problem
can arise.

The feature is disabled by default and can be enabled by adding
'check_client_connection true' to the unicorn config.
---
 examples/unicorn.conf.rb         |    6 ++++++
 ext/unicorn_http/unicorn_http.rl |    4 +++-
 lib/unicorn/configurator.rb      |   14 ++++++++++++++
 lib/unicorn/const.rb             |   12 ++++++++----
 lib/unicorn/http_request.rb      |   19 +++++++++++++++++++
 lib/unicorn/http_response.rb     |    5 +++--
 lib/unicorn/http_server.rb       |   22 ++++++++++++++++++++--
 test/exec/test_exec.rb           |    7 ++++++-
 test/unit/test_configurator.rb   |   24 ++++++++++++++++++++++++
 9 files changed, 103 insertions(+), 10 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/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index 96dcf83..52f7f3c 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -115,7 +115,7 @@ struct http_parser {
   } len;
 };

-static ID id_clear, id_set_backtrace;
+static ID id_clear, id_set_backtrace, id_response_start_sent;

 static void finalize_header(struct http_parser *hp);

@@ -626,6 +626,7 @@ static VALUE HttpParser_clear(VALUE self)

   http_parser_init(hp);
   rb_funcall(hp->env, id_clear, 0);
+  rb_funcall(self, id_response_start_sent, 1, Qfalse);

   return self;
 }
@@ -1031,6 +1032,7 @@ void Init_unicorn_http(void)
   SET_GLOBAL(g_http_connection, "CONNECTION");
   id_clear = rb_intern("clear");
   id_set_backtrace = rb_intern("set_backtrace");
+  id_response_start_sent = rb_intern("response_start_sent=");
   init_unicorn_httpdate();
 }
 #undef SET_GLOBAL
diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 89cbf5c..5f329af 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,
@@ -96,6 +97,13 @@ class Unicorn::Configurator
     if ready_pipe = RACKUP.delete(:ready_pipe)
       server.ready_pipe = ready_pipe
     end
+    if set[:check_client_connection]
+      set[:listeners].each do |address|
+        if set[:listener_opts][address][:tcp_nopush] == true ||
set[:listener_opts][address][:tcp_nodelay] == true
+          raise ArgumentError, "With check_client_connection set to
true both :tcp_nopush and :tcp_nodelay listener options must be set to
false."
+        end
+      end
+    end
     set.each do |key, value|
       value == :unset and next
       skip.include?(key) and next
@@ -454,6 +462,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..f0c4c12 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -29,12 +29,16 @@ module Unicorn::Const

   # :stopdoc:
   # common errors we'll send back
-  ERROR_400_RESPONSE = "HTTP/1.1 400 Bad Request\r\n\r\n"
-  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"
+  ERROR_400_RESPONSE = "400 Bad Request\r\n\r\n"
+  ERROR_414_RESPONSE = "414 Request-URI Too Long\r\n\r\n"
+  ERROR_413_RESPONSE = "413 Request Entity Too Large\r\n\r\n"
+  ERROR_500_RESPONSE = "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..79ead2e 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -22,11 +22,14 @@ class Unicorn::HttpParser

   NULL_IO = StringIO.new("")

+  attr_accessor :response_start_sent
+
   # :stopdoc:
   # A frozen format for this is about 15% faster
   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 +38,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 +82,13 @@ 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?
+      @response_start_sent = true
+      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..9c2bacc 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -18,11 +18,12 @@ module Unicorn::HttpResponse
   CRLF = "\r\n"

   # writes the rack_response to socket as an HTTP response
-  def http_response_write(socket, status, headers, body)
+  def http_response_write(socket, status, headers, body,
response_start_sent=false)
     status = CODES[status.to_i] || status

+    http_response_start = response_start_sent ? '' : 'HTTP/1.1 '
     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..8729830 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
@@ -524,23 +533,32 @@ class Unicorn::HttpServer
       Unicorn.log_error(@logger, "app error", e)
       Unicorn::Const::ERROR_500_RESPONSE
     end
+    msg = "HTTP/1.1 #{msg}" unless @request.response_start_sent
     client.kgio_trywrite(msg)
     client.close
     rescue
   end

+  def expect_100_response
+    if @request.response_start_sent
+      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
     @request.headers? or headers = nil
-    http_response_write(client, status, headers, body)
+    http_response_write(client, status, headers, body,
@request.response_start_sent)
     client.shutdown # in case of fork() in Rack app
     client.close # flush and uncork socket immediately, no keepalive
   rescue => e
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..b9c22d7 100644
--- a/test/unit/test_configurator.rb
+++ b/test/unit/test_configurator.rb
@@ -139,6 +139,30 @@ 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_check_client_connection_with_tcp_bad
+    tmp = Tempfile.new('unicorn_config')
+    test_struct = TestStruct.new
+    listener = "127.0.0.1:12345"
+    tmp.syswrite("check_client_connection true\n")
+    tmp.syswrite("listen '#{listener}', :tcp_nopush => true\n")
+
+    assert_raises(ArgumentError) do
+      Unicorn::Configurator.new(:config_file => tmp.path).commit!(test_struct)
+    end
+  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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson@yhbt.net> wrote:
> > We can wait until you can report on how this change improves your
> > situation before fleshing this out.  Real-world results are always
> > good to have :>
> 
> Agreed.
> 
> We're going to be testing this this week so I should have some results
> to share by the weekend.  We only experience the problem during major
> sales so it may or may not manifest without our traffic generation
> tool.

Holidays are coming up, should be lots of traffic :)

> I'd like to be testing a patch close to the finished product so I've
> addressed all of your comments from the previous email (including the
> modification to the C extension) in the patch below.

Thanks for the updated patch.  A few comments below, apologies for the
nitpicks :)

> > Random thought:  HttpResponse generation gains even more coupling
> > with the current Http{Request,Parser} state.  Organizing code is hard :x
> Agreed.  Without a class wrapping the raw socket to hold state I
> didn't see a better way to accomplish this, but I've only been looking
> at this code for the past week or so.

I'm leaning towards just having one HttpState class which encompasses
the entire client state (request/response).  I did that earlier this
year for a single-purpose (non-Ruby) HTTP daemon and that design makes
the most sense to me.

Maybe we'll modify unicorn around that sooner or later...

> diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
> index 96dcf83..52f7f3c 100644
> --- a/ext/unicorn_http/unicorn_http.rl
> +++ b/ext/unicorn_http/unicorn_http.rl
> @@ -115,7 +115,7 @@ struct http_parser {
>    } len;
>  };
> 
> -static ID id_clear, id_set_backtrace;
> +static ID id_clear, id_set_backtrace, id_response_start_sent;
> 
>  static void finalize_header(struct http_parser *hp);
> 
> @@ -626,6 +626,7 @@ static VALUE HttpParser_clear(VALUE self)
> 
>    http_parser_init(hp);
>    rb_funcall(hp->env, id_clear, 0);
> +  rb_funcall(self, id_response_start_sent, 1, Qfalse);

Nitpick:  Since HttpParser is already an alias of the HttpRequest class,
rb_ivar_set() should be a hair faster as it won't have to go through
normal method lookup.

>    return self;
>  }
> @@ -1031,6 +1032,7 @@ void Init_unicorn_http(void)
>    SET_GLOBAL(g_http_connection, "CONNECTION");
>    id_clear = rb_intern("clear");
>    id_set_backtrace = rb_intern("set_backtrace");
> +  id_response_start_sent = rb_intern("response_start_sent=");
>    init_unicorn_httpdate();
>  }
>  #undef SET_GLOBAL
> diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
> index 89cbf5c..5f329af 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,
> @@ -96,6 +97,13 @@ class Unicorn::Configurator
>      if ready_pipe = RACKUP.delete(:ready_pipe)
>        server.ready_pipe = ready_pipe
>      end
> +    if set[:check_client_connection]
> +      set[:listeners].each do |address|
> +        if set[:listener_opts][address][:tcp_nopush] == true ||
> set[:listener_opts][address][:tcp_nodelay] == true

Oops, I missed long lines the first time.  It looks like your MUA did
mangle the long line.  Wrap everything at <80 columns should help avoid
the issue (regardless of MUA, my brain cannot process >80 columns well)

> +          raise ArgumentError, "With check_client_connection set to
> true both :tcp_nopush and :tcp_nodelay listener options must be set to
> false."

Likewise, error messages should be more concise and easier to read
in smaller terminals.

Probably:

check_client_connection is incompatible with (tcp_nopush|tcp_nodelay):true

> @@ -454,6 +462,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.

Good to document the :tcp_* listener option incompatibilities here, too.

> +  def check_client_connection(bool)
> +    set_bool(:check_client_connection, bool)
> +  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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

On Tue, Nov 6, 2012 at 4:23 PM, Eric Wong <normalperson@yhbt.net> wrote:
>
> Tom Burns <tom.burns@jadedpixel.com> wrote:
> > On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson@yhbt.net> wrote:
> > > We can wait until you can report on how this change improves your
> > > situation before fleshing this out.  Real-world results are always
> > > good to have :>
> >
> > Agreed.
> >
> > We're going to be testing this this week so I should have some results
> > to share by the weekend.  We only experience the problem during major
> > sales so it may or may not manifest without our traffic generation
> > tool.
>
> Holidays are coming up, should be lots of traffic :)

So we just finished the US Black Friday / Cyber Monday weekend running
unicorn forked with the last version of the patch I had sent you.  It
worked splendidly and helped us handle huge flash sales without
increased response time over the weekend.

Whereas in previous flash traffic scenarios we would see the number of
HTTP 499 responses grow past the number of real HTTP 200 responses,
over the weekend we saw no growth in 499s during flash sales.

Unexpectedly the patch also helped us ward off a DoS attack where the
attackers were disconnecting immediately after making a request.

I've attached the patch again, with your last comments addressed.  Let
me know if there's anything else.

Thanks again for your help in this matter.

Cheers,
Tom

>From 89c8c51355c75b0196469812580443fba390632e Mon Sep 17 00:00:00 2001
From: Tom Burns <tom.burns@jadedpixel.com>
Date: Tue, 30 Oct 2012 16:22:21 -0400
Subject: [PATCH] Begin writing HTTP request headers early to detect
 disconnected clients

This patch checks incoming connections and avoids calling the application
if the connection has been closed.

It works by sending the beginning of the HTTP response before calling
the application to see if the socket can successfully be written to.

By enabling this feature users can avoid wasting application rendering
time only to find the connection is closed when attempting to write, and
throwing out the result.

When a client disconnects while being queued or processed, Nginx will log
HTTP response 499 but the application will log a 200.

Enabling this feature will minimize the time window during which the problem
can arise.

The feature is disabled by default and can be enabled by adding
'check_client_connection true' to the unicorn config.
---
 examples/unicorn.conf.rb         |    6 ++++++
 ext/unicorn_http/unicorn_http.rl |    4 +++-
 lib/unicorn/configurator.rb      |   25 +++++++++++++++++++++++++
 lib/unicorn/const.rb             |   12 ++++++++----
 lib/unicorn/http_request.rb      |   19 +++++++++++++++++++
 lib/unicorn/http_response.rb     |    5 +++--
 lib/unicorn/http_server.rb       |   22 ++++++++++++++++++++--
 test/exec/test_exec.rb           |    7 ++++++-
 test/unit/test_configurator.rb   |   24 ++++++++++++++++++++++++
 9 files changed, 114 insertions(+), 10 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/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index 96dcf83..1a8003f 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -115,7 +115,7 @@ struct http_parser {
   } len;
 };

-static ID id_clear, id_set_backtrace;
+static ID id_clear, id_set_backtrace, id_response_start_sent;

 static void finalize_header(struct http_parser *hp);

@@ -626,6 +626,7 @@ static VALUE HttpParser_clear(VALUE self)

   http_parser_init(hp);
   rb_funcall(hp->env, id_clear, 0);
+  rb_ivar_set(self, id_response_start_sent, Qfalse);

   return self;
 }
@@ -1031,6 +1032,7 @@ void Init_unicorn_http(void)
   SET_GLOBAL(g_http_connection, "CONNECTION");
   id_clear = rb_intern("clear");
   id_set_backtrace = rb_intern("set_backtrace");
+  id_response_start_sent = rb_intern("@response_start_sent");
   init_unicorn_httpdate();
 }
 #undef SET_GLOBAL
diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 89cbf5c..9752cdd 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,
@@ -96,6 +97,18 @@ class Unicorn::Configurator
     if ready_pipe = RACKUP.delete(:ready_pipe)
       server.ready_pipe = ready_pipe
     end
+    if set[:check_client_connection]
+      set[:listeners].each do |address|
+        if set[:listener_opts][address][:tcp_nopush] == true
+          raise ArgumentError,
+            "check_client_connection is incompatible with tcp_nopush:true"
+        end
+        if set[:listener_opts][address][:tcp_nodelay] == true
+          raise ArgumentError,
+            "check_client_connection is incompatible with tcp_nodelay:true"
+        end
+      end
+    end
     set.each do |key, value|
       value == :unset and next
       skip.include?(key) and next
@@ -454,6 +467,18 @@ 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.
+  #
+  # This will prevent calling the application for clients who have
+  # disconnected while their connection was queued.
+  #
+  # This option cannot be used in conjunction with tcp_nodelay or
+  # tcp_nopush.
+  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..f0c4c12 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -29,12 +29,16 @@ module Unicorn::Const

   # :stopdoc:
   # common errors we'll send back
-  ERROR_400_RESPONSE = "HTTP/1.1 400 Bad Request\r\n\r\n"
-  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"
+  ERROR_400_RESPONSE = "400 Bad Request\r\n\r\n"
+  ERROR_414_RESPONSE = "414 Request-URI Too Long\r\n\r\n"
+  ERROR_413_RESPONSE = "413 Request Entity Too Large\r\n\r\n"
+  ERROR_500_RESPONSE = "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..79ead2e 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -22,11 +22,14 @@ class Unicorn::HttpParser

   NULL_IO = StringIO.new("")

+  attr_accessor :response_start_sent
+
   # :stopdoc:
   # A frozen format for this is about 15% faster
   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 +38,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 +82,13 @@ 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?
+      @response_start_sent = true
+      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..9c2bacc 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -18,11 +18,12 @@ module Unicorn::HttpResponse
   CRLF = "\r\n"

   # writes the rack_response to socket as an HTTP response
-  def http_response_write(socket, status, headers, body)
+  def http_response_write(socket, status, headers, body,
response_start_sent=false)
     status = CODES[status.to_i] || status

+    http_response_start = response_start_sent ? '' : 'HTTP/1.1 '
     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..8729830 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
@@ -524,23 +533,32 @@ class Unicorn::HttpServer
       Unicorn.log_error(@logger, "app error", e)
       Unicorn::Const::ERROR_500_RESPONSE
     end
+    msg = "HTTP/1.1 #{msg}" unless @request.response_start_sent
     client.kgio_trywrite(msg)
     client.close
     rescue
   end

+  def expect_100_response
+    if @request.response_start_sent
+      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
     @request.headers? or headers = nil
-    http_response_write(client, status, headers, body)
+    http_response_write(client, status, headers, body,
@request.response_start_sent)
     client.shutdown # in case of fork() in Rack app
     client.close # flush and uncork socket immediately, no keepalive
   rescue => e
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..b9c22d7 100644
--- a/test/unit/test_configurator.rb
+++ b/test/unit/test_configurator.rb
@@ -139,6 +139,30 @@ 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_check_client_connection_with_tcp_bad
+    tmp = Tempfile.new('unicorn_config')
+    test_struct = TestStruct.new
+    listener = "127.0.0.1:12345"
+    tmp.syswrite("check_client_connection true\n")
+    tmp.syswrite("listen '#{listener}', :tcp_nopush => true\n")
+
+    assert_raises(ArgumentError) do
+      Unicorn::Configurator.new(:config_file => tmp.path).commit!(test_struct)
+    end
+  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.10.2 (Apple Git-33)
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Lawrence Pit]

> [2] 
> http://mid.gmane.org/CAK4qKG32dGgNbzTzCb6NoH5hu_DrHMOFaFhk-6Xvy-T86++ukA@mail.gmail.com
> I haven't heard back on results of our nasty/crazy solution, though.

fyi: I've been running that patch for a while in our staging 
environment. As far as I can tell it works. Though we haven't tested it 
with older browsers such as IE6 (we do not have that requirement) nor 
with all of the API clients we see (java, python, etc., we do have that 
requirement).

Hopefully we're bringing that patch into our production environment 
within the next couple of weeks, and then we should see results later 
that week.

Tom Burns wrote:
> Unexpectedly the patch also helped us ward off a DoS attack where the
> attackers were disconnecting immediately after making a request.

Awesome!

The only thing that worries me is that Eric calls this a "nasty/crazy 
solution". :p Not sure why. Seems to me it's a rather smart solution 
that perhaps in the future would deserve to be the default.

A related question: is it possible to get insight in what's in the 
unicorn queue and for how long requests have been queued there?


Cheers,
Lawrence

_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Tom Burns]

On Thu, Nov 29, 2012 at 3:41 PM, Eric Wong <normalperson@yhbt.net> wrote:
> I fixed up some minor line-wrapping, signed-off, and added your
> quote above to the commit message.  Pushed as
> commit 5c700fc2cf398848ddcf71a2aa3f0f2a6563e87b
> to git://bogomips.org/unicorn.git
>
> I'll tag and push a 4.5.0.preview1 gem soon

Excellent news :)

On Thu, Nov 29, 2012 at 3:30 PM, Lawrence Pit <lawrence.pit@gmail.com> wrote:
> we haven't tested it with older browsers
> such as IE6 (we do not have that requirement) nor with all of the API
> clients we see (java, python, etc., we do have that requirement).

With nginx and proxy_pass you can buffer the entire request/response
from the rails application, so clients will not notice any difference.

>
> Hopefully we're bringing that patch into our production environment within
> the next couple of weeks, and then we should see results later that week.
>
>
> Tom Burns wrote:
>>
>> Unexpectedly the patch also helped us ward off a DoS attack where the
>> attackers were disconnecting immediately after making a request.
>
>
> Awesome!
>
> The only thing that worries me is that Eric calls this a "nasty/crazy
> solution". :p Not sure why. Seems to me it's a rather smart solution that
> perhaps in the future would deserve to be the default.
>
> A related question: is it possible to get insight in what's in the unicorn
> queue and for how long requests have been queued there?

We use New Relic for response queue monitoring.

https://newrelic.com/docs/features/tracking-front-end-time

Cheers,
Tom
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Lawrence Pit <lawrence.pit@gmail.com> wrote:
> >[2] http://mid.gmane.org/CAK4qKG32dGgNbzTzCb6NoH5hu_DrHMOFaFhk-6Xvy-T86++ukA@mail.gmail.com
> >I haven't heard back on results of our nasty/crazy solution, though.
> 
> fyi: I've been running that patch for a while in our staging
> environment. As far as I can tell it works. Though we haven't tested
> it with older browsers such as IE6 (we do not have that requirement)
> nor with all of the API clients we see (java, python, etc., we do
> have that requirement).
> 
> Hopefully we're bringing that patch into our production environment
> within the next couple of weeks, and then we should see results
> later that week.
> 
> Tom Burns wrote:
> >Unexpectedly the patch also helped us ward off a DoS attack where the
> >attackers were disconnecting immediately after making a request.
> 
> Awesome!
> 
> The only thing that worries me is that Eric calls this a
> "nasty/crazy solution". :p Not sure why. Seems to me it's a rather
> smart solution that perhaps in the future would deserve to be the
> default.

Until now, it was unproven.  AFAIK, no other app server does
anything similar.

Maybe other servers/protocols have similar things, I'd love to know.

> A related question: is it possible to get insight in what's in the
> unicorn queue and for how long requests have been queued there?

On Linux, tcpi_last_data_recv in the structure returned by TCP_INFO
tells you when the last packet was received on the socket, so it's an
accurate indication of queue time.  Raindrops provides an interface for
accessing it:

  http://raindrops.bogomips.org/Raindrops/LastDataRecv.html

I haven't heard much feedback about it, though.

Using SystemTap (or similar solutions), I think the same information may
be retrieved without modifying the Ruby process.  I'm just starting my
exploration of SystemTap, though...

Raindrops::Watcher is less accurate/informative, but can run without
modifying the process.  I've used this heavily, but haven't
found anything interesting since I don't get enough traffic.

  http://raindrops.bogomips.org/Raindrops/Watcher.html
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Lawrence Pit <lawrence.pit@gmail.com> wrote:
> > A related question: is it possible to get insight in what's in the
> > unicorn queue and for how long requests have been queued there?

<snip>

> Using SystemTap (or similar solutions), I think the same information may
> be retrieved without modifying the Ruby process.  I'm just starting my
> exploration of SystemTap, though...

With a little help from a SystemTap maintainer, I wrote a script
to get the TCP queue times easily.

The UNIX domain socket one is still pretty fragile (comments inline) but
appears to work:

http://mid.gmane.org/20121130223518.GA13976@dcvr.yhbt.net

If you're using an older SystemTap, you may need to swap cpu_clock_*(0)
with gettimeofday_*().
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> So we just finished the US Black Friday / Cyber Monday weekend running
> unicorn forked with the last version of the patch I had sent you.  It
> worked splendidly and helped us handle huge flash sales without
> increased response time over the weekend.
> 
> Whereas in previous flash traffic scenarios we would see the number of
> HTTP 499 responses grow past the number of real HTTP 200 responses,
> over the weekend we saw no growth in 499s during flash sales.
> 
> Unexpectedly the patch also helped us ward off a DoS attack where the
> attackers were disconnecting immediately after making a request.

That is absolutely wonderful to hear.  I've amended your commit message
with the above quoted portion.

> I've attached the patch again, with your last comments addressed.  Let
> me know if there's anything else.
> 
> Thanks again for your help in this matter.

Thank _you_ for the patch, documentation and most importantly:
testing with real traffic.

I fixed up some minor line-wrapping, signed-off, and added your
quote above to the commit message.  Pushed as
commit 5c700fc2cf398848ddcf71a2aa3f0f2a6563e87b
to git://bogomips.org/unicorn.git

I'll tag and push a 4.5.0.preview1 gem soon
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> I fixed up some minor line-wrapping, signed-off, and added your
> quote above to the commit message.  Pushed as
> commit 5c700fc2cf398848ddcf71a2aa3f0f2a6563e87b
> to git://bogomips.org/unicorn.git

One more fix/cleanup to maintain compatibility with Rainbows!

>From 69e6a793d34ff71da7c8ca59962d627e2fb508d8 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Tue, 4 Dec 2012 02:35:26 +0000
Subject: [PATCH] fix const error responses for Rainbows!

Rainbows! relies on the ERROR_XXX_RESPONSE constants of unicorn
4.x.  Changing the constants in unicorn 4.x will break existing
versions of Rainbows!, so remove the dependency on the constants
and generate the error response dynamically.

Unlike Mongrel, unicorn is unlikely to see malicious traffic and
thus unlikely to benefit from making error messages constant.

For unicorn 5.x, we will drop these constants entirely.

(Rainbows! most likely cannot support check_client_connection
 consistently across all concurrency models since some of them
 pessimistically buffer all writes in userspace.  However, the
 extra concurrency of Rainbows! makes it less likely to be
 overloaded than unicorn, so this feature is likely less useful
 for Rainbows!)
---
 lib/unicorn/const.rb         | 10 ++++++----
 lib/unicorn/http_response.rb |  4 ++++
 lib/unicorn/http_server.rb   | 15 +++++++--------
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/lib/unicorn/const.rb b/lib/unicorn/const.rb
index 60a63b1..02e29c7 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -29,10 +29,12 @@ module Unicorn::Const
 
   # :stopdoc:
   # common errors we'll send back
-  ERROR_400_RESPONSE = "400 Bad Request\r\n\r\n"
-  ERROR_414_RESPONSE = "414 Request-URI Too Long\r\n\r\n"
-  ERROR_413_RESPONSE = "413 Request Entity Too Large\r\n\r\n"
-  ERROR_500_RESPONSE = "500 Internal Server Error\r\n\r\n"
+  # (N.B. these are not used by unicorn, but we won't drop them until
+  #  unicorn 5.x to avoid breaking Rainbows!).
+  ERROR_400_RESPONSE = "HTTP/1.1 400 Bad Request\r\n\r\n"
+  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 "
diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index 61563cd..579d957 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -17,6 +17,10 @@ module Unicorn::HttpResponse
   }
   CRLF = "\r\n"
 
+  def err_response(code, response_start_sent)
+    "#{response_start_sent ? '' : 'HTTP/1.1 '}#{CODES[code]}\r\n\r\n"
+  end
+
   # writes the rack_response to socket as an HTTP response
   def http_response_write(socket, status, headers, body,
                           response_start_sent=false)
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index ef1ea58..aa98aeb 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -519,22 +519,21 @@ class Unicorn::HttpServer
   # 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)
-    msg = case e
+    code = case e
     when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF,
          Errno::ENOTCONN
-      Unicorn::Const::ERROR_500_RESPONSE
+      500
     when Unicorn::RequestURITooLongError
-      Unicorn::Const::ERROR_414_RESPONSE
+      414
     when Unicorn::RequestEntityTooLargeError
-      Unicorn::Const::ERROR_413_RESPONSE
+      413
     when Unicorn::HttpParserError # try to tell the client they're bad
-      Unicorn::Const::ERROR_400_RESPONSE
+      400
     else
       Unicorn.log_error(@logger, "app error", e)
-      Unicorn::Const::ERROR_500_RESPONSE
+      500
     end
-    msg = "HTTP/1.1 #{msg}" unless @request.response_start_sent
-    client.kgio_trywrite(msg)
+    client.kgio_trywrite(err_response(code, @request.response_start_sent))
     client.close
     rescue
   end
-- 
Eric Wong
_______________________________________________
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

Re: Combating nginx 499 HTTP responses during flash traffic scenario

[Eric Wong]

Tom Burns <tom.burns@jadedpixel.com> wrote:
> +    if set[:check_client_connection]
> +      set[:listeners].each do |address|
> +        if set[:listener_opts][address][:tcp_nopush] == true
> +          raise ArgumentError,
> +            "check_client_connection is incompatible with tcp_nopush:true"
> +        end

Btw, were you using:

1) TCP over loopback (bound to 0.0.0.0, client comes from 127.0.0.1)
2) TCP over a LAN (separate client/server hosts)
3) Unix domain socket

I wonder if we can drop the below hunk for checking :tcp_nodelay,
and document that check_client_connection requires the client to
either be from a Unix domain socket or TCP loopback to work.

> +        if set[:listener_opts][address][:tcp_nodelay] == true
> +          raise ArgumentError,
> +            "check_client_connection is incompatible with tcp_nodelay:true"
> +        end

I couldn't get 2) to work with either value of tcp_nodelay.  My small
LAN at home only has ~0.100ms latency.

Happily, with TCP over loopback (on Linux 3.6), either value of
tcp_nodelay works, so the tcp_nodelay check seems unnecessary after
all.
_______________________________________________
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

[RFC/PATCH] check_client_connection: document local-only requirement

[Eric Wong]

In my testing, only dropped clients over Unix domain sockets or
loopback TCP were detected with this option.  Since many
nginx+unicorn combinations run on the same host, this is not a
problem.

Furthermore, tcp_nodelay:true appears to work over loopback,
so remove the requirement for tcp_nodelay:false.
---
  Eric Wong <normalperson@yhbt.net> wrote:
  > Tom Burns <tom.burns@jadedpixel.com> wrote:
  > > +    if set[:check_client_connection]
  > > +      set[:listeners].each do |address|
  > > +        if set[:listener_opts][address][:tcp_nopush] == true
  > > +          raise ArgumentError,
  > > +            "check_client_connection is incompatible with tcp_nopush:true"
  > > +        end
  > 
  > Btw, were you using:
  > 
  > 1) TCP over loopback (bound to 0.0.0.0, client comes from 127.0.0.1)
  > 2) TCP over a LAN (separate client/server hosts)
  > 3) Unix domain socket
  > 
  > I wonder if we can drop the below hunk for checking :tcp_nodelay,
  > and document that check_client_connection requires the client to
  > either be from a Unix domain socket or TCP loopback to work.
  > 
  > > +        if set[:listener_opts][address][:tcp_nodelay] == true
  > > +          raise ArgumentError,
  > > +            "check_client_connection is incompatible with tcp_nodelay:true"
  > > +        end
  > 
  > I couldn't get 2) to work with either value of tcp_nodelay.  My small
  > LAN at home only has ~0.100ms latency.
  > 
  > Happily, with TCP over loopback (on Linux 3.6), either value of
  > tcp_nodelay works, so the tcp_nodelay check seems unnecessary after
  > all.

 lib/unicorn/configurator.rb | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 9752cdd..7651093 100644
--- a/lib/unicorn/configurator.rb
+++ b/lib/unicorn/configurator.rb
@@ -103,10 +103,6 @@ class Unicorn::Configurator
           raise ArgumentError,
             "check_client_connection is incompatible with tcp_nopush:true"
         end
-        if set[:listener_opts][address][:tcp_nodelay] == true
-          raise ArgumentError,
-            "check_client_connection is incompatible with tcp_nodelay:true"
-        end
       end
     end
     set.each do |key, value|
@@ -473,8 +469,11 @@ class Unicorn::Configurator
   # This will prevent calling the application for clients who have
   # disconnected while their connection was queued.
   #
-  # This option cannot be used in conjunction with tcp_nodelay or
-  # tcp_nopush.
+  # This only affects clients connecting over Unix domain sockets
+  # and TCP via loopback (127.*.*.*).  It is unlikely to detect
+  # disconnects if the client is on a remote host (even on a fast LAN).
+  #
+  # This option cannot be used in conjunction with :tcp_nopush.
   def check_client_connection(bool)
     set_bool(:check_client_connection, bool)
   end
-- 
1.8.0.3.gdd57fab.dirty
_______________________________________________
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

Re: [RFC/PATCH] check_client_connection: document local-only requirement

[Tom Burns]

On Thu, Nov 29, 2012 at 4:55 PM, Eric Wong <normalperson@yhbt.net> wrote:
> In my testing, only dropped clients over Unix domain sockets or
> loopback TCP were detected with this option.  Since many
> nginx+unicorn combinations run on the same host, this is not a
> problem.
>
> Furthermore, tcp_nodelay:true appears to work over loopback,
> so remove the requirement for tcp_nodelay:false.

In production our configuration is same host / UNIX domain socket, so
I don't have any comment on the TCP changes.

It makes sense to me though, and the documentation looks better!

Cheers,
Tom
_______________________________________________
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