* [PATCH] check_client_connection: use tcp state on linux @ 2017-02-25 14:03 Simon Eskildsen 2017-02-25 16:19 ` Simon Eskildsen 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-02-25 14:03 UTC (permalink / raw) To: unicorn-public The implementation of the check_client_connection causing an early write is ineffective when not performed on loopback. In my testing, when on non-loopback, such as another host, we only see a 10-20% rejection rate with TCP_NODELAY of clients that are closed. This means 90-80% of responses in this case are rendered in vain. This patch uses getosockopt(2) with TCP_INFO on Linux to read the socket state. If the socket is in a state where it doesn't take a response, we'll abort the request with a similar error as to what write(2) would give us on a closed socket in case of an error. In my testing, this has a 100% rejection rate. This testing was conducted with the following script: 100.times do |i| client = TCPSocket.new("some-unicorn", 20_000) client.write("GET /collections/#{rand(10000)} HTTP/1.1\r\nHost:walrusser.myshopify.com\r\n\r\n") client.close end --- lib/unicorn/http_request.rb | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index 0c1f9bb..b4c95fc 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -31,6 +31,9 @@ class Unicorn::HttpParser @@input_class = Unicorn::TeeInput @@check_client_connection = false + # TCP_TIME_WAIT: 6, TCP_CLOSE: 7, TCP_CLOSE_WAIT: 8, TCP_LAST_ACK: 9 + IGNORED_CHECK_CLIENT_SOCKET_STATES = (6..9) + def self.input_class @@input_class end @@ -83,10 +86,18 @@ def read(socket) 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) } + # detect if the socket is valid by checking client connection. + if @@check_client_connection + if defined?(Raindrops::TCP_Info) + tcp_info = Raindrops::TCP_Info.new(socket) + if IGNORED_CHECK_CLIENT_SOCKET_STATES.cover?(tcp_info.state) + socket.close + raise Errno::EPIPE + end + elsif headers? + self.response_start_sent = true + HTTP_RESPONSE_START.each { |c| socket.write(c) } + end end e['rack.input'] = 0 == content_length ? -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-02-25 14:03 [PATCH] check_client_connection: use tcp state on linux Simon Eskildsen @ 2017-02-25 16:19 ` Simon Eskildsen 2017-02-25 23:12 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-02-25 16:19 UTC (permalink / raw) To: unicorn-public I left out a TCPSocket check, we'll need a `socket.is_a?(TCPSocket)` in there. I'll wait with sending an updated patch in case you have other comments. I'm also not entirely sure we need the `socket.close`. What do you think? On Sat, Feb 25, 2017 at 9:03 AM, Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > The implementation of the check_client_connection causing an early write is > ineffective when not performed on loopback. In my testing, when on non-loopback, > such as another host, we only see a 10-20% rejection rate with TCP_NODELAY of > clients that are closed. This means 90-80% of responses in this case are > rendered in vain. > > This patch uses getosockopt(2) with TCP_INFO on Linux to read the socket state. > If the socket is in a state where it doesn't take a response, we'll abort the > request with a similar error as to what write(2) would give us on a closed > socket in case of an error. > > In my testing, this has a 100% rejection rate. This testing was conducted with > the following script: > > 100.times do |i| > client = TCPSocket.new("some-unicorn", 20_000) > client.write("GET /collections/#{rand(10000)} > HTTP/1.1\r\nHost:walrusser.myshopify.com\r\n\r\n") > client.close > end > --- > lib/unicorn/http_request.rb | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb > index 0c1f9bb..b4c95fc 100644 > --- a/lib/unicorn/http_request.rb > +++ b/lib/unicorn/http_request.rb > @@ -31,6 +31,9 @@ class Unicorn::HttpParser > @@input_class = Unicorn::TeeInput > @@check_client_connection = false > > + # TCP_TIME_WAIT: 6, TCP_CLOSE: 7, TCP_CLOSE_WAIT: 8, TCP_LAST_ACK: 9 > + IGNORED_CHECK_CLIENT_SOCKET_STATES = (6..9) > + > def self.input_class > @@input_class > end > @@ -83,10 +86,18 @@ def read(socket) > 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) } > + # detect if the socket is valid by checking client connection. > + if @@check_client_connection > + if defined?(Raindrops::TCP_Info) > + tcp_info = Raindrops::TCP_Info.new(socket) > + if IGNORED_CHECK_CLIENT_SOCKET_STATES.cover?(tcp_info.state) > + socket.close > + raise Errno::EPIPE > + end > + elsif headers? > + self.response_start_sent = true > + HTTP_RESPONSE_START.each { |c| socket.write(c) } > + end > end > > e['rack.input'] = 0 == content_length ? > -- > 2.11.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-02-25 16:19 ` Simon Eskildsen @ 2017-02-25 23:12 ` Eric Wong 2017-02-27 11:44 ` Simon Eskildsen 0 siblings, 1 reply; 14+ messages in thread From: Eric Wong @ 2017-02-25 23:12 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > On Sat, Feb 25, 2017 at 9:03 AM, Simon Eskildsen > <simon.eskildsen@shopify.com> wrote: > > The implementation of the check_client_connection causing an early write is > > ineffective when not performed on loopback. In my testing, when on non-loopback, > > such as another host, we only see a 10-20% rejection rate with TCP_NODELAY of > > clients that are closed. This means 90-80% of responses in this case are > > rendered in vain. > > > > This patch uses getosockopt(2) with TCP_INFO on Linux to read the socket state. > > If the socket is in a state where it doesn't take a response, we'll abort the > > request with a similar error as to what write(2) would give us on a closed > > socket in case of an error. > > > > In my testing, this has a 100% rejection rate. This testing was conducted with > > the following script: Good to know! > > diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb > > index 0c1f9bb..b4c95fc 100644 > > --- a/lib/unicorn/http_request.rb > > +++ b/lib/unicorn/http_request.rb > > @@ -31,6 +31,9 @@ class Unicorn::HttpParser > > @@input_class = Unicorn::TeeInput > > @@check_client_connection = false > > > > + # TCP_TIME_WAIT: 6, TCP_CLOSE: 7, TCP_CLOSE_WAIT: 8, TCP_LAST_ACK: 9 > > + IGNORED_CHECK_CLIENT_SOCKET_STATES = (6..9) Thanks for documenting the numbers. I prefer we use a hash or case statement. Both allow more optimization in the YARV VM of CRuby (opt_aref and opt_case_dispatch in insns.def). case _might_ be a little faster if there's no constant lookup overhead, but a microbench or dumping the bytecode will be necessary to be sure :) A hash or a case can also help portability-wise in case we hit a system where these numbers are non-sequential; or if we forgot something. > > - # 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) } > > + # detect if the socket is valid by checking client connection. > > + if @@check_client_connection We can probably split everything inside this if to a new method, this avoid penalizing people who don't use this feature. Using check_client_connection will already have a high cost, since it requires at least one extra syscall. > > + if defined?(Raindrops::TCP_Info) ...because defined? only needs to be checked once for the lifetime of the process; we can define different methods at load time to avoid the check for each and every request. > > + tcp_info = Raindrops::TCP_Info.new(socket) > > + if IGNORED_CHECK_CLIENT_SOCKET_STATES.cover?(tcp_info.state) > > + socket.close Right, no need to socket.close, here; handle_error does it. > > + raise Errno::EPIPE Since we don't print out the backtrace in handle_error, we can raise without a backtrace to avoid excess garbage. > > + end > > + elsif headers? > > + self.response_start_sent = true > > + HTTP_RESPONSE_START.each { |c| socket.write(c) } > > + end > > end > I left out a TCPSocket check, we'll need a `socket.is_a?(TCPSocket)` > in there. I'll wait with sending an updated patch in case you have > other comments. I'm also not entirely sure we need the `socket.close`. > What do you think? Yep, we need to account for the UNIX socket case. I forget if kgio even makes them different... Anyways I won't be online much for a few days, and it's the weekend, so no rush :) Thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-02-25 23:12 ` Eric Wong @ 2017-02-27 11:44 ` Simon Eskildsen 2017-02-28 21:12 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-02-27 11:44 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public > I prefer we use a hash or case statement. Both allow more > optimization in the YARV VM of CRuby (opt_aref and > opt_case_dispatch in insns.def). case _might_ be a little > faster if there's no constant lookup overhead, but > a microbench or dumping the bytecode will be necessary > to be sure :) > > A hash or a case can also help portability-wise in case > we hit a system where these numbers are non-sequential; > or if we forgot something. Good point. I double checked all the states on Linux and found that we were missing TCP_CLOSING [1] [2]. This is a state where the other side is closed, and you have buffered data on your side. It doesn't seem like this would ever happen in Unicorn, but I think we should include it for completeness. This also means the range becomes non-sequential. I looked at Illumus (solaris-derived) [3] and BSD [4] and for the TCP states we're interested in it also appears to have a non-continues range. My co-worker, Kir Shatrov, benchmarked a bunch of approaches to the state check and found that case is a good solution [5]. Due to the realness of non-sequential states in common operating systems, I think case is the way to go here as you suggested. I've made sure to short-circuit the common-case of TCP_ESTABLISHED. I've only seen CLOSE_WAIT in testing, but in the wild-life of large production scale I would assume you would see TIME_WAIT and CLOSE. LAST_ACK_CLOSING it seems pretty unlikely to hit, but not impossible. As with CLOSING, I've included LAST_ACK_CLOSING for completeness. [1] https://github.com/torvalds/linux/blob/5924bbecd0267d87c24110cbe2041b5075173a25/include/net/tcp_states.h#L27 [2] https://github.com/torvalds/linux/blob/ca78d3173cff3503bcd15723b049757f75762d15/net/ipv4/tcp.c#L228 [3] https://github.com/freebsd/freebsd/blob/386ddae58459341ec567604707805814a2128a57/sys/netinet/tcp_fsm.h [4] https://github.com/illumos/illumos-gate/blob/f7877f5d39900cfd8b20dd673e5ccc1ef7cc7447/usr/src/uts/common/netinet/tcp_fsm.h [5] https://gist.github.com/kirs/11ba4ce84c08188c9f7eba9c639616a5 > Yep, we need to account for the UNIX socket case. I forget if > kgio even makes them different... I read the implementation and verified by dumping the class when testing on some test boxes. You are right—it's a simple Kgio::Socket object, not differentiating between Kgio::TCPSocket and Kgio::UnixSocket at the class level. Kgio only does this if they're explicitly passed to override the class returned from #try_accept. Unicorn doesn't do this. I've tried to find a way to determine the socket domain (INET vs. UNIX) on the socket object, but neither Ruby's Socket class nor Kgio seems to expose this. I'm not entirely sure what the simplest way to do this check would be. We could have the accept loop pass the correct class to #try_accept based on the listening socket that came back from #accept. If we passed the listening socket to #read after accept, we'd know.. but I don't like that the request knows about the listener either. Alternatively, we could expose the socket domain in Kgio, but that'll be problematic in the near-ish future as you've mentioned wanting to move away from Kgio as Ruby's IO library is at parity as per Ruby 2.4. What do you suggest pursuing here to check whether the client socket is a TCP socket? Below is a patch addressing the other concerns. I had to include require raindrops so the `defined?` check would do the right thing, as the only other file that requires Raindrops is the worker one which is loaded after http_request. I can change the load-order or require raindrops in lib/unicorn.rb if you prefer. Missing is the socket type check. Thanks for your feedback! --- lib/unicorn/http_request.rb | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index 0c1f9bb..eedccac 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) @@ -29,6 +30,7 @@ class Unicorn::HttpParser # 2.2+ optimizes hash assignments when used with literal string keys HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] @@input_class = Unicorn::TeeInput + @@raindrops_tcp_info_defined = defined?(Raindrops::TCP_Info) @@check_client_connection = false def self.input_class @@ -83,11 +85,7 @@ def read(socket) 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) if @@check_client_connection e['rack.input'] = 0 == content_length ? NULL_IO : @@input_class.new(socket, self) @@ -108,4 +106,27 @@ def call def hijacked? env.include?('rack.hijack_io'.freeze) end + + private + + def check_client_connection(socket) + if @@raindrops_tcp_info_defined + tcp_info = Raindrops::TCP_Info.new(socket) + raise Errno::EPIPE, "client closed connection".freeze, [] if closed_state?(tcp_info.state) + elsif headers? + self.response_start_sent = true + HTTP_RESPONSE_START.each { |c| socket.write(c) } + end + end + + def closed_state?(state) + case state + when 1 # ESTABLISHED + false + when 6, 7, 8, 9, 11 # TIME_WAIT, CLOSE, CLOSE_WAIT, LAST_ACK, CLOSING + true + else + false + end + end end -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-02-27 11:44 ` Simon Eskildsen @ 2017-02-28 21:12 ` Eric Wong 2017-03-01 3:18 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Eric Wong @ 2017-02-28 21:12 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public Simon Eskildsen <simon.eskildsen@shopify.com> wrote: <snip> > I would assume you would see TIME_WAIT and CLOSE. LAST_ACK_CLOSING it > seems pretty unlikely to hit, but not impossible. As with CLOSING, > I've included LAST_ACK_CLOSING for completeness. Did you mean "LAST_ACK, and CLOSING"? (not joined by underscore) Anyways, thanks for testing and adding > <e@80x24.org> wrote: > > Yep, we need to account for the UNIX socket case. I forget if > > kgio even makes them different... > > I read the implementation and verified by dumping the class when > testing on some test boxes. You are right—it's a simple Kgio::Socket > object, not differentiating between Kgio::TCPSocket and > Kgio::UnixSocket at the class level. Kgio only does this if they're > explicitly passed to override the class returned from #try_accept. > Unicorn doesn't do this. > > I've tried to find a way to determine the socket domain (INET vs. > UNIX) on the socket object, but neither Ruby's Socket class nor Kgio > seems to expose this. I'm not entirely sure what the simplest way to > do this check would be. We could have the accept loop pass the correct > class to #try_accept based on the listening socket that came back from > #accept. If we passed the listening socket to #read after accept, we'd > know.. but I don't like that the request knows about the listener > either. Alternatively, we could expose the socket domain in Kgio, but > that'll be problematic in the near-ish future as you've mentioned > wanting to move away from Kgio as Ruby's IO library is at parity as > per Ruby 2.4. > > What do you suggest pursuing here to check whether the client socket > is a TCP socket? I think passing the listening socket is the best way to go about detecting whether a socket is INET or UNIX, for now. You're right about kgio, I'd rather not make more changes to kgio but we will still need it to for Ruby <2.2.x users. And #read is an overloaded name, feel free to change it :) > Below is a patch addressing the other concerns. I had to include > require raindrops so the `defined?` check would do the right thing, as > the only other file that requires Raindrops is the worker one which is > loaded after http_request. I can change the load-order or require > raindrops in lib/unicorn.rb if you prefer. The require is fine. However we do not need a class variable, below... > # TODO: remove redundant names > Unicorn.const_set(:HttpRequest, Unicorn::HttpParser) > @@ -29,6 +30,7 @@ class Unicorn::HttpParser > # 2.2+ optimizes hash assignments when used with literal string keys > HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] > @@input_class = Unicorn::TeeInput > + @@raindrops_tcp_info_defined = defined?(Raindrops::TCP_Info) I prefer we avoid adding this cvar, instead... > @@check_client_connection = false > > def self.input_class > @@ -83,11 +85,7 @@ def read(socket) > 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) if @@check_client_connection > > e['rack.input'] = 0 == content_length ? > NULL_IO : @@input_class.new(socket, self) > @@ -108,4 +106,27 @@ def call > def hijacked? > env.include?('rack.hijack_io'.freeze) > end ... we can have different methods defined: if defined?(Raindrops::TCP_Info) # Linux, maybe FreeBSD def check_client_connection(client, listener) # :nodoc: ... end else # portable version def check_client_connection(client, listener) # :nodoc: ... end end And eliminate the class variable entirely. > + > + private I prefer to avoid marking methods as 'private' to ease any ad-hoc unit testing which may come up. Instead, rely on :nodoc: directives to discourage people from depending on it. Thanks. > + def check_client_connection(socket) > + if @@raindrops_tcp_info_defined > + tcp_info = Raindrops::TCP_Info.new(socket) > + raise Errno::EPIPE, "client closed connection".freeze, [] if > closed_state?(tcp_info.state) > + elsif headers? > + self.response_start_sent = true > + HTTP_RESPONSE_START.each { |c| socket.write(c) } > + end > + end > + > + def closed_state?(state) > + case state > + when 1 # ESTABLISHED > + false > + when 6, 7, 8, 9, 11 # TIME_WAIT, CLOSE, CLOSE_WAIT, LAST_ACK, CLOSING > + true > + else > + false > + end > + end > end closed_state? looks good to me, good call on short-circuiting the common case of ESTABLISHED! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-02-28 21:12 ` Eric Wong @ 2017-03-01 3:18 ` Eric Wong 2017-03-06 21:32 ` Simon Eskildsen 0 siblings, 1 reply; 14+ messages in thread From: Eric Wong @ 2017-03-01 3:18 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public > Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > > + tcp_info = Raindrops::TCP_Info.new(socket) > > + raise Errno::EPIPE, "client closed connection".freeze, [] if > > closed_state?(tcp_info.state) Also, I guess if you're using this frequently, it might make sense to keep the tcp_info object around and recycle it using the Raindrops::TCP_Info#get! method. #get! has always been supported in raindrops, but I just noticed RDoc wasn't picking it up properly :x Anyways I've fixed the documentation over on the raindrops list https://bogomips.org/raindrops-public/20170301025541.26183-1-e@80x24.org/ ("[PATCH] ext: fix documentation for C ext-defined classes") https://bogomips.org/raindrops-public/20170301025546.26233-1-e@80x24.org/ ("[PATCH] TCP_Info: custom documentation for #get!") ... and updated the RDoc on https://bogomips.org/raindrops/ Heck, I wonder if it even makes sense to reuse a frozen empty Array when raising the exception... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-01 3:18 ` Eric Wong @ 2017-03-06 21:32 ` Simon Eskildsen 2017-03-07 22:50 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-03-06 21:32 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public Here's another update Eric! * 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 --- lib/unicorn/http_request.rb | 44 ++++++++++++++++++++++++++++++++++++++------ lib/unicorn/http_server.rb | 6 +++--- test/unit/test_request.rb | 28 ++++++++++++++-------------- 3 files changed, 55 insertions(+), 23 deletions(-) diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index 0c1f9bb..21a99ef 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) @@ -28,6 +29,7 @@ class Unicorn::HttpParser # Drop these frozen strings when Ruby 2.2 becomes more prevalent, # 2.2+ optimizes hash assignments when used with literal string keys HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] + EMPTY_ARRAY = [].freeze @@input_class = Unicorn::TeeInput @@check_client_connection = false @@ -62,7 +64,7 @@ def self.check_client_connection=(bool) # 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 @@ -83,11 +85,7 @@ def read(socket) 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) @@ -108,4 +106,38 @@ def call 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 35bd100..4190641 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -558,8 +558,8 @@ def e100_response_write(client, env) # 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 @@ def worker_loop(worker) # 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/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 @@ def setup 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 @@ def test_options 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 @@ def test_absolute_uri_with_query 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 @@ def test_absolute_uri_with_fragment 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 @@ def test_absolute_uri_unsupported_schemes %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 @@ def test_x_forwarded_proto_https 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 @@ def test_x_forwarded_proto_http 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 @@ def test_x_forwarded_proto_invalid 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 @@ def test_rack_lint_get 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 @@ def test_zero_content_stringio 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 @@ def test_real_content_not_stringio 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 @@ def test_rack_lint_put "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 @@ def client.kgio_read!(*args) "\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 { -- 2.11.0 On Tue, Feb 28, 2017 at 10:18 PM, Eric Wong <e@80x24.org> wrote: >> Simon Eskildsen <simon.eskildsen@shopify.com> wrote: >> > + tcp_info = Raindrops::TCP_Info.new(socket) >> > + raise Errno::EPIPE, "client closed connection".freeze, [] if >> > closed_state?(tcp_info.state) > > Also, I guess if you're using this frequently, it might make > sense to keep the tcp_info object around and recycle it > using the Raindrops::TCP_Info#get! method. > > #get! has always been supported in raindrops, but I just noticed > RDoc wasn't picking it up properly :x > > Anyways I've fixed the documentation over on the raindrops list > > https://bogomips.org/raindrops-public/20170301025541.26183-1-e@80x24.org/ > ("[PATCH] ext: fix documentation for C ext-defined classes") > > https://bogomips.org/raindrops-public/20170301025546.26233-1-e@80x24.org/ > ("[PATCH] TCP_Info: custom documentation for #get!") > > ... and updated the RDoc on https://bogomips.org/raindrops/ > > Heck, I wonder if it even makes sense to reuse a frozen empty > Array when raising the exception... ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-06 21:32 ` Simon Eskildsen @ 2017-03-07 22:50 ` Eric Wong 2017-03-08 0:26 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Eric Wong @ 2017-03-07 22:50 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public, Aman Gupta Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > Here's another update Eric! Thanks, a few teeny issues fixed up locally (but commented inline, below). However, there's a bigger problem which I'm Cc:-ing Aman about for tmm1/gctools changing process_client in the internal API. I won't burden him maintaining extra versions, nor will I force users to use a certain version of unicorn or gctools to match. Aman: for reference, relevant messages in the archives: https://bogomips.org/unicorn-public/?q=d:20170222-+check_client_connection&x=t (TL;DR: I don't expect Aman will have to do anything, just keeping him in the loop) > +++ b/lib/unicorn/http_server.rb > @@ -558,8 +558,8 @@ def e100_response_write(client, env) > > # 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)) I can squash this fix in, locally: 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 @@ def self.new(app, interval = 5, path = %r{\A/}) #: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 However, https://github.com/tmm1/gctools also depends on this undocumented internal API of ours; and I will not consider breaking it for release... Pushed to the "ccc-tcp" branch @ git://bogomips.org/unicorn (commit beaee769c6553bf4e0260be2507b8235f0aa764f) > begin > return if @request.hijacked? > @@ -655,7 +655,7 @@ def worker_loop(worker) > # 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 > @@ -28,6 +29,7 @@ class Unicorn::HttpParser > # Drop these frozen strings when Ruby 2.2 becomes more prevalent, > # 2.2+ optimizes hash assignments when used with literal string keys > HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] > + EMPTY_ARRAY = [].freeze (minor) this was made before commit e06b699683738f22 ("http_request: freeze constant strings passed IO#write") but I've trivially fixed it up, locally. > + 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) (minor) I needed to wrap that line since I use gigantic fonts (fixed up locally) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-07 22:50 ` Eric Wong @ 2017-03-08 0:26 ` Eric Wong 2017-03-08 12:06 ` Simon Eskildsen 0 siblings, 1 reply; 14+ messages in thread From: Eric Wong @ 2017-03-08 0:26 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public, Aman Gupta Eric Wong <e@80x24.org> wrote: > Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > > @@ -28,6 +29,7 @@ class Unicorn::HttpParser > > # Drop these frozen strings when Ruby 2.2 becomes more prevalent, > > # 2.2+ optimizes hash assignments when used with literal string keys > > HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] > > + EMPTY_ARRAY = [].freeze > > (minor) this was made before commit e06b699683738f22 > ("http_request: freeze constant strings passed IO#write") > but I've trivially fixed it up, locally. And I actually screwed it up, pushed out ccc-tcp-v2 branch with that fix squashed in :x Writing tests, now... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-08 0:26 ` Eric Wong @ 2017-03-08 12:06 ` Simon Eskildsen 2017-03-13 20:16 ` Simon Eskildsen 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-03-08 12:06 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public Patch-set looks great Eric, thanks! I'm hoping to test this in production later this week or next, but we're a year behind on Unicorn (ugh), so will need to carefully roll that out. On Tue, Mar 7, 2017 at 7:26 PM, Eric Wong <e@80x24.org> wrote: > Eric Wong <e@80x24.org> wrote: >> Simon Eskildsen <simon.eskildsen@shopify.com> wrote: >> > @@ -28,6 +29,7 @@ class Unicorn::HttpParser >> > # Drop these frozen strings when Ruby 2.2 becomes more prevalent, >> > # 2.2+ optimizes hash assignments when used with literal string keys >> > HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] >> > + EMPTY_ARRAY = [].freeze >> >> (minor) this was made before commit e06b699683738f22 >> ("http_request: freeze constant strings passed IO#write") >> but I've trivially fixed it up, locally. > > And I actually screwed it up, pushed out ccc-tcp-v2 branch > with that fix squashed in :x > > Writing tests, now... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-08 12:06 ` Simon Eskildsen @ 2017-03-13 20:16 ` Simon Eskildsen 2017-03-13 20:37 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-03-13 20:16 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public I've put ccc-tcp-v3 in production today--it appears to be working as expected, still rejecting at the exact same rate as before (100-200 per second for us during steady-state). On Wed, Mar 8, 2017 at 7:06 AM, Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > Patch-set looks great Eric, thanks! > > I'm hoping to test this in production later this week or next, but > we're a year behind on Unicorn (ugh), so will need to carefully roll > that out. > > On Tue, Mar 7, 2017 at 7:26 PM, Eric Wong <e@80x24.org> wrote: >> Eric Wong <e@80x24.org> wrote: >>> Simon Eskildsen <simon.eskildsen@shopify.com> wrote: >>> > @@ -28,6 +29,7 @@ class Unicorn::HttpParser >>> > # Drop these frozen strings when Ruby 2.2 becomes more prevalent, >>> > # 2.2+ optimizes hash assignments when used with literal string keys >>> > HTTP_RESPONSE_START = [ 'HTTP', '/1.1 '] >>> > + EMPTY_ARRAY = [].freeze >>> >>> (minor) this was made before commit e06b699683738f22 >>> ("http_request: freeze constant strings passed IO#write") >>> but I've trivially fixed it up, locally. >> >> And I actually screwed it up, pushed out ccc-tcp-v2 branch >> with that fix squashed in :x >> >> Writing tests, now... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-13 20:16 ` Simon Eskildsen @ 2017-03-13 20:37 ` Eric Wong 2017-03-14 16:14 ` Simon Eskildsen 0 siblings, 1 reply; 14+ messages in thread From: Eric Wong @ 2017-03-13 20:37 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public Awesome! Thanks for the update. I'll merge ccc-tcp-v3 into master soon and push out 5.3.0-rc1 (Maybe there's need to quote at all, every message is archived forever in several public places: https://bogomips.org/unicorn-public/ https://www.mail-archive.com/unicorn-public@bogomips.org maybe some others...) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-13 20:37 ` Eric Wong @ 2017-03-14 16:14 ` Simon Eskildsen 2017-03-14 16:41 ` Eric Wong 0 siblings, 1 reply; 14+ messages in thread From: Simon Eskildsen @ 2017-03-14 16:14 UTC (permalink / raw) To: Eric Wong; +Cc: unicorn-public Looks like Puma encountered some issues with some Linux distro's kernels not supporting this. That's crazy.. https://github.com/puma/puma/issues/1241 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] check_client_connection: use tcp state on linux 2017-03-14 16:14 ` Simon Eskildsen @ 2017-03-14 16:41 ` Eric Wong 0 siblings, 0 replies; 14+ messages in thread From: Eric Wong @ 2017-03-14 16:41 UTC (permalink / raw) To: Simon Eskildsen; +Cc: unicorn-public Simon Eskildsen <simon.eskildsen@shopify.com> wrote: > Looks like Puma encountered some issues with some Linux distro's > kernels not supporting this. That's crazy.. > > https://github.com/puma/puma/issues/1241 No, it's not a Linux kernel problem. That looks like a Unix socket they're using, but they only use TCPServer.for_fd in their import_from_env function, and inherit_unix_listener leaves TCPServer objects as-is. TCPServer.for_fd won't barf if given a Unix socket. In other words, this is fine: u = UNIXServer.new '/tmp/foo' t = TCPServer.for_fd(u.fileno) t.accept Feel free to forward this to them, I do not use non-Free messaging systems (and I hate centralized ones). ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-14 16:41 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-25 14:03 [PATCH] check_client_connection: use tcp state on linux Simon Eskildsen 2017-02-25 16:19 ` Simon Eskildsen 2017-02-25 23:12 ` Eric Wong 2017-02-27 11:44 ` Simon Eskildsen 2017-02-28 21:12 ` Eric Wong 2017-03-01 3:18 ` Eric Wong 2017-03-06 21:32 ` Simon Eskildsen 2017-03-07 22:50 ` Eric Wong 2017-03-08 0:26 ` Eric Wong 2017-03-08 12:06 ` Simon Eskildsen 2017-03-13 20:16 ` Simon Eskildsen 2017-03-13 20:37 ` Eric Wong 2017-03-14 16:14 ` Simon Eskildsen 2017-03-14 16:41 ` Eric Wong
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).