From 9a9bd739e555771cacfd5b3757a251754ef9512b Mon Sep 17 00:00:00 2001 From: Simon Eskildsen Date: Mon, 6 Mar 2017 16:32:02 -0500 Subject: check_client_connection: use tcp state on linux * Use a frozen empty array and a class variable for TCP_Info to avoid garbage. As far as I can tell, this shouldn't result in any garbage on any requests (other than on the first request). * Pass listener socket to #read to only check the client connection on a TCP server. * Short circuit CLOSE_WAIT after ESTABLISHED since in my testing it's the most common state after ESTABLISHED, it makes the numbers un-ordered, though. But comment should make it OK. * Definition of of `check_client_connection` based on whether Raindrops::TCP_Info is defined, instead of the class variable approach. * Changed the unit tests to pass a `nil` listener. Tested on our staging environment, and still works like a dream. I should note that I got the idea between this patch into Puma as well! https://github.com/puma/puma/pull/1227 [ew: squashed in temporary change for oob_gc.rb, but we'll come up with a different change to avoid breaking gctools ] Acked-by: Eric Wong --- test/unit/test_request.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) (limited to 'test') diff --git a/test/unit/test_request.rb b/test/unit/test_request.rb index f0ccaf7..dbe8af7 100644 --- a/test/unit/test_request.rb +++ b/test/unit/test_request.rb @@ -30,7 +30,7 @@ class RequestTest < Test::Unit::TestCase def test_options client = MockRequest.new("OPTIONS * HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal '', env['REQUEST_PATH'] assert_equal '', env['PATH_INFO'] assert_equal '*', env['REQUEST_URI'] @@ -40,7 +40,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_query client = MockRequest.new("GET http://e:3/x?y=z HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal 'y=z', env['QUERY_STRING'] @@ -50,7 +50,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_fragment client = MockRequest.new("GET http://e:3/x#frag HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal '', env['QUERY_STRING'] @@ -61,7 +61,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_query_and_fragment client = MockRequest.new("GET http://e:3/x?a=b#frag HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal 'a=b', env['QUERY_STRING'] @@ -73,7 +73,7 @@ class RequestTest < Test::Unit::TestCase %w(ssh+http://e/ ftp://e/x http+ssh://e/x).each do |abs_uri| client = MockRequest.new("GET #{abs_uri} HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - assert_raises(HttpParserError) { @request.read(client) } + assert_raises(HttpParserError) { @request.read(client, nil) } end end @@ -81,7 +81,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: https\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal "https", env['rack.url_scheme'] res = @lint.call(env) end @@ -90,7 +90,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: http\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal "http", env['rack.url_scheme'] res = @lint.call(env) end @@ -99,14 +99,14 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: ftp\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal "http", env['rack.url_scheme'] res = @lint.call(env) end def test_rack_lint_get client = MockRequest.new("GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal "http", env['rack.url_scheme'] assert_equal '127.0.0.1', env['REMOTE_ADDR'] res = @lint.call(env) @@ -114,7 +114,7 @@ class RequestTest < Test::Unit::TestCase def test_no_content_stringio client = MockRequest.new("GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal StringIO, env['rack.input'].class end @@ -122,7 +122,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("PUT / HTTP/1.1\r\n" \ "Content-Length: 0\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal StringIO, env['rack.input'].class end @@ -130,7 +130,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("PUT / HTTP/1.1\r\n" \ "Content-Length: 1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read(client, nil) assert_equal Unicorn::TeeInput, env['rack.input'].class end @@ -141,7 +141,7 @@ class RequestTest < Test::Unit::TestCase "Content-Length: 5\r\n" \ "\r\n" \ "abcde") - env = @request.read(client) + env = @request.read(client, nil) assert ! env.include?(:http_body) res = @lint.call(env) end @@ -167,7 +167,7 @@ class RequestTest < Test::Unit::TestCase "\r\n") count.times { assert_equal bs, client.syswrite(buf) } assert_equal 0, client.sysseek(0) - env = @request.read(client) + env = @request.read(client, nil) assert ! env.include?(:http_body) assert_equal length, env['rack.input'].size count.times { -- cgit v1.2.3-24-ge0c7 From 77b9ec2aa017cabe9babbbcba4f0cf5cea5f7aca Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 8 Mar 2017 00:33:06 +0000 Subject: new test for check_client_connection This was a bit tricky to test, but it's probably more reliable now that we're relying on TCP_INFO. Based on test by Simon Eskildsen : https://bogomips.org/unicorn-public/CAO3HKM49+aLD=KLij3zhJqkWnR7bCWVan0mOvxD85xfrW8RXOw@mail.gmail.com/ --- test/unit/test_ccc.rb | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 test/unit/test_ccc.rb (limited to 'test') diff --git a/test/unit/test_ccc.rb b/test/unit/test_ccc.rb new file mode 100644 index 0000000..22b1a9c --- /dev/null +++ b/test/unit/test_ccc.rb @@ -0,0 +1,81 @@ +require 'socket' +require 'unicorn' +require 'io/wait' +require 'tempfile' +require 'test/unit' + +class TestCccTCPI < Test::Unit::TestCase + def test_ccc_tcpi + start_pid = $$ + host = '127.0.0.1' + srv = TCPServer.new(host, 0) + port = srv.addr[1] + err = Tempfile.new('unicorn_ccc') + rd, wr = IO.pipe + pid = fork do + reqs = 0 + rd.close + worker_pid = nil + app = lambda do |env| + worker_pid ||= begin + at_exit { wr.write(reqs.to_s) if worker_pid == $$ } + $$ + end + reqs += 1 + sleep(1) if env['PATH_INFO'] == '/sleep' + [ 200, [ %w(Content-Length 0), %w(Content-Type text/plain) ], [] ] + end + ENV['UNICORN_FD'] = srv.fileno.to_s + opts = { + listeners: [ "#{host}:#{port}" ], + stderr_path: err.path, + check_client_connection: true, + } + uni = Unicorn::HttpServer.new(app, opts) + uni.start.join + end + wr.close + + # make sure the server is running, at least + client = TCPSocket.new(host, port) + client.write("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n") + assert client.wait_readable(10), 'never got response from server' + res = client.read + assert_match %r{\AHTTP/1\.1 200}, res, 'got part of first response' + assert_match %r{\r\n\r\n\z}, res, 'got end of response, server is ready' + client.close + + # start a slow request... + sleeper = TCPSocket.new(host, port) + sleeper.write("GET /sleep HTTP/1.1\r\nHost: example.com\r\n\r\n") + + # and a bunch of aborted ones + nr = 100 + nr.times do |i| + client = TCPSocket.new(host, port) + client.write("GET /collections/#{rand(10000)} HTTP/1.1\r\n" \ + "Host: example.com\r\n\r\n") + client.close + end + sleeper.close + kpid = pid + pid = nil + Process.kill(:QUIT, kpid) + _, status = Process.waitpid2(kpid) + assert status.success? + reqs = rd.read.to_i + warn "server got #{reqs} requests with #{nr} CCC aborted\n" if $DEBUG + assert_operator reqs, :<, nr + assert_operator reqs, :>=, 2, 'first 2 requests got through, at least' + ensure + return if start_pid != $$ + srv.close if srv + if pid + Process.kill(:QUIT, pid) + _, status = Process.waitpid2(pid) + assert status.success? + end + err.close! if err + rd.close if rd + end +end -- cgit v1.2.3-24-ge0c7 From 8ce88a3756b110e5e3001f640ebd53a5b11d8c65 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 8 Mar 2017 00:52:23 +0000 Subject: revert signature change to HttpServer#process_client We can force kgio_tryaccept to return an internal class for TCP objects by subclassing Kgio::TCPServer. This avoids breakage in any unfortunate projects which depend on our undocumented internal APIs, such as gctools --- test/unit/test_request.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) (limited to 'test') diff --git a/test/unit/test_request.rb b/test/unit/test_request.rb index dbe8af7..f0ccaf7 100644 --- a/test/unit/test_request.rb +++ b/test/unit/test_request.rb @@ -30,7 +30,7 @@ class RequestTest < Test::Unit::TestCase def test_options client = MockRequest.new("OPTIONS * HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal '', env['REQUEST_PATH'] assert_equal '', env['PATH_INFO'] assert_equal '*', env['REQUEST_URI'] @@ -40,7 +40,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_query client = MockRequest.new("GET http://e:3/x?y=z HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal 'y=z', env['QUERY_STRING'] @@ -50,7 +50,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_fragment client = MockRequest.new("GET http://e:3/x#frag HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal '', env['QUERY_STRING'] @@ -61,7 +61,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_query_and_fragment client = MockRequest.new("GET http://e:3/x?a=b#frag HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal 'a=b', env['QUERY_STRING'] @@ -73,7 +73,7 @@ class RequestTest < Test::Unit::TestCase %w(ssh+http://e/ ftp://e/x http+ssh://e/x).each do |abs_uri| client = MockRequest.new("GET #{abs_uri} HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - assert_raises(HttpParserError) { @request.read(client, nil) } + assert_raises(HttpParserError) { @request.read(client) } end end @@ -81,7 +81,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: https\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal "https", env['rack.url_scheme'] res = @lint.call(env) end @@ -90,7 +90,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: http\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal "http", env['rack.url_scheme'] res = @lint.call(env) end @@ -99,14 +99,14 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: ftp\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal "http", env['rack.url_scheme'] res = @lint.call(env) end def test_rack_lint_get client = MockRequest.new("GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal "http", env['rack.url_scheme'] assert_equal '127.0.0.1', env['REMOTE_ADDR'] res = @lint.call(env) @@ -114,7 +114,7 @@ class RequestTest < Test::Unit::TestCase def test_no_content_stringio client = MockRequest.new("GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal StringIO, env['rack.input'].class end @@ -122,7 +122,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("PUT / HTTP/1.1\r\n" \ "Content-Length: 0\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal StringIO, env['rack.input'].class end @@ -130,7 +130,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("PUT / HTTP/1.1\r\n" \ "Content-Length: 1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client, nil) + env = @request.read(client) assert_equal Unicorn::TeeInput, env['rack.input'].class end @@ -141,7 +141,7 @@ class RequestTest < Test::Unit::TestCase "Content-Length: 5\r\n" \ "\r\n" \ "abcde") - env = @request.read(client, nil) + env = @request.read(client) assert ! env.include?(:http_body) res = @lint.call(env) end @@ -167,7 +167,7 @@ class RequestTest < Test::Unit::TestCase "\r\n") count.times { assert_equal bs, client.syswrite(buf) } assert_equal 0, client.sysseek(0) - env = @request.read(client, nil) + env = @request.read(client) assert ! env.include?(:http_body) assert_equal length, env['rack.input'].size count.times { -- cgit v1.2.3-24-ge0c7