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.

3 files changed, 153 insertions, 10 deletions

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