diff options
author | Eric Wong <normalperson@yhbt.net> | 2009-05-03 16:16:48 -0700 |
---|---|---|
committer | Eric Wong <normalperson@yhbt.net> | 2009-05-03 23:29:03 +0000 |
commit | df76b78d359f34a73ce407f40f594577e04b014b (patch) | |
tree | d7d75b2c850760b07b0e65aa0008b781133732f0 | |
parent | 895ae151c8703ebcd52becf5f2ca297d21274ef6 (diff) | |
download | unicorn-df76b78d359f34a73ce407f40f594577e04b014b.tar.gz |
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.
-rw-r--r-- | TODO | 2 | ||||
-rw-r--r-- | lib/unicorn.rb | 3 | ||||
-rw-r--r-- | lib/unicorn/configurator.rb | 8 | ||||
-rw-r--r-- | test/unit/test_signals.rb | 26 |
4 files changed, 32 insertions, 7 deletions
@@ -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 }, |