From: Tom Burns <tom.burns@jadedpixel.com>
To: unicorn list <mongrel-unicorn@rubyforge.org>
Subject: Re: Combating nginx 499 HTTP responses during flash traffic scenario
Date: Sat, 3 Nov 2012 18:45:13 -0400 [thread overview]
Message-ID: <CAK4qKG1AJdsXvTuUT+Q85qTZzfCTLUMDd=d1LzFgfM6THc5G=g@mail.gmail.com> (raw)
In-Reply-To: <20121102193803.GA17916@dcvr.yhbt.net>
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
next prev parent reply other threads:[~2012-11-03 22:45 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-29 17:44 Combating nginx 499 HTTP responses during flash traffic scenario Tom Burns
2012-10-29 18:45 ` Eric Wong
2012-10-29 19:27 ` Hongli Lai
2012-10-29 19:41 ` Eric Wong
2012-10-29 21:06 ` Hongli Lai
2012-10-29 21:53 ` Eric Wong
2012-10-29 22:21 ` Tom Burns
2012-10-30 20:40 ` Tom Burns
2012-10-30 21:37 ` Eric Wong
2012-11-02 17:59 ` Tom Burns
2012-11-02 19:38 ` Eric Wong
2012-11-03 22:45 ` Tom Burns [this message]
2012-11-05 11:48 ` Eric Wong
2012-11-06 3:16 ` Tom Burns
2012-11-06 21:23 ` Eric Wong
2012-11-29 15:52 ` Tom Burns
2012-11-29 20:30 ` Lawrence Pit
2012-11-29 20:57 ` Tom Burns
2012-11-29 21:30 ` Eric Wong
2012-11-30 23:47 ` Eric Wong
2012-11-29 20:41 ` Eric Wong
2012-12-04 3:00 ` Eric Wong
2012-11-29 21:19 ` Eric Wong
2012-11-29 21:55 ` [RFC/PATCH] check_client_connection: document local-only requirement Eric Wong
2012-11-29 23:47 ` Tom Burns
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://yhbt.net/unicorn/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK4qKG1AJdsXvTuUT+Q85qTZzfCTLUMDd=d1LzFgfM6THc5G=g@mail.gmail.com' \
--to=tom.burns@jadedpixel.com \
--cc=mongrel-unicorn@rubyforge.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhbt.net/unicorn.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).