unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
From: Simon Eskildsen <simon.eskildsen@shopify.com>
To: unicorn-public@bogomips.org
Subject: check_client_connection using getsockopt(2)
Date: Wed, 22 Feb 2017 07:02:56 -0500	[thread overview]
Message-ID: <CAO3HKM49+aLD=KLij3zhJqkWnR7bCWVan0mOvxD85xfrW8RXOw@mail.gmail.com> (raw)

Hello!

Almost five years ago Tom Burns contributed the patch in collaboration
with Eric that introduced the `check_client_connection` configuration
option. To summarize the patch, it was a solution to a problem we have
of rapid refreshes during sales where Unicorn would render a page 5
times if an eager customer refreshed Shopify 5 times, despite only
seeing one-rendering.  This is a large amount of lost capacity. Four
of these connections would effectively be in a `CLOSE` state in the
backlog, get `accept(2)`ed and a response would be sent back only to
get an error that the client had closed its connection.

The patch solved this problem by instead of doing a single `write(2)`,
it would do a write of the generic HTTP version line, then jump into
the middleware stack and render the Rack response in a second write.
If the client had closed, the first `write(2)` of the HTTP version
header would usually throw an exception causing Unicorn to bail before
rendering the heavy Rack response. This saves a large amount of
capacity during spiky traffic.

A subsequent commit after testing by Eric revealed that:

> This only affects clients connecting over Unix domain sockets and TCP via loopback (127...*). It is unlikely to detect disconnects if the client is on a remote host (even on a fast LAN).

Thanks for that testing Eric. If we hadn't stumbled upon this in the
documentation proactively, this wouldn't have been easy to debug in
production.

In my testing, I can confirm Eric's tests. My testing essentially
consists of a snippet like the following to send rapid requests and
then closing the client. I've confirmed with Wireshark this is roughly
how popular browsers behave when refreshing fast on a slowly rendered
page:

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

This confirms Eric's comment that the existing
`check_client_connection` works perfectly on loopback, but as soon as
you put an actual network between the Unicorn and client—it's only
effective 20% of the time, even with `TCP_NODELAY`. I'm assuming, due
to buffering, even when disabling Nagle's. As we're changing our
architecture, we move from ngx (lb) -> ngx (host) -> unicorn to ngx
(lb) -> unicorn. That means this patch will no longer work for us.

I propose instead of the early `write(2)` hack, that we use
`getsockopt(2)` with the `TCP_INFO` flag and read the state of the
socket. If it's in `CLOSE_WAIT` or `CLOSE`, we kill the connection and
move on to the next.

https://github.com/torvalds/linux/blob/8fa3b6f9392bf6d90cb7b908e07bd90166639f0a/include/uapi/linux/tcp.h#L163

This is not going to be portable, but we can do this on only Linux
which I suspect is where most production deployments of Unicorn that
would benefit from this feature run. It's not useful in development
(which is likely more common to not be Linux). We could also introduce
it under a new configuration option if desired. In my testing, this
works to reject 100% of requests early when not on loopback.

The code is essentially:

def client_closed?
  tcp_info = socket.getsockopt(Socket::SOL_TCP, Socket::TCP_INFO)
  state = tcp_info.unpack("c")[0]
  state == TCP_CLOSE || state == TCP_CLOSE_WAIT
end

This could be called at the top of `#process_client` in `http_server.rb`.

Would there be interest in this upstream? Any comments on this
proposed implementation? Currently, we're using a middleware with the
Rack hijack API, but this seems like it belongs at the webserver
level.

             reply	other threads:[~2017-02-22 12:02 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-22 12:02 Simon Eskildsen [this message]
2017-02-22 18:33 ` check_client_connection using getsockopt(2) Eric Wong
2017-02-22 20:09   ` Simon Eskildsen
2017-02-23  1:42     ` Eric Wong
2017-02-23  2:42       ` Simon Eskildsen
2017-02-23  3:52         ` Eric Wong

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='CAO3HKM49+aLD=KLij3zhJqkWnR7bCWVan0mOvxD85xfrW8RXOw@mail.gmail.com' \
    --to=simon.eskildsen@shopify.com \
    --cc=unicorn-public@bogomips.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).