[PATCH 0/2] socket inheritance behavior/bug => feature!

[PATCH 0/2] socket inheritance behavior/bug => feature!

[Eric Wong]

Given long enough, some bugs need to become documented behavior
(see [PATCH 2/2]).

Anyways, I guess we'll release 5.0.0 final in a day or two.  I was
intending to release 5.0.0 today until I noticed how un-DRY having
listeners configured in both systemd and unicorn config files would
be.  And then I noticed we have an existing bug as documented in 2/2
which would allow the user to be DRY after all.

Eric Wong (2):
      sd_listen_fds emulation cleanup
      inheriting sockets from UNICORN_FD does not close them

 Documentation/unicorn.1.txt |  4 +---
 lib/unicorn/http_server.rb  |  4 ++--
 test/exec/test_exec.rb      | 50 ++++++++++++++++++++++++++++++++++-----------
 3 files changed, 41 insertions(+), 17 deletions(-)

Fwiw, in case anybody is uncomfortable with the mere mention of the
word "systemd"; unicorn will never have any dependencies on systemd.
It will merely use systemd if available.

[PATCH 1/2] sd_listen_fds emulation cleanup

[Eric Wong]

Re-enable and expand on the test case while we're at it for new
Rubies.  The bug is now fixed in Ruby 2.3.0dev as of r51576.  We
shall assume anybody running a pre-release 2.3.0 at this point is
running a fairly recent snapshot, so we won't bother doing a
finer-grained check in the test for an exact revision number.
---
 lib/unicorn/http_server.rb |  4 ++--
 test/exec/test_exec.rb     | 40 +++++++++++++++++++++++-----------------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index 3dbfd3e..c1a2e60 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -772,12 +772,12 @@ def inherit_listeners!
     sd_pid, sd_fds = ENV.values_at('LISTEN_PID', 'LISTEN_FDS')
     if sd_pid && sd_pid.to_i == $$
       # 3 = SD_LISTEN_FDS_START
-      inherited.concat((3...(3 + sd_fds.to_i)).map { |fd| Socket.for_fd(fd) })
+      inherited.concat((3...(3 + sd_fds.to_i)).to_a)
     end
     # to ease debugging, we will not unset LISTEN_PID and LISTEN_FDS
 
     inherited.map! do |fd|
-      io = String === fd ? Socket.for_fd(fd.to_i) : fd
+      io = Socket.for_fd(fd.to_i)
       io.autoclose = false
       io = server_cast(io)
       set_server_sockopt(io, listener_opts[sock_name(io)])
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index 33d768a..af6f151 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -96,31 +96,37 @@ def teardown
     end
   end
 
-  # FIXME https://bugs.ruby-lang.org/issues/11336
-  # [ruby-core:69895] [Bug #11336]
-  def disabled_test_sd_listen_fds_emulation
+  def test_sd_listen_fds_emulation
     File.open("config.ru", "wb") { |fp| fp.write(HI) }
     sock = TCPServer.new(@addr, @port)
-    sock.setsockopt(:SOL_SOCKET, :SO_KEEPALIVE, 0)
 
-    pid = xfork do
-      redirect_test_io do
-        # pretend to be systemd
-        ENV['LISTEN_PID'] = "#$$"
-        ENV['LISTEN_FDS'] = '1'
+    [ %W(-l #@addr:#@port), nil ].each do |l|
+      sock.setsockopt(:SOL_SOCKET, :SO_KEEPALIVE, 0)
+
+      pid = xfork do
+        redirect_test_io do
+          # pretend to be systemd
+          ENV['LISTEN_PID'] = "#$$"
+          ENV['LISTEN_FDS'] = '1'
 
-        # 3 = SD_LISTEN_FDS_START
-        exec($unicorn_bin, "-l", "#@addr:#@port", 3 => sock)
+          # 3 = SD_LISTEN_FDS_START
+          args = [ $unicorn_bin ]
+          args.concat(l) if l
+          args << { 3 => sock }
+          exec(*args)
+        end
       end
+      res = hit(["http://#@addr:#@port/"])
+      assert_equal [ "HI\n" ], res
+      assert_shutdown(pid)
+      assert_equal 1, sock.getsockopt(:SOL_SOCKET, :SO_KEEPALIVE).int,
+                  'unicorn should always set SO_KEEPALIVE on inherited sockets'
     end
-    res = hit(["http://#{@addr}:#{@port}/"])
-    assert_equal [ "HI\n"], res
-    assert_shutdown(pid)
-    assert_equal 1, sock.getsockopt(:SOL_SOCKET, :SO_KEEPALIVE).int,
-                 "unicorn should always set SO_KEEPALIVE on inherited sockets"
   ensure
     sock.close if sock
-  end
+    # disabled test on old Rubies: https://bugs.ruby-lang.org/issues/11336
+    # [ruby-core:69895] [Bug #11336] fixed by r51576
+  end if RUBY_VERSION.to_f >= 2.3
 
   def test_working_directory_rel_path_config_file
     other = Tempfile.new('unicorn.wd')
-- 
EW

[PATCH 2/2] inheriting sockets from UNICORN_FD does not close them

[Eric Wong]

For some reason, I thought invalid descriptors passed to UNICORN_FD
would be automatically closed by the master process; but apparently
this hasn't been the case.  On the other hand, this bug has been
around for over 6 years now and nobody noticed or cared enough to
tell us, so fixing it might break existing setups.

Since there may be users relying on this behavior, we cannot change
the behavior anymore; so update the documentation and write at test
to ensure we can never "fix" this bug at the expense of breaking
any working setups which may be out there.

Keep in mind that a before_exec hook may always be used to modify
the UNICORN_FD environment by setting the close_on_exec flag and
removing the appropriate descriptor from the environment.

I originally intended to add the ability to inherit new listeners
without a config file specification so systemd users can avoid
repeating themselves in the systemd and unicorn config files,
but apparently there is nothing to change in our code.
---
 Documentation/unicorn.1.txt |  4 +---
 test/exec/test_exec.rb      | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/unicorn.1.txt b/Documentation/unicorn.1.txt
index 193860f..5b82ad5 100644
--- a/Documentation/unicorn.1.txt
+++ b/Documentation/unicorn.1.txt
@@ -166,9 +166,7 @@ variable internally when doing transparent upgrades.
 UNICORN_FD is a comma-delimited list of one or more file descriptors
 used to implement USR2 upgrades.  Init systems may bind listen sockets
 itself and spawn unicorn with UNICORN_FD set to the file descriptor
-numbers of the listen socket(s).  The unicorn CONFIG_FILE must still
-have the inherited listen socket parameters defined as in a normal
-startup, otherwise the socket will be closed.
+numbers of the listen socket(s).
 
 # SEE ALSO
 
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index af6f151..ca0b7bc 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -128,6 +128,26 @@ def test_sd_listen_fds_emulation
     # [ruby-core:69895] [Bug #11336] fixed by r51576
   end if RUBY_VERSION.to_f >= 2.3
 
+  def test_inherit_listener_unspecified
+    File.open("config.ru", "wb") { |fp| fp.write(HI) }
+    sock = TCPServer.new(@addr, @port)
+    sock.setsockopt(:SOL_SOCKET, :SO_KEEPALIVE, 0)
+
+    pid = xfork do
+      redirect_test_io do
+        ENV['UNICORN_FD'] = sock.fileno.to_s
+        exec($unicorn_bin, sock.fileno => sock.fileno)
+      end
+    end
+    res = hit(["http://#@addr:#@port/"])
+    assert_equal [ "HI\n" ], res
+    assert_shutdown(pid)
+    assert_equal 1, sock.getsockopt(:SOL_SOCKET, :SO_KEEPALIVE).int,
+                'unicorn should always set SO_KEEPALIVE on inherited sockets'
+  ensure
+    sock.close if sock
+  end
+
   def test_working_directory_rel_path_config_file
     other = Tempfile.new('unicorn.wd')
     File.unlink(other.path)
-- 
EW