summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2012-04-12 16:46:24 -0700
committerEric Wong <normalperson@yhbt.net>2012-04-12 18:50:39 -0700
commitb26d3e2c4387707ca958cd9c63c213fc7ac558fa (patch)
tree00644868cdee0a39aee9b4c7459bd08c9d2ae3c4
parentd258653745e1c8e8fa13b95b1944729294804946 (diff)
Previously we relied on implicit socket shutdown() from the
close() syscall.  However, some Rack applications fork()
(without calling exec()), creating a potentially long-lived
reference to the underlying socket in a child process.  This
ends up causing nginx to wait on the socket shutdown when the
child process exits.

Calling shutdown() explicitly signals nginx (or whatever client)
that the unicorn worker is done with the socket, regardless of
the number of FD references to the underlying socket in
existence.

This was not an issue for applications which exec() since
FD_CLOEXEC is always set on the client socket.

Thanks to Patrick Wenger for discovering this.  Thanks to
Hongli Lai for the tip on using shutdown() as is done in
Passenger.

ref: http://mid.gmane.org/CAOG6bOTseAPbjU5LYchODqjdF3-Ez4+M8jo-D_D2Wq0jkdc4Rw@mail.gmail.com
-rw-r--r--lib/unicorn/http_server.rb1
-rw-r--r--lib/unicorn/ssl_client.rb5
-rw-r--r--t/detach.ru11
-rwxr-xr-xt/t0021-process_detach.sh29
4 files changed, 46 insertions, 0 deletions
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index ede6264..f942e2f 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -536,6 +536,7 @@ class Unicorn::HttpServer
     end
     @request.headers? or headers = nil
     http_response_write(client, status, headers, body)
+    client.shutdown # in case of fork() in Rack app
     client.close # flush and uncork socket immediately, no keepalive
   rescue => e
     handle_error(client, e)
diff --git a/lib/unicorn/ssl_client.rb b/lib/unicorn/ssl_client.rb
index 7b41cd2..a8c79e3 100644
--- a/lib/unicorn/ssl_client.rb
+++ b/lib/unicorn/ssl_client.rb
@@ -3,4 +3,9 @@
 class Unicorn::SSLClient < Kgio::SSL
   alias write kgio_write
   alias close kgio_close
+
+  # this is no-op for now, to be fixed in kgio-monkey if people care
+  # about SSL support...
+  def shutdown(how = nil)
+  end
 end
diff --git a/t/detach.ru b/t/detach.ru
new file mode 100644
index 0000000..bbd998e
--- /dev/null
+++ b/t/detach.ru
@@ -0,0 +1,11 @@
+use Rack::ContentType, "text/plain"
+fifo_path = ENV["TEST_FIFO"] or abort "TEST_FIFO not set"
+run lambda { |env|
+  pid = fork do
+    File.open(fifo_path, "wb") do |fp|
+      fp.write "HIHI"
+    end
+  end
+  Process.detach(pid)
+  [ 200, {}, [ pid.to_s ] ]
+}
diff --git a/t/t0021-process_detach.sh b/t/t0021-process_detach.sh
new file mode 100755
index 0000000..f03c497
--- /dev/null
+++ b/t/t0021-process_detach.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+. ./test-lib.sh
+
+t_plan 5 "Process.detach on forked background process works"
+
+t_begin "setup and startup" && {
+        t_fifos process_detach
+        unicorn_setup
+        TEST_FIFO=$process_detach \
+          unicorn -E none -D detach.ru -c $unicorn_config
+        unicorn_wait_start
+}
+
+t_begin "read detached PID with HTTP/1.0" && {
+        detached_pid=$(curl -0 -sSf http://$listen/)
+        t_info "detached_pid=$detached_pid"
+}
+
+t_begin "read background FIFO" && {
+        test xHIHI = x"$(cat $process_detach)"
+}
+
+t_begin "killing succeeds" && {
+        kill $unicorn_pid
+}
+
+t_begin "check stderr" && check_stderr
+
+t_done