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 --- lib/unicorn/http_request.rb | 45 +++++++++++++++++++++++++++++++++++++++------ lib/unicorn/http_server.rb | 6 +++--- lib/unicorn/oob_gc.rb | 4 ++-- 3 files changed, 44 insertions(+), 11 deletions(-) (limited to 'lib/unicorn') diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index c176083..9acde50 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -2,6 +2,7 @@ # :enddoc: # no stable API here require 'unicorn_http' +require 'raindrops' # TODO: remove redundant names Unicorn.const_set(:HttpRequest, Unicorn::HttpParser) @@ -25,6 +26,7 @@ class Unicorn::HttpParser # :stopdoc: HTTP_RESPONSE_START = [ 'HTTP'.freeze, '/1.1 '.freeze ] + EMPTY_ARRAY = [].freeze @@input_class = Unicorn::TeeInput @@check_client_connection = false @@ -59,7 +61,7 @@ class Unicorn::HttpParser # returns an environment hash suitable for Rack if successful # This does minimal exception trapping and it is up to the caller # to handle any socket errors (e.g. user aborted upload). - def read(socket) + def read(socket, listener) clear e = env @@ -80,11 +82,7 @@ class Unicorn::HttpParser 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? - self.response_start_sent = true - HTTP_RESPONSE_START.each { |c| socket.write(c) } - end + check_client_connection(socket, listener) if @@check_client_connection e['rack.input'] = 0 == content_length ? NULL_IO : @@input_class.new(socket, self) @@ -105,4 +103,39 @@ class Unicorn::HttpParser def hijacked? env.include?('rack.hijack_io'.freeze) end + + if defined?(Raindrops::TCP_Info) + def check_client_connection(socket, listener) # :nodoc: + if Kgio::TCPServer === listener + @@tcp_info ||= Raindrops::TCP_Info.new(socket) + @@tcp_info.get!(socket) + raise Errno::EPIPE, "client closed connection".freeze, + EMPTY_ARRAY if closed_state?(@@tcp_info.state) + else + write_http_header(socket) + end + end + + def closed_state?(state) # :nodoc: + case state + when 1 # ESTABLISHED + false + when 8, 6, 7, 9, 11 # CLOSE_WAIT, TIME_WAIT, CLOSE, LAST_ACK, CLOSING + true + else + false + end + end + else + def check_client_connection(socket, listener) # :nodoc: + write_http_header(socket) + end + end + + def write_http_header(socket) # :nodoc: + if headers? + self.response_start_sent = true + HTTP_RESPONSE_START.each { |c| socket.write(c) } + end + end end diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb index c2086cb..2aa1072 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -558,8 +558,8 @@ class Unicorn::HttpServer # 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)) + def process_client(client, listener) + status, headers, body = @app.call(env = @request.read(client, listener)) begin return if @request.hijacked? @@ -655,7 +655,7 @@ class Unicorn::HttpServer # Unicorn::Worker#kgio_tryaccept is not like accept(2) at all, # but that will return false if client = sock.kgio_tryaccept - process_client(client) + process_client(client, sock) nr += 1 worker.tick = time_now.to_i end diff --git a/lib/unicorn/oob_gc.rb b/lib/unicorn/oob_gc.rb index 5572e59..74a1d51 100644 --- a/lib/unicorn/oob_gc.rb +++ b/lib/unicorn/oob_gc.rb @@ -67,8 +67,8 @@ module Unicorn::OobGC #:stopdoc: PATH_INFO = "PATH_INFO" - def process_client(client) - super(client) # Unicorn::HttpServer#process_client + def process_client(client, listener) + super(client, listener) # Unicorn::HttpServer#process_client if OOBGC_PATH =~ OOBGC_ENV[PATH_INFO] && ((@@nr -= 1) <= 0) @@nr = OOBGC_INTERVAL OOBGC_ENV.clear -- 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 --- lib/unicorn/http_request.rb | 10 +++++----- lib/unicorn/http_server.rb | 6 +++--- lib/unicorn/oob_gc.rb | 4 ++-- lib/unicorn/socket_helper.rb | 16 ++++++++++++++-- 4 files changed, 24 insertions(+), 12 deletions(-) (limited to 'lib/unicorn') diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index 9acde50..68bde16 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -61,7 +61,7 @@ class Unicorn::HttpParser # returns an environment hash suitable for Rack if successful # This does minimal exception trapping and it is up to the caller # to handle any socket errors (e.g. user aborted upload). - def read(socket, listener) + def read(socket) clear e = env @@ -82,7 +82,7 @@ class Unicorn::HttpParser false until add_parse(socket.kgio_read!(16384)) end - check_client_connection(socket, listener) if @@check_client_connection + check_client_connection(socket) if @@check_client_connection e['rack.input'] = 0 == content_length ? NULL_IO : @@input_class.new(socket, self) @@ -105,8 +105,8 @@ class Unicorn::HttpParser end if defined?(Raindrops::TCP_Info) - def check_client_connection(socket, listener) # :nodoc: - if Kgio::TCPServer === listener + def check_client_connection(socket) # :nodoc: + if Unicorn::TCPClient === socket @@tcp_info ||= Raindrops::TCP_Info.new(socket) @@tcp_info.get!(socket) raise Errno::EPIPE, "client closed connection".freeze, @@ -127,7 +127,7 @@ class Unicorn::HttpParser end end else - def check_client_connection(socket, listener) # :nodoc: + def check_client_connection(socket) # :nodoc: write_http_header(socket) end end diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb index 2aa1072..c2086cb 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -558,8 +558,8 @@ class Unicorn::HttpServer # 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, listener) - status, headers, body = @app.call(env = @request.read(client, listener)) + def process_client(client) + status, headers, body = @app.call(env = @request.read(client)) begin return if @request.hijacked? @@ -655,7 +655,7 @@ class Unicorn::HttpServer # Unicorn::Worker#kgio_tryaccept is not like accept(2) at all, # but that will return false if client = sock.kgio_tryaccept - process_client(client, sock) + process_client(client) nr += 1 worker.tick = time_now.to_i end diff --git a/lib/unicorn/oob_gc.rb b/lib/unicorn/oob_gc.rb index 74a1d51..5572e59 100644 --- a/lib/unicorn/oob_gc.rb +++ b/lib/unicorn/oob_gc.rb @@ -67,8 +67,8 @@ module Unicorn::OobGC #:stopdoc: PATH_INFO = "PATH_INFO" - def process_client(client, listener) - super(client, listener) # Unicorn::HttpServer#process_client + def process_client(client) + super(client) # Unicorn::HttpServer#process_client if OOBGC_PATH =~ OOBGC_ENV[PATH_INFO] && ((@@nr -= 1) <= 0) @@nr = OOBGC_INTERVAL OOBGC_ENV.clear diff --git a/lib/unicorn/socket_helper.rb b/lib/unicorn/socket_helper.rb index df8315e..5371413 100644 --- a/lib/unicorn/socket_helper.rb +++ b/lib/unicorn/socket_helper.rb @@ -3,6 +3,18 @@ require 'socket' module Unicorn + + # Instead of using a generic Kgio::Socket for everything, + # tag TCP sockets so we can use TCP_INFO under Linux without + # incurring extra syscalls for Unix domain sockets. + # TODO: remove these when we remove kgio + TCPClient = Class.new(Kgio::Socket) # :nodoc: + class TCPSrv < Kgio::TCPServer # :nodoc: + def kgio_tryaccept # :nodoc: + super(TCPClient) + end + end + module SocketHelper # internal interface @@ -148,7 +160,7 @@ module Unicorn end sock.bind(Socket.pack_sockaddr_in(port, addr)) sock.autoclose = false - Kgio::TCPServer.for_fd(sock.fileno) + TCPSrv.for_fd(sock.fileno) end # returns rfc2732-style (e.g. "[::1]:666") addresses for IPv6 @@ -185,7 +197,7 @@ module Unicorn def server_cast(sock) begin Socket.unpack_sockaddr_in(sock.getsockname) - Kgio::TCPServer.for_fd(sock.fileno) + TCPSrv.for_fd(sock.fileno) rescue ArgumentError Kgio::UNIXServer.for_fd(sock.fileno) end -- cgit v1.2.3-24-ge0c7 From 873218e317773462be2a72556d26dc4a723cc7be Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 8 Mar 2017 04:56:46 +0000 Subject: support "struct tcp_info" on non-Linux and Ruby 2.2+ Ruby 2.2+ can show "struct tcp_info" as a string via Socket::Option#inspect, and we can attempt to parse it out to extract the information we need. Parsing this string is inefficient, but does not depend on the ordering of the tcp_info struct. --- lib/unicorn/http_request.rb | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) (limited to 'lib/unicorn') diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index 68bde16..c08097c 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -29,6 +29,7 @@ class Unicorn::HttpParser EMPTY_ARRAY = [].freeze @@input_class = Unicorn::TeeInput @@check_client_connection = false + @@tcpi_inspect_ok = true def self.input_class @@input_class @@ -127,8 +128,37 @@ class Unicorn::HttpParser end end else + + # Ruby 2.2+ can show struct tcp_info as a string Socket::Option#inspect. + # Not that efficient, but probably still better than doing unnecessary + # work after a client gives up. def check_client_connection(socket) # :nodoc: - write_http_header(socket) + if Unicorn::TCPClient === socket && @@tcpi_inspect_ok + opt = socket.getsockopt(:IPPROTO_TCP, :TCP_INFO).inspect + if opt =~ /\bstate=(\S+)/ + @@tcpi_inspect_ok = true + raise Errno::EPIPE, "client closed connection".freeze, + EMPTY_ARRAY if closed_state_str?($1) + else + @@tcpi_inspect_ok = false + write_http_header(socket) + end + opt.clear + else + write_http_header(socket) + end + end + + def closed_state_str?(state) + case state + when 'ESTABLISHED' + false + # not a typo, ruby maps TCP_CLOSE (no 'D') to state=CLOSED (w/ 'D') + when 'CLOSE_WAIT', 'TIME_WAIT', 'CLOSED', 'LAST_ACK', 'CLOSING' + true + else + false + end end end -- cgit v1.2.3-24-ge0c7 From 20c66dbf1ebd0ca993e7a79c9d0d833d747df358 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 14 Mar 2017 19:18:55 +0000 Subject: http_request: reduce insn size for check_client_connection Unlike constants and instance variables, class variable access is not optimized in the mainline Ruby VM. Use a constant instead, to take advantage of inline constant caching. This further reduces runtime instruction size by avoiding a branch by allocating the Raindrops::TCP_Info object up front. This reduces the method size by roughly 300 bytes on 64-bit. --- lib/unicorn/http_request.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'lib/unicorn') diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index c08097c..9010007 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -106,12 +106,13 @@ class Unicorn::HttpParser end if defined?(Raindrops::TCP_Info) + TCPI = Raindrops::TCP_Info.allocate + def check_client_connection(socket) # :nodoc: if Unicorn::TCPClient === socket - @@tcp_info ||= Raindrops::TCP_Info.new(socket) - @@tcp_info.get!(socket) + # Raindrops::TCP_Info#get!, #state (reads struct tcp_info#tcpi_state) raise Errno::EPIPE, "client closed connection".freeze, - EMPTY_ARRAY if closed_state?(@@tcp_info.state) + EMPTY_ARRAY if closed_state?(TCPI.get!(socket).state) else write_http_header(socket) end -- cgit v1.2.3-24-ge0c7