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 <[email protected]> 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
