* [ANN] unicorn 5.3.0 - Rack HTTP server for fast clients and Unix
@ 2017-04-01 8:08 4% Eric Wong
0 siblings, 0 replies; 18+ results
From: Eric Wong @ 2017-04-01 8:08 UTC (permalink / raw)
To: ruby-talk, unicorn-public
Cc: Jeremy Evans, Simon Eskildsen, Dylan Thacker-Smith
unicorn is an HTTP server for Rack applications designed to only serve
fast clients on low-latency, high-bandwidth connections and take
advantage of features in Unix/Unix-like kernels. Slow clients should
only be served by placing a reverse proxy capable of fully buffering
both the the request and response in between unicorn and slow clients.
* https://bogomips.org/unicorn/
* public list: unicorn-public@bogomips.org
* mail archives: https://bogomips.org/unicorn-public/
* git clone git://bogomips.org/unicorn.git
* https://bogomips.org/unicorn/NEWS.atom.xml
* nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
Changes:
unicorn 5.3.0
A couple of portability fixes from Dylan Thacker-Smith and
Jeremy Evans since 5.3.0.pre1 over a week ago, but this looks
ready for a stable release, today.
When I started this over 8 years ago, I wondered if this would
just end up being an April Fools' joke. Guess not. I guess I
somehow tricked people into using a terribly marketed web server
that cannot talk directly to untrusted clients :x Anyways,
unicorn won't be able to handle slow clients 8 years from now,
either, or 80 years from now. And I vow never to learn to use
new-fangled things like epoll, kqueue, or threads :P
Anyways, this is a largish release with several new features,
and no backwards incompatibilities.
Simon Eskildsen contributed heavily using TCP_INFO under Linux
to implement the (now 5 year old) check_client_connection feature:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-check_client_connection
https://bogomips.org/unicorn-public/?q=s:check_client_connection&d:..20170401&x=t
This also led to FreeBSD and OpenBSD portability improvements in
one of our dependencies, raindrops:
https://bogomips.org/raindrops-public/20170323024829.GA5190@dcvr/T/#u
Jeremy Evans contributed several new features. First he
implemented after_worker_exit to aid debugging:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-after_worker_exit
https://bogomips.org/unicorn-public/?q=s:after_worker_exit&d:..20170401&x=t#t
And then security-related features to isolate workers. Workers
may now chroot to drop access to the master filesystem, and the
new after_worker_ready configuration hook now exists to aid with
chroot support in workers:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-after_worker_ready
https://bogomips.org/unicorn/Unicorn/Worker.html#method-i-user
https://bogomips.org/unicorn-public/?q=s:after_worker_ready&d:..20170401&x=t#t
https://bogomips.org/unicorn-public/?q=s:chroot&d:..20170401&x=t#t
Additionally, workers may run in a completely different VM space
(nullifying preload_app and any CoW savings) with the new
worker_exec option:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-worker_exec
https://bogomips.org/unicorn-public/?q=s:worker_exec&d:..20170401&x=t#t
There are also several improvements to FreeBSD and OpenBSD
support with the addition of these features.
shortlog of changes since v5.2.0 (2016-10-31):
Dylan Thacker-Smith (1):
Check for Socket::TCP_INFO constant before trying to get TCP_INFO
Eric Wong (30):
drop rb_str_set_len compatibility replacement
TUNING: document THP caveat for Linux users
tee_input: simplify condition for IO#write
remove response_start_sent
http_request: freeze constant strings passed IO#write
Revert "remove response_start_sent"
t/t0012-reload-empty-config.sh: access ivars directly if needed
t0011-active-unix-socket.sh: fix race condition in test
new test for check_client_connection
revert signature change to HttpServer#process_client
support "struct tcp_info" on non-Linux and Ruby 2.2+
unicorn_http: reduce rb_global_variable calls
oob_gc: rely on opt_aref_with optimization on Ruby 2.2+
http_request: reduce insn size for check_client_connection
freebsd: avoid EINVAL when setting accept filter
test-lib: expr(1) portability fix
tests: keep disabled tests defined
test_exec: SO_KEEPALIVE value only needs to be true
doc: fix links to raindrops project
http_request: support proposed Raindrops::TCP states on non-Linux
ISSUES: expand on mail archive info + subscription disclaimer
test_ccc: use a pipe to synchronize test
doc: remove private email support address
input: update documentation and hide internals.
http_server: initialize @pid ivar
gemspec: remove olddoc from build dependency
doc: add version annotations for new features
unicorn 5.3.0.pre1
doc: note after_worker_exit is also 5.3.0+
test_exec: SO_KEEPALIVE value only needs to be true (take #2)
Jeremy Evans (7):
Add after_worker_exit configuration option
Fix code example in after_worker_exit documentation
Add support for chroot to Worker#user
Add after_worker_ready configuration option
Add worker_exec configuration option
Don't pass a block for fork when forking workers
Check for SocketError on first ccc attempt
Simon Eskildsen (1):
check_client_connection: use tcp state on linux
--
Yes, this release is real despite the date.
^ permalink raw reply [relevance 4%]
* [ANN] unicorn 5.3.0.pre1 - Rack HTTP server for fast clients and Unix
@ 2017-03-24 0:28 4% Eric Wong
0 siblings, 0 replies; 18+ results
From: Eric Wong @ 2017-03-24 0:28 UTC (permalink / raw)
To: ruby-talk, unicorn-public; +Cc: Jeremy Evans, Simon Eskildsen
unicorn is an HTTP server for Rack applications designed to only serve
fast clients on low-latency, high-bandwidth connections and take
advantage of features in Unix/Unix-like kernels. Slow clients should
only be served by placing a reverse proxy capable of fully buffering
both the the request and response in between unicorn and slow clients.
* https://bogomips.org/unicorn/
* public list: unicorn-public@bogomips.org
* mail archives: https://bogomips.org/unicorn-public/
* git clone git://bogomips.org/unicorn.git
* https://bogomips.org/unicorn/NEWS.atom.xml
* nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
This is a pre-release RubyGem intended for testing.
Changes:
unicorn 5.3.0.pre1
A largish release with several new features.
Simon Eskildsen contributed heavily using TCP_INFO under Linux
to implement the (now 5 year old) check_client_connection feature:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-check_client_connection
https://bogomips.org/unicorn-public/?q=s:check_client_connection&d:..20170324&x=t
This also led to FreeBSD and OpenBSD portability improvements in
one of our dependencies, raindrops:
https://bogomips.org/raindrops-public/20170323024829.GA5190@dcvr/T/#u
Jeremy Evans contributed several new features. First he
implemented after_worker_exit to aid debugging:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-after_worker_exit
https://bogomips.org/unicorn-public/?q=s:after_worker_exit&d:..20170324&x=t#t
And then security-related features to isolate workers. Workers
may now chroot to drop access to the master filesystem, and the
new after_worker_ready configuration hook now exists to aid with
chroot support in workers:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-after_worker_ready
https://bogomips.org/unicorn/Unicorn/Worker.html#method-i-user
https://bogomips.org/unicorn-public/?q=s:after_worker_ready&d:..20170324&x=t#t
https://bogomips.org/unicorn-public/?q=s:chroot&d:..20170324&x=t#t
Additionally, workers may run in a completely different VM space
(nullifying preload_app and any CoW savings) with the new
worker_exec option:
https://bogomips.org/unicorn/Unicorn/Configurator.html#method-i-worker_exec
https://bogomips.org/unicorn-public/?q=s:worker_exec&d:..20170324&x=t#t
There are also several improvements to FreeBSD and OpenBSD
support with the addition of these features.
34 changes since 5.2.0 (2016-10-31):
Eric Wong (27):
drop rb_str_set_len compatibility replacement
TUNING: document THP caveat for Linux users
tee_input: simplify condition for IO#write
remove response_start_sent
http_request: freeze constant strings passed IO#write
Revert "remove response_start_sent"
t/t0012-reload-empty-config.sh: access ivars directly if needed
t0011-active-unix-socket.sh: fix race condition in test
new test for check_client_connection
revert signature change to HttpServer#process_client
support "struct tcp_info" on non-Linux and Ruby 2.2+
unicorn_http: reduce rb_global_variable calls
oob_gc: rely on opt_aref_with optimization on Ruby 2.2+
http_request: reduce insn size for check_client_connection
freebsd: avoid EINVAL when setting accept filter
test-lib: expr(1) portability fix
tests: keep disabled tests defined
test_exec: SO_KEEPALIVE value only needs to be true
doc: fix links to raindrops project
http_request: support proposed Raindrops::TCP states on non-Linux
ISSUES: expand on mail archive info + subscription disclaimer
test_ccc: use a pipe to synchronize test
doc: remove private email support address
input: update documentation and hide internals.
http_server: initialize @pid ivar
gemspec: remove olddoc from build dependency
doc: add version annotations for new features
Jeremy Evans (6):
Add after_worker_exit configuration option
Fix code example in after_worker_exit documentation
Add support for chroot to Worker#user
Add after_worker_ready configuration option
Add worker_exec configuration option
Don't pass a block for fork when forking workers
Simon Eskildsen (1):
check_client_connection: use tcp state on linux
--
5.3.0 in a week, maybe?
^ permalink raw reply [relevance 4%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-14 16:14 7% ` Simon Eskildsen
@ 2017-03-14 16:41 7% ` Eric Wong
0 siblings, 0 replies; 18+ 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 7%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-13 20:37 7% ` Eric Wong
@ 2017-03-14 16:14 7% ` Simon Eskildsen
2017-03-14 16:41 7% ` Eric Wong
0 siblings, 1 reply; 18+ 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 7%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-13 20:16 7% ` Simon Eskildsen
@ 2017-03-13 20:37 7% ` Eric Wong
2017-03-14 16:14 7% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 7%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-08 12:06 7% ` Simon Eskildsen
@ 2017-03-13 20:16 7% ` Simon Eskildsen
2017-03-13 20:37 7% ` Eric Wong
0 siblings, 1 reply; 18+ 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 7%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-08 0:26 7% ` Eric Wong
@ 2017-03-08 12:06 7% ` Simon Eskildsen
2017-03-13 20:16 7% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 7%]
* Re: [PATCH 0/3] TCP_INFO check_client_connection followups
2017-03-08 6:02 5% [PATCH 0/3] TCP_INFO check_client_connection followups Eric Wong
@ 2017-03-08 10:14 0% ` Simon Eskildsen
0 siblings, 0 replies; 18+ 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 0%]
* [PATCH 0/3] TCP_INFO check_client_connection followups
@ 2017-03-08 6:02 5% Eric Wong
2017-03-08 10:14 0% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 5%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-07 22:50 5% ` Eric Wong
@ 2017-03-08 0:26 7% ` Eric Wong
2017-03-08 12:06 7% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 7%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-06 21:32 3% ` Simon Eskildsen
@ 2017-03-07 22:50 5% ` Eric Wong
2017-03-08 0:26 7% ` Eric Wong
0 siblings, 1 reply; 18+ 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 5%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-03-01 3:18 6% ` Eric Wong
@ 2017-03-06 21:32 3% ` Simon Eskildsen
2017-03-07 22:50 5% ` Eric Wong
0 siblings, 1 reply; 18+ 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 3%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-02-28 21:12 6% ` Eric Wong
@ 2017-03-01 3:18 6% ` Eric Wong
2017-03-06 21:32 3% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 6%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-02-27 11:44 3% ` Simon Eskildsen
@ 2017-02-28 21:12 6% ` Eric Wong
2017-03-01 3:18 6% ` Eric Wong
0 siblings, 1 reply; 18+ 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 6%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-02-25 23:12 6% ` Eric Wong
@ 2017-02-27 11:44 3% ` Simon Eskildsen
2017-02-28 21:12 6% ` Eric Wong
0 siblings, 1 reply; 18+ 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 3%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-02-25 16:19 7% ` Simon Eskildsen
@ 2017-02-25 23:12 6% ` Eric Wong
2017-02-27 11:44 3% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 6%]
* Re: [PATCH] check_client_connection: use tcp state on linux
2017-02-25 14:03 5% [PATCH] check_client_connection: use tcp state on linux Simon Eskildsen
@ 2017-02-25 16:19 7% ` Simon Eskildsen
2017-02-25 23:12 6% ` Eric Wong
0 siblings, 1 reply; 18+ 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 7%]
* [PATCH] check_client_connection: use tcp state on linux
@ 2017-02-25 14:03 5% Simon Eskildsen
2017-02-25 16:19 7% ` Simon Eskildsen
0 siblings, 1 reply; 18+ 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 5%]
Results 1-18 of 18 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2017-02-25 14:03 5% [PATCH] check_client_connection: use tcp state on linux Simon Eskildsen
2017-02-25 16:19 7% ` Simon Eskildsen
2017-02-25 23:12 6% ` Eric Wong
2017-02-27 11:44 3% ` Simon Eskildsen
2017-02-28 21:12 6% ` Eric Wong
2017-03-01 3:18 6% ` Eric Wong
2017-03-06 21:32 3% ` Simon Eskildsen
2017-03-07 22:50 5% ` Eric Wong
2017-03-08 0:26 7% ` Eric Wong
2017-03-08 12:06 7% ` Simon Eskildsen
2017-03-13 20:16 7% ` Simon Eskildsen
2017-03-13 20:37 7% ` Eric Wong
2017-03-14 16:14 7% ` Simon Eskildsen
2017-03-14 16:41 7% ` Eric Wong
2017-03-08 6:02 5% [PATCH 0/3] TCP_INFO check_client_connection followups Eric Wong
2017-03-08 10:14 0% ` Simon Eskildsen
2017-03-24 0:28 4% [ANN] unicorn 5.3.0.pre1 - Rack HTTP server for fast clients and Unix Eric Wong
2017-04-01 8:08 4% [ANN] unicorn 5.3.0 " 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).