From df76b78d359f34a73ce407f40f594577e04b014b Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Sun, 3 May 2009 16:16:48 -0700
Subject: Safer timeout handling and test case

Timeouts of less than 2 seconds are unsafe due to the lack of
subsecond resolution in most POSIX filesystems.  This is the
trade-off for using a low-complexity solution for timeouts.
Since this type of timeout is a last resort; 2 seconds is not
entirely unreasonable IMNSHO.  Additionally, timing out too
aggressively can put us in a fork loop and slow down the system.

Of course, the default is 60 seconds and most people do not
bother to change it.
---
 TODO                        |  2 --
 lib/unicorn.rb              |  3 +--
 lib/unicorn/configurator.rb |  8 +++++---
 test/unit/test_signals.rb   | 26 ++++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/TODO b/TODO
index 204bb7d..085ef70 100644
--- a/TODO
+++ b/TODO
@@ -2,8 +2,6 @@
 
   * integration tests with nginx including bad client handling
 
-  * tests for timeout
-
   * manpages (why do so few Ruby executables come with proper manpages?)
 
 == 1.1.0
diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index f753ea2..2b528fa 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -375,9 +375,8 @@ module Unicorn
     # is stale for >@timeout seconds, then we'll kill the corresponding
     # worker.
     def murder_lazy_workers
-      now = Time.now
       WORKERS.each_pair do |pid, worker|
-        (now - worker.tempfile.ctime) <= @timeout and next
+        Time.now - worker.tempfile.ctime <= @timeout and next
         logger.error "worker=#{worker.nr} PID:#{pid} is too old, killing"
         kill_worker(:KILL, pid) # take no prisoners for @timeout violations
         worker.tempfile.close rescue nil
diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 6f49905..7724ff0 100644
--- a/lib/unicorn/configurator.rb
+++ b/lib/unicorn/configurator.rb
@@ -143,12 +143,14 @@ module Unicorn
     # handling the request/app.call/response cycle taking longer than
     # this time period will be forcibly killed (via SIGKILL).  This
     # timeout is enforced by the master process itself and not subject
-    # to the scheduling limitations by the worker process.
+    # to the scheduling limitations by the worker process.  Due the
+    # low-complexity, low-overhead implementation, timeouts of less
+    # than 2.0 seconds can be considered inaccurate and unsafe.
     def timeout(seconds)
       Numeric === seconds or raise ArgumentError,
                                   "not numeric: timeout=#{seconds.inspect}"
-      seconds > 0 or raise ArgumentError,
-                                  "not positive: timeout=#{seconds.inspect}"
+      seconds >= 2 or raise ArgumentError,
+                                  "too low: timeout=#{seconds.inspect}"
       @set[:timeout] = seconds
     end
 
diff --git a/test/unit/test_signals.rb b/test/unit/test_signals.rb
index bedce01..d8af285 100644
--- a/test/unit/test_signals.rb
+++ b/test/unit/test_signals.rb
@@ -37,6 +37,32 @@ class SignalsTest < Test::Unit::TestCase
     @server = nil
   end
 
+  def test_timeout_slow_response
+    pid = fork {
+      app = lambda { |env| sleep }
+      opts = @server_opts.merge(:timeout => 2)
+      redirect_test_io { HttpServer.new(app, opts).start.join }
+    }
+    t0 = Time.now
+    sock = nil
+    assert_nothing_raised do
+      sock = TCPSocket.new('127.0.0.1', @port)
+      sock.syswrite("GET / HTTP/1.0\r\n\r\n")
+    end
+
+    buf = nil
+    assert_raises(EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,
+                  Errno::EBADF) do
+      buf = sock.sysread(4096)
+    end
+    diff = Time.now - t0
+    assert_nil buf
+    assert diff > 1.0, "diff was #{diff.inspect}"
+    assert diff < 60.0
+    ensure
+      Process.kill(:QUIT, pid) rescue nil
+  end
+
   def test_response_write
     app = lambda { |env|
       [ 200, { 'Content-Type' => 'text/plain', 'X-Pid' => Process.pid.to_s },
-- 
cgit v1.2.3-24-ge0c7