mongrel unicorn - Jun 2012 - [RFC/PATCH] FreeBSD: do not attempt to set TCP_NOPUSH to false

Can some FreeBSD users review this?  Thank you.
This issue was reported privately by a FreeBSD 8.1-RELEASE user.
>From a837437b4ca7903b3fcb90af1f257d841c9bd22f Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson at yhbt.net>
Date: Sat, 2 Jun 2012 19:29:58 +0000
Subject: [PATCH] FreeBSD: do not attempt to set TCP_NOPUSH to false

Instead of blindly turning TCP_NOPUSH off when setting up the
listen socket, only enable TCP_NOPUSH (if requested by the user)
but never disable it.

A FreeBSD 8.1-RELEASE user privately reported EADDRNOTAVAIL
errors when setting up the TCP listener due to the default
(tcp_nopush: false) value:

  ERROR -- : $HOST:$PORT{:tcp_defer_accept=>1,
:accept_filter=>"httpready", :backlog=>1024,
:tcp_nopush=>false, :tcp_nodelay=>true}: Can''t assign requested
address (Errno::EADDRNOTAVAIL)*

Additionally, the user reported the listen(backlog: 1024) value
got dropped and reset back to 5.

This unfortunately means we won''t be able disable TCP_NOPUSH
once turned on (via SIGHUP/SIGUSR2).
---
 lib/unicorn/socket_helper.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/unicorn/socket_helper.rb b/lib/unicorn/socket_helper.rb
index 21c52e3..1a6e374 100644
--- a/lib/unicorn/socket_helper.rb
+++ b/lib/unicorn/socket_helper.rb
@@ -66,7 +66,8 @@ module Unicorn
       val = val ? 1 : 0
       if defined?(TCP_CORK) # Linux
         sock.setsockopt(IPPROTO_TCP, TCP_CORK, val)
-      elsif defined?(TCP_NOPUSH) # TCP_NOPUSH is untested (FreeBSD)
+      elsif defined?(TCP_NOPUSH) && val == 1
+        # TCP_NOPUSH is lightly-tested (FreeBSD)
         sock.setsockopt(IPPROTO_TCP, TCP_NOPUSH, val)
       end
 
-- 
Eric Wong

Eric Wong <normalperson at yhbt.net> wrote:
> Can some FreeBSD users review this?  Thank you.
> This issue was reported privately by a FreeBSD 8.1-RELEASE user.
(Original (private) bug reporter Bcc-ed)

ref: http://mid.gmane.org/20120602194148.GA390 at dcvr.yhbt.net
> This unfortunately means we won''t be able disable TCP_NOPUSH
> once turned on (via SIGHUP/SIGUSR2).
I think I''ll push the following patch instead.  Some versions of
FreeBSD
(or Debian kFreeBSD) have always worked fine with TCP_NOPUSH explicitly
set (to 0/1) and I don''t want to break the case where somebody wants to
disable TCP_NOPUSH once turned on.

diff --git a/lib/unicorn/socket_helper.rb b/lib/unicorn/socket_helper.rb
index 21c52e3..18b0be7 100644
--- a/lib/unicorn/socket_helper.rb
+++ b/lib/unicorn/socket_helper.rb
@@ -28,7 +28,7 @@ module Unicorn
       :backlog => 1024,
 
       # favor latency over bandwidth savings
-      :tcp_nopush => false,
+      :tcp_nopush => nil,
       :tcp_nodelay => true,
     }
     #:startdoc:
@@ -62,12 +62,12 @@ module Unicorn
       end
 
       val = opt[:tcp_nopush]
-      val = DEFAULTS[:tcp_nopush] if nil == val
-      val = val ? 1 : 0
-      if defined?(TCP_CORK) # Linux
-        sock.setsockopt(IPPROTO_TCP, TCP_CORK, val)
-      elsif defined?(TCP_NOPUSH) # TCP_NOPUSH is untested (FreeBSD)
-        sock.setsockopt(IPPROTO_TCP, TCP_NOPUSH, val)
+      unless val.nil?
+        if defined?(TCP_CORK) # Linux
+          sock.setsockopt(IPPROTO_TCP, TCP_CORK, val)
+        elsif defined?(TCP_NOPUSH) # TCP_NOPUSH is lightly tested (FreeBSD)
+          sock.setsockopt(IPPROTO_TCP, TCP_NOPUSH, val)
+        end
       end
 
       # No good reason to ever have deferred accepts off
-- 
Eric Wong