From 3d22c178d766d0601b75f5c0de7ee0696745c41c Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 13 Feb 2009 00:08:33 -0800 Subject: Remove tempfile reuse from HttpRequest, upload tests Tempfile reuse was over-engineered and the problem was not nearly as big a problem as initially thought. Additionally, it could lead to a subtle bug in an applications that link(2)s or rename(2)s the temporary file to a permanent location _without_ closing it after the request is done. Applications that suffer from the problem of directory bloat are still free to modify ENV['TMPDIR'] to influence the creation of Tempfiles. --- README | 6 -- lib/unicorn/http_request.rb | 7 +-- test/unit/test_upload.rb | 150 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 10 deletions(-) create mode 100644 test/unit/test_upload.rb diff --git a/README b/README index 04b9440..45f0a8e 100644 --- a/README +++ b/README @@ -107,12 +107,6 @@ proxy we know of that meets this requirement. unportable event notification solutions for watching few file descriptors. - * Since workers only work on one client at a time, the temporary - file for storing large POST/PUT requests is reused for the - lifetime of the process. This should limit temp directory - growth on UNIX filesystems where temp file names are never - purged from the containing directory. - * If the master process dies unexpectedly for any reason, workers will notice within :timeout/2 seconds and follow the master to its death. diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index 47600d6..7e7166b 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -15,7 +15,7 @@ module Unicorn def initialize(logger) @logger = logger - @tempfile = @body = nil + @body = nil @buffer = ' ' * Const::CHUNK_SIZE # initial size, may grow @parser = HttpParser.new @params = Hash.new @@ -24,7 +24,6 @@ module Unicorn def reset @parser.reset @params.clear - @body.truncate(0) rescue nil @body.close rescue nil @body = nil end @@ -99,8 +98,7 @@ module Unicorn # small body, just use that @body = StringIO.new(http_body) else # huge body, put it in a tempfile - @tempfile ||= Tempfile.new(Const::UNICORN_TMP_BASE) - @body = File.open(@tempfile.path, "wb+") + @body = Tempfile.new(Const::UNICORN_TMP_BASE) @body.sync = true @body.syswrite(http_body) end @@ -162,6 +160,7 @@ module Unicorn # Any errors means we should delete the file, including if the file # is dumped. Truncate it ASAP to help avoid page flushes to disk. + @body.truncate(0) rescue nil reset false end diff --git a/test/unit/test_upload.rb b/test/unit/test_upload.rb new file mode 100644 index 0000000..a2ccc54 --- /dev/null +++ b/test/unit/test_upload.rb @@ -0,0 +1,150 @@ +# Copyright (c) 2009 Eric Wong +require 'test/test_helper' + +include Unicorn + +class UploadTest < Test::Unit::TestCase + + def setup + @addr = ENV['UNICORN_TEST_ADDR'] || '127.0.0.1' + @port = unused_port + @hdr = {'Content-Type' => 'text/plain', 'Content-Length' => '0'} + @bs = 4096 + @count = 256 + @server = nil + + # we want random binary data to test 1.9 encoding-aware IO craziness + @random = File.open('/dev/urandom','rb') + @sha1 = Digest::SHA1.new + @sha1_app = lambda do |env| + input = env['rack.input'] + resp = { :pos => input.pos, :size => input.stat.size } + begin + loop { @sha1.update(input.sysread(@bs)) } + rescue EOFError + end + resp[:sha1] = @sha1.hexdigest + [ 200, @hdr.merge({'X-Resp' => resp.inspect}), [] ] + end + end + + def teardown + redirect_test_io { @server.stop(true) } if @server + end + + def test_put + start_server(@sha1_app) + sock = TCPSocket.new(@addr, @port) + sock.syswrite("PUT / HTTP/1.0\r\nContent-Length: #{length}\r\n\r\n") + @count.times do + buf = @random.sysread(@bs) + @sha1.update(buf) + sock.syswrite(buf) + end + read = sock.read.split(/\r\n/) + assert_equal "HTTP/1.1 200 OK", read[0] + resp = eval(read.grep(/^X-Resp: /).first.sub!(/X-Resp: /, '')) + assert_equal length, resp[:size] + assert_equal 0, resp[:pos] + assert_equal @sha1.hexdigest, resp[:sha1] + end + + + def test_put_keepalive_truncates_small_overwrite + start_server(@sha1_app) + sock = TCPSocket.new(@addr, @port) + to_upload = length + 1 + sock.syswrite("PUT / HTTP/1.0\r\nContent-Length: #{to_upload}\r\n\r\n") + @count.times do + buf = @random.sysread(@bs) + @sha1.update(buf) + sock.syswrite(buf) + end + sock.syswrite('12345') # write 4 bytes more than we expected + @sha1.update('1') + + read = sock.read.split(/\r\n/) + assert_equal "HTTP/1.1 200 OK", read[0] + resp = eval(read.grep(/^X-Resp: /).first.sub!(/X-Resp: /, '')) + assert_equal to_upload, resp[:size] + assert_equal 0, resp[:pos] + assert_equal @sha1.hexdigest, resp[:sha1] + end + + def test_put_excessive_overwrite_closed + start_server(lambda { |env| [ 200, @hdr, [] ] }) + sock = TCPSocket.new(@addr, @port) + buf = ' ' * @bs + sock.syswrite("PUT / HTTP/1.0\r\nContent-Length: #{length}\r\n\r\n") + @count.times { sock.syswrite(buf) } + assert_raise Errno::ECONNRESET do + ::Unicorn::Const::CHUNK_SIZE.times { sock.syswrite(buf) } + end + end + + def test_put_handler_closed_file + nr = '0' + start_server(lambda { |env| + env['rack.input'].close + resp = { :nr => nr.succ! } + [ 200, @hdr.merge({ 'X-Resp' => resp.inspect}), [] ] + }) + sock = TCPSocket.new(@addr, @port) + buf = ' ' * @bs + sock.syswrite("PUT / HTTP/1.0\r\nContent-Length: #{length}\r\n\r\n") + @count.times { sock.syswrite(buf) } + read = sock.read.split(/\r\n/) + assert_equal "HTTP/1.1 200 OK", read[0] + resp = eval(read.grep(/^X-Resp: /).first.sub!(/X-Resp: /, '')) + assert_equal '1', resp[:nr] + + # server still alive? + sock = TCPSocket.new(@addr, @port) + sock.syswrite("GET / HTTP/1.0\r\n\r\n") + read = sock.read.split(/\r\n/) + assert_equal "HTTP/1.1 200 OK", read[0] + resp = eval(read.grep(/^X-Resp: /).first.sub!(/X-Resp: /, '')) + assert_equal '2', resp[:nr] + end + + def test_renamed_file_not_closed + start_server(lambda { |env| + new_tmp = Tempfile.new('unicorn_test') + input = env['rack.input'] + File.rename(input.path, new_tmp) + resp = { + :inode => input.stat.ino, + :size => input.stat.size, + :new_tmp => new_tmp.path, + :old_tmp => input.path, + } + [ 200, @hdr.merge({ 'X-Resp' => resp.inspect}), [] ] + }) + sock = TCPSocket.new(@addr, @port) + buf = ' ' * @bs + sock.syswrite("PUT / HTTP/1.0\r\nContent-Length: #{length}\r\n\r\n") + @count.times { sock.syswrite(buf) } + read = sock.read.split(/\r\n/) + assert_equal "HTTP/1.1 200 OK", read[0] + resp = eval(read.grep(/^X-Resp: /).first.sub!(/X-Resp: /, '')) + new_tmp = File.open(resp[:new_tmp]) + assert_equal resp[:inode], new_tmp.stat.ino + assert_equal length, resp[:size] + assert ! File.exist?(resp[:old_tmp]) + assert_equal resp[:size], new_tmp.stat.size + end + + private + + def length + @bs * @count + end + + def start_server(app) + redirect_test_io do + @server = HttpServer.new(app, :listeners => [ "#{@addr}:#{@port}" ] ) + @server.start + end + end + +end -- cgit v1.2.3-24-ge0c7