unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / Atom feed
* [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on
       [not found] <20170316031652.17433-1-e@80x24.org>
@ 2017-03-21  2:55 ` Eric Wong
  2017-03-21  7:50   ` Simon Eskildsen
  2017-03-21 18:48   ` Jeremy Evans
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Wong @ 2017-03-21  2:55 UTC (permalink / raw)
  To: Simon Eskildsen, unicorn-public; +Cc: raindrops-public, Jeremy Evans

TCP states names/numbers seem stable for each OS, but differ in
name and numeric values between them.  So I started upon this
patch series for raindrops last week:

  https://bogomips.org/raindrops-public/20170316031652.17433-1-e@80x24.org/T/

And things seem to be alright, for the most part.  Anyways
here's a proposed patch for unicorn which takes advantage of
the above (proposed) changes for raindrops and will allow
unicorn to support check_client_connection

Also Cc:-ing Jeremy to see if he has any input on the OpenBSD
side of things.

This goes on top of commit 20c66dbf1ebd0ca993e7a79c9d0d833d747df358
("http_request: reduce insn size for check_client_connection")
at: git://bogomips.org/unicorn ccc-tcp-v3

-------8<--------
Subject: [PATCH] http_request: support proposed Raindrops::TCP states on
 non-Linux

raindrops 0.18+ will have Raindrops::TCP state hash for portable
mapping of TCP states to their respective numeric values.  This
was necessary because TCP state numbers (and even macro names)
differ between FreeBSD and Linux (and possibly other OSes).

Favor using the Raindrops::TCP state hash if available, but
fall back to the hard-coded values since older versions of
raindrops did not support TCP_INFO on non-Linux systems.

While we're in the area, favor "const_defined?" over "defined?"
to reduce the inline constant cache footprint for branches
which are only evaluated once.

Patches to implement Raindrops::TCP for FreeBSD are available at:

  https://bogomips.org/raindrops-public/20170316031652.17433-1-e@80x24.org/T/
---
 lib/unicorn/http_request.rb | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index 9010007..7253497 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -105,7 +105,7 @@ def hijacked?
     env.include?('rack.hijack_io'.freeze)
   end
 
-  if defined?(Raindrops::TCP_Info)
+  if Raindrops.const_defined?(:TCP_Info)
     TCPI = Raindrops::TCP_Info.allocate
 
     def check_client_connection(socket) # :nodoc:
@@ -118,14 +118,34 @@ def check_client_connection(socket) # :nodoc:
       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
+    if Raindrops.const_defined?(:TCP)
+      # raindrops 0.18.0+ supports FreeBSD + Linux using the same names
+      # Evaluate these hash lookups at load time so we can
+      # generate an opt_case_dispatch instruction
+      eval <<-EOS
+      def closed_state?(state) # :nodoc:
+        case state
+        when #{Raindrops::TCP[:ESTABLISHED]}
+          false
+        when #{Raindrops::TCP.values_at(
+              :CLOSE_WAIT, :TIME_WAIT, :CLOSE, :LAST_ACK, :CLOSING).join(',')}
+          true
+        else
+          false
+        end
+      end
+      EOS
+    else
+      # raindrops before 0.18 only supported TCP_INFO under Linux
+      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
     end
   else
-- 
EW

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on
  2017-03-21  2:55 ` [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on Eric Wong
@ 2017-03-21  7:50   ` Simon Eskildsen
  2017-03-21  8:19     ` Eric Wong
  2017-03-21 18:48   ` Jeremy Evans
  1 sibling, 1 reply; 5+ messages in thread
From: Simon Eskildsen @ 2017-03-21  7:50 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public, raindrops-public, Jeremy Evans

This looks good to me.

Only question is, why not make Raindrops 0.18+ a requirement to avoid
the Raindrops.const_defined?(:TCP)?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on
  2017-03-21  7:50   ` Simon Eskildsen
@ 2017-03-21  8:19     ` Eric Wong
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Wong @ 2017-03-21  8:19 UTC (permalink / raw)
  To: Simon Eskildsen; +Cc: unicorn-public, raindrops-public, Jeremy Evans

Simon Eskildsen <simon.eskildsen@shopify.com> wrote:
> This looks good to me.

Thanks for taking a look.

> Only question is, why not make Raindrops 0.18+ a requirement to avoid
> the Raindrops.const_defined?(:TCP)?

I'd rather have some wiggle room in case problems are found on
either side; so people can rollback one without affecting the
other(*).

It also relaxes things for distro packagers for coordinating
releases; in case there's other dependencies (whether human or
technical) which slow down the process.


(*) We may drop raindrops as a hard requirement for unicorn at
    some point, too.  The intended use case for counter sharing
    across hundredes/thousands of workers on massively multicore
    CPUs never surfaced.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on
  2017-03-21  2:55 ` [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on Eric Wong
  2017-03-21  7:50   ` Simon Eskildsen
@ 2017-03-21 18:48   ` Jeremy Evans
  2017-03-21 19:12     ` Eric Wong
  1 sibling, 1 reply; 5+ messages in thread
From: Jeremy Evans @ 2017-03-21 18:48 UTC (permalink / raw)
  To: Eric Wong; +Cc: Simon Eskildsen, unicorn-public, raindrops-public

On 03/21 02:55, Eric Wong wrote:
> TCP states names/numbers seem stable for each OS, but differ in
> name and numeric values between them.  So I started upon this
> patch series for raindrops last week:
> 
>   https://bogomips.org/raindrops-public/20170316031652.17433-1-e@80x24.org/T/
> 
> And things seem to be alright, for the most part.  Anyways
> here's a proposed patch for unicorn which takes advantage of
> the above (proposed) changes for raindrops and will allow
> unicorn to support check_client_connection
> 
> Also Cc:-ing Jeremy to see if he has any input on the OpenBSD
> side of things.

OpenBSD seems to support the constants you are using in raindrops:

#define TCPS_CLOSED             0       /* closed */
#define TCPS_LISTEN             1       /* listening for connection */
#define TCPS_SYN_SENT           2       /* active, have sent syn */
#define TCPS_SYN_RECEIVED       3       /* have sent and received syn */
#define TCPS_ESTABLISHED        4       /* established */
#define TCPS_CLOSE_WAIT         5       /* rcvd fin, waiting for close */
#define TCPS_FIN_WAIT_1         6       /* have closed, sent fin */
#define TCPS_CLOSING            7       /* closed xchd FIN; await ACK */
#define TCPS_LAST_ACK           8       /* had fin and close; await FIN ACK */
#define TCPS_FIN_WAIT_2         9       /* have closed, fin is acked */
#define TCPS_TIME_WAIT          10      /* in 2*msl quiet wait after close */

I'm fine with dropping raindrops as a unicorn dependency and making it
optional if it isn't necessary for correct operation.

Thanks,
Jeremy

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on
  2017-03-21 18:48   ` Jeremy Evans
@ 2017-03-21 19:12     ` Eric Wong
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Wong @ 2017-03-21 19:12 UTC (permalink / raw)
  To: Jeremy Evans; +Cc: Simon Eskildsen, unicorn-public, raindrops-public

Jeremy Evans <code@jeremyevans.net> wrote:
> OpenBSD seems to support the constants you are using in raindrops:

Thanks; I expected the commonality with FreeBSD; and I expect
NetBSD and DragonflyBSD to be identical, too.

> I'm fine with dropping raindrops as a unicorn dependency and making it
> optional if it isn't necessary for correct operation.

Alright, it might be in a further-off release, though.
(unless you or somebody wants to accelerate it's removal)

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20170316031652.17433-1-e@80x24.org>
2017-03-21  2:55 ` [PATCH (ccc)] http_request: support proposed Raindrops::TCP states on Eric Wong
2017-03-21  7:50   ` Simon Eskildsen
2017-03-21  8:19     ` Eric Wong
2017-03-21 18:48   ` Jeremy Evans
2017-03-21 19:12     ` Eric Wong

unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/unicorn-public
	git clone --mirror http://ou63pmih66umazou.onion/unicorn-public

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.unicorn

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox