Mongrel users - Jan 2007 - Want feedback on Mongrel patch for handling partial PUT requests

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

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)

On Jan 22, 2007, at 8:31 PM, Ezra Zygmuntowicz wrote:
>
> 	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.
>>
(snip)

I emailed the patches to Ezra. If anyone else wants them, please  
email me off list. I found a few bugs & problems after I actually  
typed this stuff in and ran it. Those bugs are fixed in the patches.

cr

On Jan 23, 2007, at 12:09 AM, Zed A. Shaw wrote:
> On Mon, 22 Jan 2007 19:27:35 -0600
> 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.
>
> I''ll take a look this weekend, but you really should have tests to
> go with this just to make sure I''m trying it the way you are
trying
> it.
Here are the patches against Mongrel 1.0 plus a test file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_upload.rb
Type: text/x-ruby-script
Size: 7680 bytes
Desc: not available
Url :
http://rubyforge.org/pipermail/mongrel-users/attachments/20070122/d93f08dc/attachment-0001.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: handlers.diff
Type: application/octet-stream
Size: 267 bytes
Desc: not available
Url :
http://rubyforge.org/pipermail/mongrel-users/attachments/20070122/d93f08dc/attachment-0002.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mongrel.diff
Type: application/octet-stream
Size: 2951 bytes
Desc: not available
Url :
http://rubyforge.org/pipermail/mongrel-users/attachments/20070122/d93f08dc/attachment-0003.obj
-------------- next part --------------
> Also, seeing how it can improve or change the upload progress  
> problem (the fact that it sucks) would be a good first step.
That problem was a bit out of scope for me so I don''t think this  
helps much. The common assumption is that people are using POST for  
uploads which this patch doesn''t address at all.
> Otherwise, I''ll look and get back to you.  Sorry I didn''t
get in
> touch with you earlier.  Just been busy.
No problem; we''re all busy!

cr

On Mon, 22 Jan 2007 19:27:35 -0600
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.
I''ll take a look this weekend, but you really should have tests to go
with this just to make sure I''m trying it the way you are trying it.

Also, seeing how it can improve or change the upload progress problem (the fact
that it sucks) would be a good first step.

Otherwise, I''ll look and get back to you.  Sorry I didn''t get
in touch with you earlier.  Just been busy.

-- 
Zed A. Shaw, MUDCRAP-CE Master Black Belt Sifu
http://www.zedshaw.com/
http://www.awprofessional.com/title/0321483502 -- The Mongrel Book
http://mongrel.rubyforge.org/
http://www.lingr.com/room/3yXhqKbfPy8 -- Come get help.