Tom Burns
2012-Oct-29 17:44 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
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
Eric Wong
2012-Oct-29 18:45 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at 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.htmlHaproxy 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 :>
Hongli Lai
2012-Oct-29 19:27 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Mon, Oct 29, 2012 at 7:45 PM, Eric Wong <normalperson at 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 at phusion.nl Chamber of commerce no: 08173483 (The Netherlands)
Eric Wong
2012-Oct-29 19:41 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Hongli Lai <hongli at phusion.nl> wrote:> On Mon, Oct 29, 2012 at 7:45 PM, Eric Wong <normalperson at 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.
Hongli Lai
2012-Oct-29 21:06 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Mon, Oct 29, 2012 at 8:41 PM, Eric Wong <normalperson at 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 at phusion.nl Chamber of commerce no: 08173483 (The Netherlands)
Eric Wong
2012-Oct-29 21:53 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Eric Wong <normalperson at yhbt.net> wrote:> Tom Burns <tom.burns at 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"
Tom Burns
2012-Oct-29 22:21 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Mon, Oct 29, 2012 at 5:53 PM, Eric Wong <normalperson at 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
Tom Burns
2012-Oct-30 20:40 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Mon, Oct 29, 2012 at 5:53 PM, Eric Wong <normalperson at 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
Eric Wong
2012-Oct-30 21:37 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at jadedpixel.com> wrote:> On Mon, Oct 29, 2012 at 5:53 PM, Eric Wong <normalperson at 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.
Tom Burns
2012-Nov-02 17:59 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Tue, Oct 30, 2012 at 5:37 PM, Eric Wong <normalperson at yhbt.net> wrote:> > Tom Burns <tom.burns at 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)
Eric Wong
2012-Nov-02 19:38 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at jadedpixel.com> wrote:> On Tue, Oct 30, 2012 at 5:37 PM, Eric Wong <normalperson at yhbt.net> wrote: > > Tom Burns <tom.burns at 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.
Tom Burns
2012-Nov-03 22:45 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Fri, Nov 2, 2012 at 3:38 PM, Eric Wong <normalperson at 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 2001From: Tom Burns <tom.burns at 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)
Eric Wong
2012-Nov-05 11:48 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at jadedpixel.com> wrote:> On Fri, Nov 2, 2012 at 3:38 PM, Eric Wong <normalperson at 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 at jadedpixel.com> > Date: Tue, 30 Oct 2012 16:22:21 -0400 > Subject: [PATCH] Begin writing HTTP request headers early to detect > disconnected clientsThis 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 = falseThis 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_sentString 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 = falseBetter 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
Tom Burns
2012-Nov-06 03:16 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson at 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 :xAgreed. 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 2001From: Tom Burns <tom.burns at 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)
Eric Wong
2012-Nov-06 21:23 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at jadedpixel.com> wrote:> On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson at 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] == trueOops, 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 > +
Tom Burns
2012-Nov-29 15:52 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Tue, Nov 6, 2012 at 4:23 PM, Eric Wong <normalperson at yhbt.net> wrote:> > Tom Burns <tom.burns at jadedpixel.com> wrote: > > On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson at 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 2001From: Tom Burns <tom.burns at 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)
Lawrence Pit
2012-Nov-29 20:30 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
> [2] > http://mid.gmane.org/CAK4qKG32dGgNbzTzCb6NoH5hu_DrHMOFaFhk-6Xvy-T86++ukA at 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
Eric Wong
2012-Nov-29 20:41 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at 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
Tom Burns
2012-Nov-29 20:57 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
On Thu, Nov 29, 2012 at 3:41 PM, Eric Wong <normalperson at 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 soonExcellent news :) On Thu, Nov 29, 2012 at 3:30 PM, Lawrence Pit <lawrence.pit at 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
Eric Wong
2012-Nov-29 21:19 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Tom Burns <tom.burns at 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" > + endBtw, 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" > + endI 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.
Eric Wong
2012-Nov-29 21:30 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Lawrence Pit <lawrence.pit at gmail.com> wrote:> >[2] http://mid.gmane.org/CAK4qKG32dGgNbzTzCb6NoH5hu_DrHMOFaFhk-6Xvy-T86++ukA at 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
Eric Wong
2012-Nov-29 21:55 UTC
[RFC/PATCH] check_client_connection: document local-only requirement
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 at yhbt.net> wrote:
> Tom Burns <tom.burns at 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
Tom Burns
2012-Nov-29 23:47 UTC
[RFC/PATCH] check_client_connection: document local-only requirement
On Thu, Nov 29, 2012 at 4:55 PM, Eric Wong <normalperson at 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
Eric Wong
2012-Nov-30 23:47 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Eric Wong <normalperson at yhbt.net> wrote:> Lawrence Pit <lawrence.pit at 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 at dcvr.yhbt.net If you''re using an older SystemTap, you may need to swap cpu_clock_*(0) with gettimeofday_*().
Eric Wong
2012-Dec-04 03:00 UTC
Combating nginx 499 HTTP responses during flash traffic scenario
Eric Wong <normalperson at 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.gitOne more fix/cleanup to maintain compatibility with Rainbows!>From 69e6a793d34ff71da7c8ca59962d627e2fb508d8 Mon Sep 17 00:00:00 2001From: Eric Wong <normalperson at 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