[Mongrel] Want feedback on Mongrel patch for handling partial PUT requests

cremes.devlist at mac.com cremes.devlist at mac.com
Mon Jan 22 20:27:35 EST 2007


By default, Mongrel will delete the HTTP request body and short  
circuit calling any handlers if a request is interrupted or  
incomplete. Unfortunately, this breaks any attempt to correctly  
handle a partial PUT. (BTW, PUT is *way* more efficient for uploads  
compared to POST which requires a MIME parsing step.) So, about a  
month ago I wrote up some patches to Mongrel 0.3.18 that allowed  
Mongrel to properly handle partial PUT requests. I sent the patch &  
tests to Zed for comment but never really heard back.

It occurred to me that there might be a better way to handle things,  
so here's my second stab at writing a patch that I hope will be  
accepted into the Mongrel trunk. This patch was written against  
Mongrel 1.0. The original patch detected if the request was a PUT and  
ran various special case code. This approach makes handling partial  
or interrupted requests in a generic way and pushes the  
responsibility for accepting or rejecting them back onto the handler.

If there's no chance this will be accepted into trunk, please let me  
know. I'd be happy to provide this as a "plugin" to monkey-patch  
mongrel for any people interested in this functionality.

So, the patch touches three (3) classes and makes minor modifications.

1. class HttpHandler
Adds a single method called #accept_partial_requests? which is false  
by default since most handlers do NOT want to handle a partial  
request. Those that do can simply override this method in the  
subclass. Used in HttpServer#process_client for determining if a  
handler should get called.

2. class HttpRequest
Changes the logic in the #read_body method so it no longer deletes  
the TempFile and sets @body to nil as a "signal" that the request was  
interrupted. Adds a #attr_reader called #interrupted? for use outside  
the class along with the appropriate initialization statement (this  
is the new "signal"). Lastly, adds a method called #cleanup which  
essentially restores the original activity from #read_body and  
deletes the TempFile.

3. class HttpServer
Some logic changes to the #process_client method which is the main  
workhorse of the class. It deletes the original statement that short  
circuits the loop ( break if request.body == nil) and instead pushes  
that check to the loop invoking all the handlers. The handler loop  
now checks each handler to verify #handler.accept_partial_requests?  
before passing a partial request to the handler. If it's a complete  
request, the original calling semantics apply. This means all  
existing handlers would continue to function as they did before. Only  
those handlers which specifically override #accept_partial_requests?  
will ever get called with a partial/malformed request.

I'm a little concerned that these extra checks would be a performance  
drag in the general case. Any feedback on improvements here would be  
much appreciated.

Lastly, the end of the method then calls #request.cleanup to delete  
any lingering TempFile.


Anyway, enough talk... time for the code. It's all listed below,  
though currently *untested*. I want to get feedback from Zed and the  
group before I take the extra step of adding and testing all of this  
(though I expect it to pass all tests just like the original patch).

Thanks for your attention.

cr

---- code below ----

module Mongrel
   class HttpHandler
     # NEW
     def accept_partial_requests?
       false
     end
   end

   class HttpRequest
     # NEW
     attr_reader :interrupted?

     def initialize
       # original code plus...
       self.interrupted? = false
     end

     # modify #read_body so it does NOT delete the @body if an  
exception is raised.
     def read_body(remain, total)
       begin
         # write the odd sized chunk first
         @params.http_body = read_socket(remain % Const::CHUNK_SIZE)

         remain -= @body.write(@params.http_body)

         update_request_progress(remain, total)

         # then stream out nothing but perfectly sized chunks
         until remain <= 0 or @socket.closed?
           # ASSUME: we are writing to a disk and these writes always  
write the requested amount
           @params.http_body = read_socket(Const::CHUNK_SIZE)
           remain -= @body.write(@params.http_body)

           update_request_progress(remain, total)
         end
       rescue Object
         STDERR.puts "ERROR reading http body: #$!"
         $!.backtrace.join("\n")
         @socket.close rescue Object
         self.interrupted? = true # NEW
       end
     end

     # NEW
     # clean up any lingering temp files
     def cleanup
       @body.delete if @body.class == Tempfile && interrupted?
     end
   end


   class HttpServer
     def process_client(client)
       begin
         # deleted for brevity

         while nparsed < data.length
           nparsed = parser.execute(params, data, nparsed)

           if parser.finished?
             # deleted for brevity

             if handlers
               # deleted for brevity

               # select handlers that want more detailed request  
notification
               notifiers = handlers.select { |h| h.request_notify }
               request = HttpRequest.new(params, client, notifiers)

               # break if request.body == nil ---- REMOVED

               # request is good so far, continue processing the  
response
               response = HttpResponse.new(client) unless  
request.interrupted? # NEW statement modifier

               # Process each handler in registered order until we  
run out or one finalizes the response.
               if request.interrupted?
                 handlers.each do |handler|
                   handler.process(request, nil) if  
handler.accept_partial_requests
                 end
               else
                 # ORIGINAL logic
                 handlers.each do |handler|
                   handler.process(request, response)
                   break if response.done or client.closed?
                 end
               end

               # NEW
               # In the case of large file uploads the user could  
close the socket, so the request is in a
               # partial state. Clean up after it by removing any  
TempFile that may exist.
               request.cleanup if request.interrupted?

               # And finally, if nobody closed the response off, we  
finalize it.
               unless response.done or client.closed?
                 response.finished
               end
             else
               # brevity!
             end

             break #done
           else
             # deleted for brevity
           end
         end
       rescue  
EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF
         # deleted for brevity
       end
     end
   end
end



More information about the Mongrel-users mailing list