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

Ezra Zygmuntowicz ezmobius at gmail.com
Mon Jan 22 21:31:34 EST 2007


	I would like to play with this a bit as it changes the way the  
mongrel_upload_progress does a request_aborted to cleanup. That  
plugin relies on the @body being nil to work but I think that could  
be worked around in your approach.

	Can you please send an svn diff of this file? I don't feel like copy  
pasting all that jazz.

Thanks
-Ezra


On Jan 22, 2007, at 5:27 PM, cremes.devlist at mac.com wrote:

> 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
>
> _______________________________________________
> Mongrel-users mailing list
> Mongrel-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/mongrel-users
>

-- Ezra Zygmuntowicz 
-- Lead Rails Evangelist
-- ez at engineyard.com
-- Engine Yard, Serious Rails Hosting
-- (866) 518-YARD (9273)




More information about the Mongrel-users mailing list