unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
Search results ordered by [date|relevance]  view[summary|nested|Atom feed]
thread overview below | download mbox.gz: |
* [RFC/PATCH] check_client_connection: document local-only requirement
  @ 2012-11-29 21:55 73%                   ` Eric Wong
  2012-11-29 23:47 99%                     ` Tom Burns
  0 siblings, 1 reply; 30+ results
From: Eric Wong @ 2012-11-29 21:55 UTC (permalink / raw)
  To: unicorn list

In my testing, only dropped clients over Unix domain sockets or
loopback TCP were detected with this option.  Since many
nginx+unicorn combinations run on the same host, this is not a
problem.

Furthermore, tcp_nodelay:true appears to work over loopback,
so remove the requirement for tcp_nodelay:false.
---
  Eric Wong <normalperson@yhbt.net> wrote:
  > Tom Burns <tom.burns@jadedpixel.com> wrote:
  > > +    if set[:check_client_connection]
  > > +      set[:listeners].each do |address|
  > > +        if set[:listener_opts][address][:tcp_nopush] == true
  > > +          raise ArgumentError,
  > > +            "check_client_connection is incompatible with tcp_nopush:true"
  > > +        end
  > 
  > Btw, were you using:
  > 
  > 1) TCP over loopback (bound to 0.0.0.0, client comes from 127.0.0.1)
  > 2) TCP over a LAN (separate client/server hosts)
  > 3) Unix domain socket
  > 
  > I wonder if we can drop the below hunk for checking :tcp_nodelay,
  > and document that check_client_connection requires the client to
  > either be from a Unix domain socket or TCP loopback to work.
  > 
  > > +        if set[:listener_opts][address][:tcp_nodelay] == true
  > > +          raise ArgumentError,
  > > +            "check_client_connection is incompatible with tcp_nodelay:true"
  > > +        end
  > 
  > I couldn't get 2) to work with either value of tcp_nodelay.  My small
  > LAN at home only has ~0.100ms latency.
  > 
  > Happily, with TCP over loopback (on Linux 3.6), either value of
  > tcp_nodelay works, so the tcp_nodelay check seems unnecessary after
  > all.

 lib/unicorn/configurator.rb | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 9752cdd..7651093 100644
--- a/lib/unicorn/configurator.rb
+++ b/lib/unicorn/configurator.rb
@@ -103,10 +103,6 @@ class Unicorn::Configurator
           raise ArgumentError,
             "check_client_connection is incompatible with tcp_nopush:true"
         end
-        if set[:listener_opts][address][:tcp_nodelay] == true
-          raise ArgumentError,
-            "check_client_connection is incompatible with tcp_nodelay:true"
-        end
       end
     end
     set.each do |key, value|
@@ -473,8 +469,11 @@ class Unicorn::Configurator
   # This will prevent calling the application for clients who have
   # disconnected while their connection was queued.
   #
-  # This option cannot be used in conjunction with tcp_nodelay or
-  # tcp_nopush.
+  # 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).
+  #
+  # This option cannot be used in conjunction with :tcp_nopush.
   def check_client_connection(bool)
     set_bool(:check_client_connection, bool)
   end
-- 
1.8.0.3.gdd57fab.dirty
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

^ permalink raw reply related	[relevance 73%]

* Re: [RFC/PATCH] check_client_connection: document local-only requirement
  2012-11-29 21:55 73%                   ` [RFC/PATCH] check_client_connection: document local-only requirement Eric Wong
@ 2012-11-29 23:47 99%                     ` Tom Burns
  0 siblings, 0 replies; 30+ results
From: Tom Burns @ 2012-11-29 23:47 UTC (permalink / raw)
  To: unicorn list

On Thu, Nov 29, 2012 at 4:55 PM, Eric Wong <normalperson@yhbt.net> wrote:
> In my testing, only dropped clients over Unix domain sockets or
> loopback TCP were detected with this option.  Since many
> nginx+unicorn combinations run on the same host, this is not a
> problem.
>
> Furthermore, tcp_nodelay:true appears to work over loopback,
> so remove the requirement for tcp_nodelay:false.

In production our configuration is same host / UNIX domain socket, so
I don't have any comment on the TCP changes.

It makes sense to me though, and the documentation looks better!

Cheers,
Tom
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

^ permalink raw reply	[relevance 99%]

* [ANN] unicorn 4.5.0pre1 - check_client_connection option
@ 2012-11-30  0:07 93% Eric Wong
  0 siblings, 0 replies; 30+ results
From: Eric Wong @ 2012-11-30  0:07 UTC (permalink / raw)
  To: mongrel-unicorn

Just pushed out the RubyGem for this.

Changes:

The new check_client_connection option allows unicorn to detect
most disconnected clients before potentially expensive
application processing begins.

This feature is useful for applications experiencing spikes of
traffic leading to undesirable queue times, as clients will
disconnect (and perhaps even retry, compounding the problem)
before unicorn can even start processing the request.

To enable this feature, add the following line to a unicorn
config file:

  check_client_connection true

A huge thanks to Tom Burns for implementing and testing this
change in production with real traffic (including mitigating
an unexpected DoS attack).

* http://unicorn.bogomips.org/
* mongrel-unicorn@rubyforge.org
* git://bogomips.org/unicorn.git
* http://unicorn.bogomips.org/NEWS.atom.xml
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

^ permalink raw reply	[relevance 93%]

* [ANN] unicorn 4.5.0 (final) - check_client_connection option
@ 2012-12-08  0:17 89% Eric Wong
  0 siblings, 0 replies; 30+ results
From: Eric Wong @ 2012-12-08  0:17 UTC (permalink / raw)
  To: mongrel-unicorn

Changes:

The new check_client_connection option allows unicorn to detect
most disconnected local clients before potentially expensive
application processing begins.

This feature is useful for applications experiencing spikes of
traffic leading to undesirable queue times, as clients will
disconnect (and perhaps even retry, compounding the problem)
before unicorn can even start processing the request.

To enable this feature, add the following line to a unicorn
config file:

      check_client_connection true

This feature only works when nginx (or any other HTTP/1.0+
client) is on the same machine as unicorn.

A huge thanks to Tom Burns for implementing and testing this
change in production with real traffic (including mitigating
an unexpected DoS attack).

ref: http://mid.gmane.org/CAK4qKG3rkfVYLyeqEqQyuNEh_nZ8yw0X_cwTxJfJ+TOU+y8F+w@mail.gmail.com

This release fixes broken Rainbows! compatibility in 4.5.0pre1.

* http://unicorn.bogomips.org/
* mongrel-unicorn@rubyforge.org
* git://bogomips.org/unicorn.git
* http://unicorn.bogomips.org/NEWS.atom.xml

-- 
Eric Wong
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

^ permalink raw reply	[relevance 89%]

* non-Linux check_client_connection users?
@ 2014-02-08  7:48 99% Eric Wong
  0 siblings, 0 replies; 30+ results
From: Eric Wong @ 2014-02-08  7:48 UTC (permalink / raw)
  To: mongrel-unicorn

Hey all, just wondering how well (if at all) the check_client_connection
feature works for non-Linux users.

Reports of success/failure and info about the connection type (TCP vs
Unix socket) and loopback or over a NIC if on TCP would be greatly
appreciated, thanks!
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

^ permalink raw reply	[relevance 99%]

* response_start_sent (check_client_connection) is staying
@ 2017-02-15 18:45 99% Eric Wong
  0 siblings, 0 replies; 30+ results
From: Eric Wong @ 2017-02-15 18:45 UTC (permalink / raw)
  To: unicorn-public

There's no way I would remove this feature or any other
feature other people rely on.  Just pushed this out:

commit c4b58719e8aa7f2b4979d139ca166a8c6a11eb7e
Author: Eric Wong <e@80x24.org>
Date:   Wed Feb 15 18:37:10 2017 +0000

    Revert "remove response_start_sent"
    
    Oops, this was a half-baked change I was considering
    but forgot about.
    
    This reverts commit 69fd4f9bbff3708166fbf70163fa6e192dde1497.

commit 69fd4f9bbff3708166fbf70163fa6e192dde1497 was the start of
a reorganization that I ultimately abandoned, at least for now.

https://bogomips.org/unicorn.git/patch?id=c4b58719e8aa7f2b
https://bogomips.org/unicorn.git/patch?id=69fd4f9bbff37081

(and I'm still working on a better git viewer for lynx/w3m users)

^ permalink raw reply	[relevance 99%]

* check_client_connection using getsockopt(2)
@ 2017-02-22 12:02 62% Simon Eskildsen
  2017-02-22 18:33 81% ` Eric Wong
  0 siblings, 1 reply; 30+ results
From: Simon Eskildsen @ 2017-02-22 12:02 UTC (permalink / raw)
  To: unicorn-public

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.

^ permalink raw reply	[relevance 62%]

* Re: check_client_connection using getsockopt(2)
  2017-02-22 12:02 62% check_client_connection using getsockopt(2) Simon Eskildsen
@ 2017-02-22 18:33 81% ` Eric Wong
  2017-02-22 20:09 65%   ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
From: Eric Wong @ 2017-02-22 18:33 UTC (permalink / raw)
  To: Simon Eskildsen; +Cc: unicorn-public, raindrops-public

Simon Eskildsen <simon.eskildsen@shopify.com> wrote:

<snip> great to know it's still working after all these years :>

> 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.

Side comment: I'm a bit curious how you guys arrived at removing
nginx at the host level (if you're allowed to share that info)

I've mainly kept nginx on the same host as unicorn, but used
haproxy or similar (with minimal buffering) at the LB level.
That allows better filesystem load distribution for large uploads.

> 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.

Good to know about it's effectiveness!  I don't mind adding
Linux-only features as long as existing functionality still
works on the server-oriented *BSDs.

> 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

+Cc: raindrops-public@bogomips.org -> https://bogomips.org/raindrops-public/

Fwiw, raindrops (already a dependency of unicorn) has TCP_INFO
support under Linux:

https://bogomips.org/raindrops.git/tree/ext/raindrops/linux_tcp_info.c

I haven't used it, much, but FreeBSD also implements a subset of
this feature, nowadays, and ought to be supportable, too.  We
can look into adding it for raindrops.

I don't know about other BSDs...

> 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.

I guess hijack means you have to discard other middlewares and
the normal rack response handling in unicorn?  If so, ouch!

Unfortunately, without hijack, there's no portable way in Rack
to get at the underlying IO object; so I guess this needs to
be done at the server level...

So yeah, I guess it belongs in the webserver.

I think we can automatically use TCP_INFO if available for
check_client_connection; then fall back to the old early write
hack for Unix sockets and systems w/o TCP_INFO (which would set
the response_start_sent flag).

No need to introduce yet another configuration option.

^ permalink raw reply	[relevance 81%]

* Re: check_client_connection using getsockopt(2)
  2017-02-22 18:33 81% ` Eric Wong
@ 2017-02-22 20:09 65%   ` Simon Eskildsen
  2017-02-23  1:42 86%     ` Eric Wong
  0 siblings, 1 reply; 30+ results
From: Simon Eskildsen @ 2017-02-22 20:09 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public, raindrops-public

On Wed, Feb 22, 2017 at 1:33 PM, Eric Wong <e@80x24.org> wrote:
> Simon Eskildsen <simon.eskildsen@shopify.com> wrote:
>
> <snip> great to know it's still working after all these years :>
>
>> 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.
>
> Side comment: I'm a bit curious how you guys arrived at removing
> nginx at the host level (if you're allowed to share that info)
>
> I've mainly kept nginx on the same host as unicorn, but used
> haproxy or similar (with minimal buffering) at the LB level.
> That allows better filesystem load distribution for large uploads.


Absolutely. We're starting to experiment with Kubernetes, and a result
we're interested in simplifying ingress as much as possible. We could
still run them, but if I can avoid explaining why they're there for
the next 5 years—I'll be happy :) As I see, the current use-cases we
have for the host nginx are (with why we can get rid of it):

* Keepalive between edge nginx LBs and host LBs to avoid excessive
network traffic. This is not a deal-breaker, so we'll just ignore this
problem. It's a _massive_ amount of traffic normally though.
* Out of rotation. We take nodes gracefully out of rotation by
touching a file that'll return 404 on a health-checking endpoint from
the edge LBs. Kubernetes implements this by just removing all the
containers on that node.
* Graceful deploys. When we deploy with containers, we take each
container out of rotation with the local host Nginx, wait for it to
come up, and put it back in rotation. We don't utilize Unicorns
graceful restarts because we want a Ruby upgrade deploy (replacing the
container) to be the same as an updated code rollout.
* File uploads. As you mention, currently we load-balance this between
them. I haven't finished the investigation on whether this is feasible
on the front LBs. Otherwise we may go for a 2-tier Nginx solution or
expand the edge. However, with Kubernetes I'd really like to avoid
having host nginxes. It's not very native to the Kubernetes paradigm.
Does it balance across the network, or only to the pods on that node?
* check_client_connection working. :-) This thread.

Slow clients and other advantages we find from the edge Nginxes.

>> 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.
>
> Good to know about it's effectiveness!  I don't mind adding
> Linux-only features as long as existing functionality still
> works on the server-oriented *BSDs.
>
>> 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
>
> +Cc: raindrops-public@bogomips.org -> https://bogomips.org/raindrops-public/
>
> Fwiw, raindrops (already a dependency of unicorn) has TCP_INFO
> support under Linux:
>
> https://bogomips.org/raindrops.git/tree/ext/raindrops/linux_tcp_info.c
>
> I haven't used it, much, but FreeBSD also implements a subset of
> this feature, nowadays, and ought to be supportable, too.  We
> can look into adding it for raindrops.

Cool, I think it makes sense to use Raindrops here, advantage being
it'd use the actual struct instead of a sketchy `#unpack`.

I meant to ask, in Raindrops why do you use the netlink API to get the
socket backlog instead of `getsockopt(2)` with `TCP_INFO` to get
`tcpi_unacked`? (as described in
http://www.ryanfrantz.com/posts/apache-tcp-backlog/) We use this to
monitor socket backlogs with a sidekick Ruby daemon. Although we're
looking to replace it with a middleware to simplify for Kubernetes.
It's one of our main metrics for monitoring performance, especially
around deploys.

>
> I don't know about other BSDs...
>
>> 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.
>
> I guess hijack means you have to discard other middlewares and
> the normal rack response handling in unicorn?  If so, ouch!
>
> Unfortunately, without hijack, there's no portable way in Rack
> to get at the underlying IO object; so I guess this needs to
> be done at the server level...
>
> So yeah, I guess it belongs in the webserver.

I was going to use `env["unicorn.socket"]`/`env["puma.socket"]`, but
you could also do `env.delete("hijack_io")` after hijacking to allow
Unicorn to still render the response. Unfortunately the
`<webserver>.socket` key is not part of the Rack standard, so I'm
hesitant to use it. When this gets into Unicorn I'm planning to
propose it upstream to Puma as well.

>
> I think we can automatically use TCP_INFO if available for
> check_client_connection; then fall back to the old early write
> hack for Unix sockets and systems w/o TCP_INFO (which would set
> the response_start_sent flag).
>
> No need to introduce yet another configuration option.

Cool. How would you suggest I check for TCP_INFO compatible platforms
in Unicorn? Is `RUBY_PLATFORM.ends_with?("linux".freeze)` sufficient
or do you prefer another mechanism? I agree that we should fall back
to the write hack on other platforms.

^ permalink raw reply	[relevance 65%]

* Re: check_client_connection using getsockopt(2)
  2017-02-22 20:09 65%   ` Simon Eskildsen
@ 2017-02-23  1:42 86%     ` Eric Wong
  2017-02-23  2:42 89%       ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
From: Eric Wong @ 2017-02-23  1:42 UTC (permalink / raw)
  To: Simon Eskildsen; +Cc: unicorn-public, raindrops-public

Simon Eskildsen <simon.eskildsen@shopify.com> wrote:

<snip> Thanks for the writeup!

Another sidenote: It seems nginx <-> unicorn is a bit odd
for deployment in a containerized environment(*).

> I meant to ask, in Raindrops why do you use the netlink API to get the
> socket backlog instead of `getsockopt(2)` with `TCP_INFO` to get
> `tcpi_unacked`? (as described in
> http://www.ryanfrantz.com/posts/apache-tcp-backlog/) We use this to
> monitor socket backlogs with a sidekick Ruby daemon. Although we're
> looking to replace it with a middleware to simplify for Kubernetes.
> It's one of our main metrics for monitoring performance, especially
> around deploys.

The netlink API allows independently-spawned processes to
monitor others; so it can be system-wide.  TCP_INFO requires the
process doing the checking to also have the socket open.

I guess this reasoning for using netlink is invalid for containers,
though...

> I was going to use `env["unicorn.socket"]`/`env["puma.socket"]`, but
> you could also do `env.delete("hijack_io")` after hijacking to allow
> Unicorn to still render the response. Unfortunately the
> `<webserver>.socket` key is not part of the Rack standard, so I'm
> hesitant to use it. When this gets into Unicorn I'm planning to
> propose it upstream to Puma as well.

I was going to say env.delete("rack.hijack_io") is dangerous
(since env could be copied by middleware); but apparently not:
rack.hijack won't work with a copied env, either.
You only need to delete with the same env object you call
rack.hijack with.

But calling rack.hijack followed by env.delete may still
have unintended side-effects in other servers; so I guess
we (again) cannot rely on hijack working portably.

> Cool. How would you suggest I check for TCP_INFO compatible platforms
> in Unicorn? Is `RUBY_PLATFORM.ends_with?("linux".freeze)` sufficient
> or do you prefer another mechanism? I agree that we should fall back
> to the write hack on other platforms.

The Raindrops::TCP_Info class should be undefined on unsupported
platforms, so I think you can check for that, instead.  Then it
should be transparent to add FreeBSD support from unicorn's
perspective.


(*) So I've been wondering if adding a "unicorn-mode" to an
    existing C10K, slow-client-capable Ruby/Rack server +
    reverse proxy makes sense for containerized deploys.
    Maybe...

^ permalink raw reply	[relevance 86%]

* Re: check_client_connection using getsockopt(2)
  2017-02-23  1:42 86%     ` Eric Wong
@ 2017-02-23  2:42 89%       ` Simon Eskildsen
  2017-02-23  3:52 83%         ` Eric Wong
  0 siblings, 1 reply; 30+ results
From: Simon Eskildsen @ 2017-02-23  2:42 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public, raindrops-public

On Wed, Feb 22, 2017 at 8:42 PM, Eric Wong <e@80x24.org> wrote:
> Simon Eskildsen <simon.eskildsen@shopify.com> wrote:
>
> <snip> Thanks for the writeup!
>
> Another sidenote: It seems nginx <-> unicorn is a bit odd
> for deployment in a containerized environment(*).
>
>> I meant to ask, in Raindrops why do you use the netlink API to get the
>> socket backlog instead of `getsockopt(2)` with `TCP_INFO` to get
>> `tcpi_unacked`? (as described in
>> http://www.ryanfrantz.com/posts/apache-tcp-backlog/) We use this to
>> monitor socket backlogs with a sidekick Ruby daemon. Although we're
>> looking to replace it with a middleware to simplify for Kubernetes.
>> It's one of our main metrics for monitoring performance, especially
>> around deploys.
>
> The netlink API allows independently-spawned processes to
> monitor others; so it can be system-wide.  TCP_INFO requires the
> process doing the checking to also have the socket open.
>
> I guess this reasoning for using netlink is invalid for containers,
> though...

If you namespace the network it's problematic, yeah. I'm considering
right now putting Raindrops in a middleware with the netlink API
inside the container, but it feels weird. That said, if you consider
the alternative of using `getsockopt(2)` on the listening socket, I
don't know how you'd get access to the Unicorn listening socket from a
middleware. Would it be nuts to expose a hook in Unicorn that allows
periodic execution for monitoring listening stats from Raindrops on
the listening socket? It seems somewhat of a narrow use-case, but on
the other hand I'm also not a fan of doing
`Raindrops::Linux.tcp_listener_stats("localhost:#{ENV["PORT"}")`, but
that might be less ugly.

>
>> I was going to use `env["unicorn.socket"]`/`env["puma.socket"]`, but
>> you could also do `env.delete("hijack_io")` after hijacking to allow
>> Unicorn to still render the response. Unfortunately the
>> `<webserver>.socket` key is not part of the Rack standard, so I'm
>> hesitant to use it. When this gets into Unicorn I'm planning to
>> propose it upstream to Puma as well.
>
> I was going to say env.delete("rack.hijack_io") is dangerous
> (since env could be copied by middleware); but apparently not:
> rack.hijack won't work with a copied env, either.
> You only need to delete with the same env object you call
> rack.hijack with.
>
> But calling rack.hijack followed by env.delete may still
> have unintended side-effects in other servers; so I guess
> we (again) cannot rely on hijack working portably.

Exactly, it gives the illusion of portability but e.g. Puma stores an
instance variable to check whether a middleware hijacked, rendering
the `env#delete` option useless.

>
>> Cool. How would you suggest I check for TCP_INFO compatible platforms
>> in Unicorn? Is `RUBY_PLATFORM.ends_with?("linux".freeze)` sufficient
>> or do you prefer another mechanism? I agree that we should fall back
>> to the write hack on other platforms.
>
> The Raindrops::TCP_Info class should be undefined on unsupported
> platforms, so I think you can check for that, instead.  Then it
> should be transparent to add FreeBSD support from unicorn's
> perspective.

Perfect. I'll start working on a patch.

>
>
> (*) So I've been wondering if adding a "unicorn-mode" to an
>     existing C10K, slow-client-capable Ruby/Rack server +
>     reverse proxy makes sense for containerized deploys.
>     Maybe...

I'd love to hear more about this idea. What are you contemplating?

^ permalink raw reply	[relevance 89%]

* Re: check_client_connection using getsockopt(2)
  2017-02-23  2:42 89%       ` Simon Eskildsen
@ 2017-02-23  3:52 83%         ` Eric Wong
  0 siblings, 0 replies; 30+ results
From: Eric Wong @ 2017-02-23  3:52 UTC (permalink / raw)
  To: Simon Eskildsen; +Cc: unicorn-public, raindrops-public

Simon Eskildsen <simon.eskildsen@shopify.com> wrote:
> On Wed, Feb 22, 2017 at 8:42 PM, Eric Wong <e@80x24.org> wrote:
> > Simon Eskildsen <simon.eskildsen@shopify.com> wrote:
> >> I meant to ask, in Raindrops why do you use the netlink API to get the
> >> socket backlog instead of `getsockopt(2)` with `TCP_INFO` to get
> >> `tcpi_unacked`? (as described in
> >> http://www.ryanfrantz.com/posts/apache-tcp-backlog/) We use this to
> >> monitor socket backlogs with a sidekick Ruby daemon. Although we're
> >> looking to replace it with a middleware to simplify for Kubernetes.
> >> It's one of our main metrics for monitoring performance, especially
> >> around deploys.
> >
> > The netlink API allows independently-spawned processes to
> > monitor others; so it can be system-wide.  TCP_INFO requires the
> > process doing the checking to also have the socket open.
> >
> > I guess this reasoning for using netlink is invalid for containers,
> > though...
> 
> If you namespace the network it's problematic, yeah. I'm considering
> right now putting Raindrops in a middleware with the netlink API
> inside the container, but it feels weird. That said, if you consider
> the alternative of using `getsockopt(2)` on the listening socket, I
> don't know how you'd get access to the Unicorn listening socket from a
> middleware. Would it be nuts to expose a hook in Unicorn that allows
> periodic execution for monitoring listening stats from Raindrops on
> the listening socket? It seems somewhat of a narrow use-case, but on
> the other hand I'm also not a fan of doing
> `Raindrops::Linux.tcp_listener_stats("localhost:#{ENV["PORT"}")`, but
> that might be less ugly.

Yeah, all options get pretty ugly since Rack doesn't expose that
in a standardized way.  Unicorn::HttpServer::LISTENERS is a
historical constant which stores all listeners used by some
parts of raindrops.

Maybe relying on ObjectSpace or walking the LISTEN_FDS env from
systemd is acceptable for other servers *shrug*


Another way might be to rely on tcpi_last_data_recv in struct
tcp_info from each and every client socket.  That should tell
you when a client stopped writing to the socket, which works for
most HTTP requests.  You could use the same getsockopt() call
you'd use for check_client_connection to read that field.

However, this won't be visible until the client is accept()ed.

raindrops actually has some support for this, but it was
ugly, too (hooking into *accept* methods):
https://bogomips.org/raindrops/Raindrops/LastDataRecv.html

Perhaps refinements could be used, nowadays (I've never used
them).  Just throwing options out there...


In any case, what I definitely do not want is to introduced more
specialized configuration or API which might lock people into
unicorn or having to burden people with versioning incompatibilies.

> > (*) So I've been wondering if adding a "unicorn-mode" to an
> >     existing C10K, slow-client-capable Ruby/Rack server +
> >     reverse proxy makes sense for containerized deploys.
> >     Maybe...
> 
> I'd love to hear more about this idea. What are you contemplating?

Maybe another time, and not on the unicorn list.  I don't feel
comfortable writing about unrelated/tangential projects on the
unicorn list, even if I'm the leader of both projects.  Anyways,
this other server is also in the rack README.md and I've been
announcing it on ruby-talk since 2013.

^ permalink raw reply	[relevance 83%]

* [PATCH] check_client_connection: use tcp state on linux
@ 2017-02-25 14:03 79% Simon Eskildsen
  2017-02-25 16:19 99% ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
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	[relevance 79%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-02-25 14:03 79% [PATCH] check_client_connection: use tcp state on linux Simon Eskildsen
@ 2017-02-25 16:19 99% ` Simon Eskildsen
  2017-02-25 23:12 84%   ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 99%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-02-25 16:19 99% ` Simon Eskildsen
@ 2017-02-25 23:12 84%   ` Eric Wong
  2017-02-27 11:44 55%     ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
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	[relevance 84%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-02-25 23:12 84%   ` Eric Wong
@ 2017-02-27 11:44 55%     ` Simon Eskildsen
  2017-02-28 21:12 88%       ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 55%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-02-27 11:44 55%     ` Simon Eskildsen
@ 2017-02-28 21:12 88%       ` Eric Wong
  2017-03-01  3:18 96%         ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 88%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-02-28 21:12 88%       ` Eric Wong
@ 2017-03-01  3:18 96%         ` Eric Wong
  2017-03-06 21:32 43%           ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
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	[relevance 96%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-01  3:18 96%         ` Eric Wong
@ 2017-03-06 21:32 43%           ` Simon Eskildsen
  2017-03-07 22:50 83%             ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 43%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-06 21:32 43%           ` Simon Eskildsen
@ 2017-03-07 22:50 83%             ` Eric Wong
  2017-03-08  0:26 99%               ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 83%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-07 22:50 83%             ` Eric Wong
@ 2017-03-08  0:26 99%               ` Eric Wong
  2017-03-08 12:06 99%                 ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
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	[relevance 99%]

* [PATCH 0/3] TCP_INFO check_client_connection followups
@ 2017-03-08  6:02 81% Eric Wong
  2017-03-08  6:02 72% ` [PATCH 1/3] new test for check_client_connection Eric Wong
  2017-03-08 10:14 99% ` [PATCH 0/3] TCP_INFO check_client_connection followups Simon Eskildsen
  0 siblings, 2 replies; 30+ results
From: Eric Wong @ 2017-03-08  6:02 UTC (permalink / raw)
  To: unicorn-public; +Cc: Simon Eskildsen

This series goes on top of Simon's excellent work to get
a TCP_INFO-based check_client_connection working under Linux.

First off, add a test extracted from one of Simon's messages;
then revert the signature changes to existing methods to
avoid breaking tmm1/gctools, and finally add a more portable
fallback for Ruby 2.2+ users (tested on FreeBSD).

Further work will improve portability of raindrops for FreeBSD
(and maybe other *BSDs incidentally, too).  That will be sent to
raindrops-public@bogomips => https://bogomips.org/raindrops-public/

Eric Wong (3):
  new test for check_client_connection
  revert signature change to HttpServer#process_client
  support "struct tcp_info" on non-Linux and Ruby 2.2+

 lib/unicorn/http_request.rb  | 42 +++++++++++++++++++----
 lib/unicorn/http_server.rb   |  6 ++--
 lib/unicorn/oob_gc.rb        |  4 +--
 lib/unicorn/socket_helper.rb | 16 +++++++--
 test/unit/test_ccc.rb        | 81 ++++++++++++++++++++++++++++++++++++++++++++
 test/unit/test_request.rb    | 28 +++++++--------
 6 files changed, 150 insertions(+), 27 deletions(-)
 create mode 100644 test/unit/test_ccc.rb

Also pushed to the ccc-tcp-v3 branch:

The following changes since commit 73e1ce827faad59bfcaff0bc758c8255a5e4f747:

  t0011-active-unix-socket.sh: fix race condition in test (2017-03-01 00:24:04 +0000)

are available in the git repository at:

  git://bogomips.org/unicorn ccc-tcp-v3

for you to fetch changes up to 873218e317773462be2a72556d26dc4a723cc7be:

  support "struct tcp_info" on non-Linux and Ruby 2.2+ (2017-03-08 05:39:55 +0000)

----------------------------------------------------------------
Eric Wong (3):
      new test for check_client_connection
      revert signature change to HttpServer#process_client
      support "struct tcp_info" on non-Linux and Ruby 2.2+

Simon Eskildsen (1):
      check_client_connection: use tcp state on linux

 lib/unicorn/http_request.rb  | 73 ++++++++++++++++++++++++++++++++++++---
 lib/unicorn/socket_helper.rb | 16 +++++++--
 test/unit/test_ccc.rb        | 81 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+), 7 deletions(-)
 create mode 100644 test/unit/test_ccc.rb

-- 
EW

^ permalink raw reply	[relevance 81%]

* [PATCH 1/3] new test for check_client_connection
  2017-03-08  6:02 81% [PATCH 0/3] TCP_INFO check_client_connection followups Eric Wong
@ 2017-03-08  6:02 72% ` Eric Wong
  2017-03-08 10:14 99% ` [PATCH 0/3] TCP_INFO check_client_connection followups Simon Eskildsen
  1 sibling, 0 replies; 30+ results
From: Eric Wong @ 2017-03-08  6:02 UTC (permalink / raw)
  To: unicorn-public; +Cc: Simon Eskildsen

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 <simon.eskildsen@shopify.com>:
  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

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
-- 
EW


^ permalink raw reply related	[relevance 72%]

* Re: [PATCH 0/3] TCP_INFO check_client_connection followups
  2017-03-08  6:02 81% [PATCH 0/3] TCP_INFO check_client_connection followups Eric Wong
  2017-03-08  6:02 72% ` [PATCH 1/3] new test for check_client_connection Eric Wong
@ 2017-03-08 10:14 99% ` Simon Eskildsen
  1 sibling, 0 replies; 30+ results
From: Simon Eskildsen @ 2017-03-08 10:14 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public

This looks great Eric. Thanks for taking this to the finish line!

On Wed, Mar 8, 2017 at 1:02 AM, Eric Wong <e@80x24.org> wrote:
> This series goes on top of Simon's excellent work to get
> a TCP_INFO-based check_client_connection working under Linux.
>
> First off, add a test extracted from one of Simon's messages;
> then revert the signature changes to existing methods to
> avoid breaking tmm1/gctools, and finally add a more portable
> fallback for Ruby 2.2+ users (tested on FreeBSD).
>
> Further work will improve portability of raindrops for FreeBSD
> (and maybe other *BSDs incidentally, too).  That will be sent to
> raindrops-public@bogomips => https://bogomips.org/raindrops-public/
>
> Eric Wong (3):
>   new test for check_client_connection
>   revert signature change to HttpServer#process_client
>   support "struct tcp_info" on non-Linux and Ruby 2.2+
>
>  lib/unicorn/http_request.rb  | 42 +++++++++++++++++++----
>  lib/unicorn/http_server.rb   |  6 ++--
>  lib/unicorn/oob_gc.rb        |  4 +--
>  lib/unicorn/socket_helper.rb | 16 +++++++--
>  test/unit/test_ccc.rb        | 81 ++++++++++++++++++++++++++++++++++++++++++++
>  test/unit/test_request.rb    | 28 +++++++--------
>  6 files changed, 150 insertions(+), 27 deletions(-)
>  create mode 100644 test/unit/test_ccc.rb
>
> Also pushed to the ccc-tcp-v3 branch:
>
> The following changes since commit 73e1ce827faad59bfcaff0bc758c8255a5e4f747:
>
>   t0011-active-unix-socket.sh: fix race condition in test (2017-03-01 00:24:04 +0000)
>
> are available in the git repository at:
>
>   git://bogomips.org/unicorn ccc-tcp-v3
>
> for you to fetch changes up to 873218e317773462be2a72556d26dc4a723cc7be:
>
>   support "struct tcp_info" on non-Linux and Ruby 2.2+ (2017-03-08 05:39:55 +0000)
>
> ----------------------------------------------------------------
> Eric Wong (3):
>       new test for check_client_connection
>       revert signature change to HttpServer#process_client
>       support "struct tcp_info" on non-Linux and Ruby 2.2+
>
> Simon Eskildsen (1):
>       check_client_connection: use tcp state on linux
>
>  lib/unicorn/http_request.rb  | 73 ++++++++++++++++++++++++++++++++++++---
>  lib/unicorn/socket_helper.rb | 16 +++++++--
>  test/unit/test_ccc.rb        | 81 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 163 insertions(+), 7 deletions(-)
>  create mode 100644 test/unit/test_ccc.rb
>
> --
> EW

^ permalink raw reply	[relevance 99%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-08  0:26 99%               ` Eric Wong
@ 2017-03-08 12:06 99%                 ` Simon Eskildsen
  2017-03-13 20:16 99%                   ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
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	[relevance 99%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-08 12:06 99%                 ` Simon Eskildsen
@ 2017-03-13 20:16 99%                   ` Simon Eskildsen
  2017-03-13 20:37 99%                     ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 99%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-13 20:16 99%                   ` Simon Eskildsen
@ 2017-03-13 20:37 99%                     ` Eric Wong
  2017-03-14 16:14 99%                       ` Simon Eskildsen
  0 siblings, 1 reply; 30+ results
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	[relevance 99%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-13 20:37 99%                     ` Eric Wong
@ 2017-03-14 16:14 99%                       ` Simon Eskildsen
  2017-03-14 16:41 99%                         ` Eric Wong
  0 siblings, 1 reply; 30+ results
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	[relevance 99%]

* Re: [PATCH] check_client_connection: use tcp state on linux
  2017-03-14 16:14 99%                       ` Simon Eskildsen
@ 2017-03-14 16:41 99%                         ` Eric Wong
  0 siblings, 0 replies; 30+ results
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	[relevance 99%]

* [PATCH ccc-tcp-v3] http_request: reduce insn size for check_client_connection
@ 2017-03-14 19:20 91% Eric Wong
  0 siblings, 0 replies; 30+ results
From: Eric Wong @ 2017-03-14 19:20 UTC (permalink / raw)
  To: Simon Eskildsen; +Cc: unicorn-public

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.
---
  Also pushed to git://bogomips.org/unicorn ccc-tcp-v3

 lib/unicorn/http_request.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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 @@ def hijacked?
   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
-- 
EW

^ permalink raw reply related	[relevance 91%]

Results 1-30 of 30 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2012-10-30 20:40     Combating nginx 499 HTTP responses during flash traffic scenario Tom Burns
2012-10-30 21:37     ` Eric Wong
2012-11-02 17:59       ` Tom Burns
2012-11-02 19:38         ` Eric Wong
2012-11-03 22:45           ` Tom Burns
2012-11-05 11:48             ` Eric Wong
2012-11-06  3:16               ` Tom Burns
2012-11-06 21:23                 ` Eric Wong
2012-11-29 15:52                   ` Tom Burns
2012-11-29 21:19                     ` Eric Wong
2012-11-29 21:55 73%                   ` [RFC/PATCH] check_client_connection: document local-only requirement Eric Wong
2012-11-29 23:47 99%                     ` Tom Burns
2012-11-30  0:07 93% [ANN] unicorn 4.5.0pre1 - check_client_connection option Eric Wong
2012-12-08  0:17 89% [ANN] unicorn 4.5.0 (final) " Eric Wong
2014-02-08  7:48 99% non-Linux check_client_connection users? Eric Wong
2017-02-15 18:45 99% response_start_sent (check_client_connection) is staying Eric Wong
2017-02-22 12:02 62% check_client_connection using getsockopt(2) Simon Eskildsen
2017-02-22 18:33 81% ` Eric Wong
2017-02-22 20:09 65%   ` Simon Eskildsen
2017-02-23  1:42 86%     ` Eric Wong
2017-02-23  2:42 89%       ` Simon Eskildsen
2017-02-23  3:52 83%         ` Eric Wong
2017-02-25 14:03 79% [PATCH] check_client_connection: use tcp state on linux Simon Eskildsen
2017-02-25 16:19 99% ` Simon Eskildsen
2017-02-25 23:12 84%   ` Eric Wong
2017-02-27 11:44 55%     ` Simon Eskildsen
2017-02-28 21:12 88%       ` Eric Wong
2017-03-01  3:18 96%         ` Eric Wong
2017-03-06 21:32 43%           ` Simon Eskildsen
2017-03-07 22:50 83%             ` Eric Wong
2017-03-08  0:26 99%               ` Eric Wong
2017-03-08 12:06 99%                 ` Simon Eskildsen
2017-03-13 20:16 99%                   ` Simon Eskildsen
2017-03-13 20:37 99%                     ` Eric Wong
2017-03-14 16:14 99%                       ` Simon Eskildsen
2017-03-14 16:41 99%                         ` Eric Wong
2017-03-08  6:02 81% [PATCH 0/3] TCP_INFO check_client_connection followups Eric Wong
2017-03-08  6:02 72% ` [PATCH 1/3] new test for check_client_connection Eric Wong
2017-03-08 10:14 99% ` [PATCH 0/3] TCP_INFO check_client_connection followups Simon Eskildsen
2017-03-14 19:20 91% [PATCH ccc-tcp-v3] http_request: reduce insn size for check_client_connection 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).