summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2010-10-04 04:17:31 +0000
committerEric Wong <normalperson@yhbt.net>2010-10-04 20:14:13 +0000
commit8612ccdd2f7f0c0b182ecc9616a76e97b879f6c0 (patch)
treeccb42511973a71f70e663cd3d14db8124f07313a
parent206d8c77bc0ac15a5c4e7a56aeff2e9fc280e6a3 (diff)
While we've always unlinked dead sockets from nuked/leftover
processes, blindly unlinking them can cause unnecessary failures
when an active process is already listening on them.  We now
make a simple connect(2) check to ensure the socket is not in
use before unlinking it.

Thanks to Jordan Ritter for the detailed bug report leading to
this fix.

ref: http://mid.gmane.org/8D95A44B-A098-43BE-B532-7D74BD957F31@darkridge.com
(cherry picked from commit 1a2363b17b1d06be6b35d347ebcaed6a0c940200)
-rw-r--r--lib/unicorn/socket_helper.rb10
-rw-r--r--t/t0011-active-unix-socket.sh79
-rw-r--r--test/unit/test_socket_helper.rb9
3 files changed, 95 insertions, 3 deletions
diff --git a/lib/unicorn/socket_helper.rb b/lib/unicorn/socket_helper.rb
index 9a155e1..1d03eab 100644
--- a/lib/unicorn/socket_helper.rb
+++ b/lib/unicorn/socket_helper.rb
@@ -111,8 +111,14 @@ module Unicorn
       sock = if address[0] == ?/
         if File.exist?(address)
           if File.socket?(address)
-            logger.info "unlinking existing socket=#{address}"
-            File.unlink(address)
+            begin
+              UNIXSocket.new(address).close
+              # fall through, try to bind(2) and fail with EADDRINUSE
+              # (or succeed from a small race condition we can't sanely avoid).
+            rescue Errno::ECONNREFUSED
+              logger.info "unlinking existing socket=#{address}"
+              File.unlink(address)
+            end
           else
             raise ArgumentError,
                   "socket=#{address} specified but it is not a socket!"
diff --git a/t/t0011-active-unix-socket.sh b/t/t0011-active-unix-socket.sh
new file mode 100644
index 0000000..6f9ac53
--- /dev/null
+++ b/t/t0011-active-unix-socket.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+. ./test-lib.sh
+t_plan 11 "existing UNIX domain socket check"
+
+read_pid_unix () {
+        x=$(printf 'GET / HTTP/1.0\r\n\r\n' | \
+            socat - UNIX:$unix_socket | \
+            tail -1)
+        test -n "$x"
+        y="$(expr "$x" : '\([0-9]\+\)')"
+        test x"$x" = x"$y"
+        test -n "$y"
+        echo "$y"
+}
+
+t_begin "setup and start" && {
+        rtmpfiles unix_socket unix_config
+        rm -f $unix_socket
+        unicorn_setup
+        grep -v ^listen < $unicorn_config > $unix_config
+        echo "listen '$unix_socket'" >> $unix_config
+        unicorn -D -c $unix_config pid.ru
+        unicorn_wait_start
+        orig_master_pid=$unicorn_pid
+}
+
+t_begin "get pid of worker" && {
+        worker_pid=$(read_pid_unix)
+        t_info "worker_pid=$worker_pid"
+}
+
+t_begin "fails to start with existing pid file" && {
+        rm -f $ok
+        unicorn -D -c $unix_config pid.ru || echo ok > $ok
+        test x"$(cat $ok)" = xok
+}
+
+t_begin "worker pid unchanged" && {
+        test x"$(read_pid_unix)" = x$worker_pid
+        > $r_err
+}
+
+t_begin "fails to start with listening UNIX domain socket bound" && {
+        rm $ok $pid
+        unicorn -D -c $unix_config pid.ru || echo ok > $ok
+        test x"$(cat $ok)" = xok
+        > $r_err
+}
+
+t_begin "worker pid unchanged (again)" && {
+        test x"$(read_pid_unix)" = x$worker_pid
+}
+
+t_begin "nuking the existing Unicorn succeeds" && {
+        kill -9 $unicorn_pid $worker_pid
+        while kill -0 $unicorn_pid
+        do
+                sleep 1
+        done
+        check_stderr
+}
+
+t_begin "succeeds in starting with leftover UNIX domain socket bound" && {
+        test -S $unix_socket
+        unicorn -D -c $unix_config pid.ru
+        unicorn_wait_start
+}
+
+t_begin "worker pid changed" && {
+        test x"$(read_pid_unix)" != x$worker_pid
+}
+
+t_begin "killing succeeds" && {
+        kill $unicorn_pid
+}
+
+t_begin "no errors" && check_stderr
+
+t_done
diff --git a/test/unit/test_socket_helper.rb b/test/unit/test_socket_helper.rb
index bbce359..c6d0d42 100644
--- a/test/unit/test_socket_helper.rb
+++ b/test/unit/test_socket_helper.rb
@@ -101,7 +101,14 @@ class TestSocketHelper < Test::Unit::TestCase
 
   def test_bind_listen_unix_rebind
     test_bind_listen_unix
-    new_listener = bind_listen(@unix_listener_path)
+    new_listener = nil
+    assert_raises(Errno::EADDRINUSE) do
+      new_listener = bind_listen(@unix_listener_path)
+    end
+    assert_nothing_raised do
+      File.unlink(@unix_listener_path)
+      new_listener = bind_listen(@unix_listener_path)
+    end
     assert UNIXServer === new_listener
     assert new_listener.fileno != @unix_listener.fileno
     assert_equal sock_name(new_listener), sock_name(@unix_listener)